8000 gpu-screen-recorder{,-gtk}: add passthru.updateScript, update by js6pak · Pull Request #367552 · NixOS/nixpkgs · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

gpu-screen-recorder{,-gtk}: add passthru.updateScript, update #367552

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Jun 6, 2025
Merged

gpu-screen-recorder{,-gtk}: add passthru.updateScript, update #367552

merged 7 commits into from
Jun 6, 2025

Conversation

js6pak
Copy link
Member
@js6pak js6pak commented Dec 23, 2024 •
edited
Loading

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

8000
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Dec 23, 2024
keenanweaver
keenanweaver approved these changes Dec 23, 2024
View reviewed changes
Copy link
Member
@keenanweaver keenanweaver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know how to test it locally, but it looks great.

@keenanweaver keenanweaver mentioned this pull request Dec 23, 2024
Closed
13 tasks
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Dec 23, 2024
@ofborg ofborg bot requested a review from babbaj December 23, 2024 17:44
@keenanweaver
Copy link
Member

5.0.0 is now out: https://git.dec05eba.com/gpu-screen-recorder/

keenanweaver
keenanweaver suggested changes Dec 31, 2024
View reviewed changes
@js6pak
Copy link
Member Author
js6pak commented Dec 31, 2024

There is also gpu-screen-recorder-ui now, which is interesting

@keenanweaver
Copy link
Member

There is also gpu-screen-recorder-ui now, which is interesting

I was just opening a packaging request for this. Looks like someone already did some preliminary work: https://github.com/Nowaaru/nix-diary/tree/6230e94b178924e7455a1dc0d318f50e10c8aec6/modules/gpu-screen-recorder/packages.

The module would probably need to be updated as well.

@js6pak js6pak force-pushed the gpu-screen-recorder/update-script branch from 3981d06 to f48ec9d Compare December 31, 2024 02:51
@js6pak js6pak mentioned this pull request Dec 31, 2024
Open
13 tasks
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Dec 31, 2024
@keenanweaver keenanweaver added the 12.approvals: 1 This PR was reviewed and approved by one person. label Dec 31, 2024
@js6pak js6pak force-pushed the gpu-screen-recorder/update-script branch from f48ec9d to 092f692 Compare February 3, 2025 00:09
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Feb 4, 2025
keenanweaver
keenanweaver approved these changes Feb 7, 2025
View reviewed changes
Copy link
Member
@keenanweaver keenanweaver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 367552

x86_64-linux

✅ 2 packages built:
  • gpu-screen-recorder
  • gpu-screen-recorder-gtk

@keenanweaver keenanweaver added the 12.approvals: 1 This PR was reviewed and approved by one person. label Feb 7, 2025
donovanglover
donovanglover reviewed Feb 8, 2025
View reviewed changes
pkgs/by-name/gp/gpu-screen-recorder-gtk/package.nix
}:

stdenv.mkDerivation (finalAttrs: {
stdenv.mkDerivation rec {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
stdenv.mkDerivation rec {
stdenv.mkDerivation (finalAttrs: {

https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using finalAttrs instead of rec for version here would lead to #310373. I don't think there is a consensus on the best solution, but rec works fine here.

pkgs/by-name/gp/gpu-screen-recorder-gtk/package.nix
url = "https://dec05eba.com/snapshot/gpu-screen-recorder-gtk.git.${finalAttrs.version}.tar.gz";
hash = "sha256-uXbiuA1XPWZVwQGLh47rKzCZSEUEPWqYALqMuCGA7do=";
src = fetchgit {
url = "https://repo.dec05eba.com/${pname}";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
url = "https://repo.dec05eba.com/${pname}";
url = "https://repo.dec05eba.com/gpu-screen-recorder-gtk";

#277994

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rec being constant doesn't introduce any overriding traps here, so whether you use ${pname} or a literal value is a subjective choice. Imo this is nicer because it can be shared across all gpu-screen-recorder-* packages.

@js6pak js6pak force-pushed the gpu-screen-recorder/update-script branch from 092f692 to 8b1deb5 Compare February 8, 2025 20:34
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Feb 9, 2025
keenanweaver
keenanweaver suggested changes Feb 27, 2025
View reviewed changes
@js6pak js6pak force-pushed the gpu-screen-recorder/update-script branch from 8b1deb5 to 4236157 Compare March 3, 2025 19:11
@js6pak js6pak requested a review from keenanweaver March 3, 2025 19:12
@js6pak
Copy link
Member Author
js6pak commented Mar 3, 2025

So... how do I get this merged?

@keenanweaver
Copy link
Member

So... how do I get this merged?

Basically have to wait for a committer to review and merge. You could try posting that the PR is ready for review in this thread which I've seen speed up PR reviews. Afraid there's not much else aside from pinging committers which I think is considered taboo.

< 67ED div class="js-timeline-item js-timeline-progressive-focus-container" data-gid="PRR_kwDOAEVQ_M6eQXZq">
keenanweaver
keenanweaver approved these changes Mar 3, 2025
View reviewed changes
Copy link
Member
@keenanweaver keenanweaver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 367552

x86_64-linux

✅ 2 packages built:
  • gpu-screen-recorder
  • gpu-screen-recorder-gtk

@keenanweaver keenanweaver added the 12.approvals: 1 This PR was reviewed and approved by one person. label Mar 3, 2025
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/5256

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two persons. label May 5, 2025
@js6pak js6pak force-pushed the gpu-screen-recorder/update-script branch from eee6cc0 to 281bffb Compare May 8, 2025 15:18
@isabelroses isabelroses removed the 12.approvals: 2 This PR was reviewed and approved by two persons. label May 8, 2025
@dec05eba dec05eba mentioned this pull request May 11, 2025
Closed
13 tasks
@Gliczy
Copy link
Member
Gliczy commented May 11, 2025

I'm not sure why ofborg-eval failed, but this still compiles and run fine.

@keenanweaver
Copy link
Member

The GTK app is no longer being developed.

No need to drop from nixpkgs yet, just something to note.

@rezarajan rezarajan mentioned this pull request May 11, 2025
Closed
13 tasks
@js6pak js6pak force-pushed the gpu-screen-recorder/update-script branch from 281bffb to c34d9ef Compare May 11, 2025 15:32
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels May 11, 2025
@js6pak
< F438 svg aria-label="Show options" role="img" height="16" viewBox="0 0 16 16" version="1.1" width="16" data-view-component="true" class="octicon octicon-kebab-horizontal"> Copy link
Member Author
js6pak commented May 11, 2025

I have added cap_sys_nice back (6480b97) since 5.5.2 is supposed to fix the issue it caused for portal capture, but it still doesn't work for me.

❯ ./result/bin/gpu-screen-recorder --list-capture-options /dev/dri/card0 amd
DP-1|3840x2160
DP-2|2560x1440
region
portal

❯ /run/wrappers/bin/gpu-screen-recorder --list-capture-options /dev/dri/card0 amd
DP-1|3840x2160
DP-2|2560x1440
region
gsr error: gsr_dbus_call_screencast_method: failed with error: Portal operation not allowed: Unable to open /proc/440809/root
gsr error: gsr_dbus_screencast_create_session: failed to setup ScreenCast session. Make sure you have a desktop portal running with support for the ScreenCast interface and that the desktop portal matches the Wayland compositor you are running.
gsr error: gsr_dbus_client_screencast_create_session: server return error: Failed to handle request (-1)

I guess that's caused by our security wrappers making the capabilities ambient?

nixpkgs/nixos/modules/security/wrappers/wrapper.c

Line 199 in c542760

if (make_caps_ambient("/proc/self/exe") != 0) {

@dec05eba
Copy link
Contributor

Hmm seems like it. The fix in 5.5.2 was to move that dbus code to another process (a child process) that runs the included program gsr-dbus-server. Normally a child process doesn't inherit the capability of the parent process since it has cap_sys_nice+ep as opposed to cap_sys_nice+eip.

@js6pak
Copy link
Member Author
js6pak commented May 11, 2025

Adding a prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_CLEAR_ALL, 0, 0, 0); before the execvp here https://git.dec05eba.com/gpu-screen-recorder/tree/dbus/client/dbus_client.c?h=5.5.2&id=0ae5b83e476ed6345c536a438ce5d61e60a11f45#n70, seems to work, but definitely feels hacky.

@dec05eba
Copy link
Contributor

I'll add that then. As long as it works!

@js6pak
Copy link
Member Author
js6pak commented May 11, 2025

@dec05eba Also, how would I test that cap_sys_nice actually works?

@dec05eba
Copy link
Contributor
dec05eba commented May 11, 2025 •
edited
Loading

@js6pak I added the ambient fix you mentioned and released it as part of 5.5.3. With that version it also mentions if cap_sys_nice is not working. If it's not working then it outputs this when you run gpu-screen-recorder --info for example:

amdgpu: amdgpu_cs_ctx_create2 failed. (-13)
gsr warning: cap_sys_nice capability is missing on the gpu-screen-recorder binary, performance might be affected. If you are using the flatpak version of gpu-screen-recorder then the only fix is to use a non-flatpak version of gpu-screen-recorder

@js6pak js6pak force-pushed the gpu-screen-recorder/update-script branch from c34d9ef to 110d7b2 Compare May 11, 2025 17:04
@js6pak
Copy link
Member Author
js6pak commented May 11, 2025

Both cap_sys_nice and portal capture are working now. Thanks @dec05eba!

@js6pak js6pak requested a review from keenanweaver May 11, 2025 18:44
keenanweaver
keenanweaver approved these changes May 12, 2025
View reviewed changes
Copy link
Member
@keenanweaver keenanweaver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still functioning great. I'm hoping this can finally be merged along with #369574

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 367552

x86_64-linux

⏩ 2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
✅ 2 packages built:
  • gpu-screen-recorder
  • gpu-screen-recorder-gtk

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two persons. label May 14, 2025
@pbsbot
Copy link
pbsbot commented Jun 6, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 367552
Commit: 110d7b29b30208a6aaa4e95b8c878e3d6e4c0709

x86_64-linux

⏩ 2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
✅ 2 packages built:
  • gpu-screen-recorder
  • gpu-screen-recorder-gtk

@pbsds pbsds merged commit 9477c8f into NixOS:master Jun 6, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@donovanglover donovanglover donovanglover left review comments

@keenanweaver keenanweaver keenanweaver approved these changes

@Gliczy Gliczy Gliczy approved these changes

@babbaj babbaj Awaiting requested review from babbaj

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 2 This PR was reviewed and approved by two persons.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@js6pak @keenanweaver @nixos-discourse @Gliczy @dec05eba @pbsbot @donovanglover @pbsds @wegank @isabelroses
0