Skip to content
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.

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

init: nixgl module #5355

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

init: nixgl module #5355

wants to merge 1 commit into from

Conversation

Smona
Copy link

@Smona Smona commented Apr 30, 2024

Description

This PR integrates nixGL into home manager in a way that does not introduce a nixGL dependency, and attempts to make user configuration as simple as possible. This accomplishes the same goal as #5332, namely making it simpler for users to run applications which require libGL on non-nixos systems managed by home manager. There's some elaboration on why I think this is a better API in the original issue.

It makes use of a wrapper which I have been using for some months to run a few GPU-bound applications on Ubuntu. Judging by post reactions it seems that many other users have also found success with this wrapper. By default it is a a no-op, but if a user has configured a nixGL.prefix, then it will symlink all files from the original package into a new one, except for binaries which are replaced by a wrapper script calling them with the configured prefix. Unlike other wrappers proposed in #3968, all derivation attributes from the original package are preserved, so the result of the wrapper can be passed along to program modules for additional overriding. And since the program binary itself is replaced, the program will be launched in the nixGL context regardless of where it is called from (terminal, desktop file, etc).

The wrapper function is also made available on lib.nixGL.wrap, so users can wrap applications which don't have a HM module, or which they want to otherwise override (e.g. to a different version) like so:

    home.packages = [ (config.lib.nixGL.wrap pkgs.example) ];

I'm very open to changing option/wrapper paths if it would help get this merged. Just having the wrapper and the option would be enough for users to wrap packages with nixGL ergonomically.

Wrapping packages by default

The default no-op behavior and derivation attribute preservation of this wrapper means that HM modules for applications could wrap their default package with nixGL. This would have the benefit of tracking which applications need nixGL to function properly on non-nixos systems, and saving end users time by making nixGL.prefix the only option they need to set to get these applications working. If a user wanted to undo the default wrapping for certain applications, they could simply override programs.<application>.package to a non-wrapped package. This PR originally included examples for kitty and firefox, which both benefit from nixGL on non-nixos systems. However, @exzombie brought up some valid concerns about doing this, particularly detecting and preventing double-wrapping, so in the interest of getting the wrapper itself merged this default wrapping has been removed, to be left for future improvement.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

    TODO

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

@kira-bruneau @rycee

Alternative to #5332
Closes #3968

@jonahbron
Copy link

This is great! Would it work for passing into programs.${program}.package as well?

@Smona
Copy link
Author

Smona commented May 1, 2024

@jonahbron yes! the wrapped package can be used interchangeably with the original one, so you can pass it to any option that's expecting a package.

@neiios
Copy link
Contributor

neiios commented May 4, 2024

Just tried out the PR, and it seems to not work with packages like easyeffects that use a separate debug output. At the same time, the module works well with other programs like mpv.

# this one doesnt work for me
services.easyeffects = {
  enable = true;
  package = (config.lib.nixGL.wrap pkgs.easyeffects);
};

When using the snippet above I get this error trace:

error: builder for '/nix/store/2gkv48bzdgrvkd6vnscjbgc8y175hmnv-nixGL-easyeffects-7.1.6.drv' failed to produce output path for output 'debug' at '/nix/store/2gkv48bzdgrvkd6vnscjbgc8y175hmnv-nixGL-easyeffects-7.1.6.drv.chroot/nix/store/aw3ajpz9s05n44kmzjgc2844y5qw15sb-nixGL-easyeffects-7.1.6-debug'
error: 1 dependencies of derivation '/nix/store/q1y0c3d2lpb5djkxr38m7823abyjkpzz-easyeffects.service.drv' failed to build
error: 1 dependencies of derivation '/nix/store/mn47qf0y738dmap03p38b5pspalckcnw-home-manager-path.drv' failed to build
error: 1 dependencies of derivation '/nix/store/3fld8n8hwynfhq4pbchhbrf6vr6rwlqj-man-paths.drv' failed to build
error: 1 dependencies of derivation '/nix/store/b5mqsb81l7y7vrs2y4nlr21jx9w0q6hk-nixGL-easyeffects-7.1.6-fish-completions.drv' failed to build
error: 1 dependencies of derivation '/nix/store/317843r43f295ad9sylz9mlysyrs8caw-home-manager-generation.drv' failed to build

@Smona can this issue be reproduced on your end?

@Smona
Copy link
Author

Smona commented May 4, 2024

@neiios thank you for testing it! That's an interesting test case since I can't imagine easyeffects benefiting from nixGL. But I could see this potentially affecting other packages with debug outputs.

I'm able to reproduce your issue with the same configuration, but interestingly that configuration builds just fine for me if I use the same wrapper as config.lib.nixGL.wrap imported from a local file. Going to have to look a bit deeper.

@michaelCTS michaelCTS mentioned this pull request May 6, 2024
6 tasks
@karmakarmeghdip
Copy link

Any updates on this? Will this be merged?

@Smona
Copy link
Author

Smona commented May 6, 2024

@neiios can you try again with 680108f? Overriding separateDebugInfo to false fixed your test case for me!

@neiios
Copy link
Contributor

neiios commented May 6, 2024

@neiios can you try again with 680108f? Overriding separateDebugInfo to false fixed your test case for me!

On the latest commit, easyeffects works for me as well!

@jonahbron
Copy link

jonahbron commented May 8, 2024

I tried the wrapper with Prusa Slicer today but it did not make a difference, the 3D viewer didn't work either way (it does when I download and install the binary from the website). Happy to help debug of there's any info I can provide. Here's the output:

$ prusa-slicer
Gtk-Message: 09:12:14.981: Failed to load module "canberra-gtk-module"
Gtk-Message: 09:12:14.983: Failed to load module "canberra-gtk-module"

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.117: gtk_cell_layout_get_cells: assertion 'GTK_IS_CELL_LAYOUT (cell_layout)' failed

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.118: gtk_cell_layout_get_cells: assertion 'GTK_IS_CELL_LAYOUT (cell_layout)' failed

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.120: gtk_cell_layout_get_cells: assertion 'GTK_IS_CELL_LAYOUT (cell_layout)' failed

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.121: gtk_cell_layout_get_cells: assertion 'GTK_IS_CELL_LAYOUT (cell_layout)' failed

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.123: gtk_cell_layout_get_cells: assertion 'GTK_IS_CELL_LAYOUT (cell_layout)' failed

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.123: gtk_cell_layout_get_cells: assertion 'GTK_IS_CELL_LAYOUT (cell_layout)' failed

(prusa-slicer:58991): GLib-WARNING **: 09:12:15.166: getpwuid_r(): failed due to unknown user id (750198)

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.309: gtk_widget_set_size_request: assertion 'width >= -1' failed

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.309: gtk_widget_set_size_request: assertion 'width >= -1' failed

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.318: gtk_cell_layout_get_cells: assertion 'GTK_IS_CELL_LAYOUT (cell_layout)' failed

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.318: gtk_cell_layout_get_cells: assertion 'GTK_IS_CELL_LAYOUT (cell_layout)' failed

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.319: gtk_cell_layout_get_cells: assertion 'GTK_IS_CELL_LAYOUT (cell_layout)' failed

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.319: gtk_cell_layout_get_cells: assertion 'GTK_IS_CELL_LAYOUT (cell_layout)' failed

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.320: gtk_cell_layout_get_cells: assertion 'GTK_IS_CELL_LAYOUT (cell_layout)' failed

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.320: gtk_cell_layout_get_cells: assertion 'GTK_IS_CELL_LAYOUT (cell_layout)' failed

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.320: gtk_cell_layout_get_cells: assertion 'GTK_IS_CELL_LAYOUT (cell_layout)' failed

(prusa-slicer:58991): Gtk-CRITICAL **: 09:12:15.321: gtk_cell_layout_get_cells: assertion 'GTK_IS_CELL_LAYOUT (cell_layout)' failed
09:12:17: Debug: window wxTreeCtrl@0x75bedb0 ("treeCtrl") lost focus even though it didn't have it
09:12:17: Debug: window wxTreeCtrl@0x75bedb0 ("treeCtrl") lost focus even though it didn't have it
09:12:17: Debug: window wxTreeCtrl@0x75bedb0 ("treeCtrl") lost focus even though it didn't have it
09:12:17: Debug: window wxTreeCtrl@0x75bedb0 ("treeCtrl") lost focus even though it didn't have it
09:12:17: Debug: window wxTreeCtrl@0x75bedb0 ("treeCtrl") lost focus even though it didn't have it
09:12:17: Debug: window wxTreeCtrl@0x75bedb0 ("treeCtrl") lost focus even though it didn't have it
09:12:17: Debug: window wxTreeCtrl@0x75bedb0 ("treeCtrl") lost focus even though it didn't have it
09:12:17: Debug: window wxTreeCtrl@0x75bedb0 ("treeCtrl") lost focus even though it didn't have it

@Smona
Copy link
Author

Smona commented May 8, 2024

@jonahbron I don't see any libGL/GLX related errors in those logs, so my guess would be that those packaging issues are unrelated to this change? Especially given that Gtk-CRITICAL errors are considered known and ignored by the project. I would expect that if nixGL weren't working properly here, you would see glXChooseFBConfig failures instead.

@exzombie
Copy link

I finally found some time to look at this. I like the approach, it is elegant. It falls a bit short, though. I'll work on extending if, hopefully later today. But I wanted to write this comment immediately just in case something comes up (as it often does) preventing me from working on this for another week.

The current implementation of wrapping falls short on packages that contain .desktop files which refer to the full path to the executable. For these packages, the binary is wrapped, but the .desktop file refers to the unwrapped binary. I have a solution to that, and plan to refactor it for inclusion into the module.

Using a single wrapper prefix can only provide OpenGL from a single vendor. This is insufficient because you surely also want Vulkan, and you may want some flexibility as to which vendor you use, e.g. for Prime offloading. This stuff gets hairy pretty fast. My own wrappers are more complete, and also quite straightforward to use, but the implementation is quite hairy. I'll try to refactor them and propose them for inclusion in this PR. My goal is to allow the user to fully use an Optimus laptop without having to know the internals of nixGL. And to keep things simple for people who don't have an Optimus laptop.

And last, I'm not convinced wrapping some packages by default the way it's done now is a good idea. If we wrap every package that needs this, it would be great, but it's not feasible. If we wrap just a couple of common packages, I imagine users would be stumped as to why some packages don't work while others do. If, instead, we wrap nothing by default, and clearly document (e.g. in the User Manual) that anything needing graphics acceleration needs to be wrapped and how to do it, it would empower users.

But, in the end, I don't have strong feelings about this. If someone wants to invest effort into wrapping lots of stuff by default, I wouldn't mind too much. Just note that the wrapper would need to be extended so as to detect and avoid double wrapping: I think it's a given that the project of default wrapping would take some time, and in the meantime, users would wrap stuff themselves. This would cause issues later when HM starts wrapping a package that wasn't wrapped before.

Which begs the question: has anyone tried simply wrapping all the packages in their home configuration? Perhaps it doesn't cause any problems, and we could go with that?

This module enables wrapping programs which require access to libGL with
nixGL on non-NixOS systems.
@Smona
Copy link
Author

Smona commented May 23, 2024

@exzombie thank you for the careful review! You brought up some very good points regarding wrapping packages by default, particularly the double-wrapping concern, so I've removed those changes from this PR for now.

I'll review the rest of your proposed changes ASAP, since you've brought up a use case I didn't really consider. It looks like you primarily want to support customizing the opengl vendor on a per-package basis, but I also want to make sure config modules that use this wrapper can be easily re-used across systems with different graphics device setups for people like me who are reusing the same HM modules across multiple systems in a flake. We may want to just leave that up to user-defined settings which get passed into the wrapper along with the package, which I think your change would support. but I want to make sure I understand both use cases and how they interact first.

Copy link

@giggio giggio left a comment

Choose a reason for hiding this comment

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

See how I tested it: #3968 (comment)

Comment on lines +56 to +58
echo "#!${pkgs.bash}/bin/bash" > "$out/bin/$(basename $file)"
echo "exec -a \"\$0\" ${cfg.prefix} $file \"\$@\"" >> "$out/bin/$(basename $file)"
chmod +x "$out/bin/$(basename $file)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Use makeWrapper may better.

Suggested change
echo "#!${pkgs.bash}/bin/bash" > "$out/bin/$(basename $file)"
echo "exec -a \"\$0\" ${cfg.prefix} $file \"\$@\"" >> "$out/bin/$(basename $file)"
chmod +x "$out/bin/$(basename $file)"
local prog="$(basename "$file")"
makeWrapper \
"${cfg.prefix}" \
"$prog" \
--argv0 "$prog" \
--add-flags "$file"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature request: nixGL integration
7 participants