init: targets.genericLinux.gpu module #8057
Conversation
exzombie
force-pushed
the
non-nixos-gpu
branch
2 times, most recently
from
495ca01 to
d5c74ff
Compare
|
Could you clean up the commits a bit. We don't need the history of development, just the final organized atomic commits.
|
exzombie
force-pushed
the
non-nixos-gpu
branch
from
d5c74ff to
d5c4c36
Compare
Done! |
exzombie
force-pushed
the
non-nixos-gpu
branch
from
d5c4c36 to
331c4bf
Compare
|
For the record, running |
That's fine as long as our buildbot outputs are building and CI is green, we're good. I think there are some things we don't build in the pipeline that get run in that all suite. I'm not familiar enough with any of the linux gpu stuff to have a real opinion on the implementation details here. Not sure if @ambroisie or @rycee or anyone else is comfortable reviewing it. |
ambroisie
left a comment
ambroisie
left a comment
There was a problem hiding this comment.
I haven't tested the module, but code-wise this is looking pretty good.
Small suggestions, nothing very impacting.
| set -e | ||
|
|
||
| gcroots=@@resources@@ | ||
| gcroots=${gcroots/store\/*/store\/}../var/nix/gcroots |
There was a problem hiding this comment.
This feels brittle, I wonder if there is a better way of getting the correct path to GC roots.
There was a problem hiding this comment.
That is a good point. When writing this, it seemed that gcroots residing next to the store is a safe assumption. I did some research and it turns out that /nix/var/nix can be moved rather easily; like, there's an environment variable to do so.
The problem is that, this being a runtime configuration parameter of Nix, there's no way to obtain the path during build. We can't do it at runtime, either, because the setup script is supposed to be run with sudo and won't have the same environment available, even the nix executable would be different.
I propose, then, to add a configuration option nixStateDirectory with the default value /nix/var/nix. I think it's reasonable to expect the few users who override this setting to know that they did so.
| customPkgs = pkgs // { | ||
| linuxPackages = pkgs.linuxPackages // { | ||
| nvidiaPackages = pkgs.linuxPackages.nvidiaPackages // { | ||
| mkDriver = args: lib.makeOverridable (_: mockNvidiaDriver) { }; | ||
| }; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Suggestion: having a finalPackage read-only option might make it easier to override with a mock driver package?
There was a problem hiding this comment.
Could you elaborate, please? I don't understand what you mean. Normally, the finalPackage attribute is used to expose the configured package for modules that add plugins to a package and such, with the intent that the user can then use this configured package in other derivations. I don't see anything worth exposing that way in this module. The configured driver environment, perhaps? But to what end? For sure I don't see how it would simplify the test code you quoted.
I added the packages module option because I foresee a common use case: using unstable nixpkgs for graphics drivers while getting the rest of home packages from stable. But to test the module option, this is not a good use case. It's also not easy to test Nvidia support the way it's normally used because there's no way downloading a proprietary driver is acceptable for testing. So, I hit two birds with one stone by using the packages option to provide a mock Nvidia driver. I don't see how the finalPackage pattern would make this simpler.
There was a problem hiding this comment.
I think we could do something like nvidiaPackage = lib.mkForce mkStub ... in the tests, which might be slightly easier.
It wouldn't really provide any value apart from that.
There was a problem hiding this comment.
I see, that's a good suggestion, I wasn't aware of the stubs. But yes, the improvement is slight because mkStubPackage doesn't create an overridable package either. I incorporated it anyway.
exzombie
force-pushed
the
non-nixos-gpu
branch
from
331c4bf to
c36f803
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/launching-applications-installed-through-home-manager/73299/5 |
|
This is great, but how would one go on about contributing it for other init systems? I'm commenting this because I managed to rewrite the service for Shepherd. |
My suggestion is to add a new option |
5 participants
Description
This PR fixes #8033 where the rationale for why this is needed is given. In short, this module builds a set of GPU drivers and provides a command (to be run with
sudo) that installs a Systemd service which sets up/run/opengl-driversymlink on boot. When the home configuration is activated, the symlink is checked and if it is missing or if outdated drivers are found, the user is instructed to run the setup command. I have been using this approach for about two months and it works much better than nixGL.I have put this under
targets.genericLinux, and I have also moved nixGL there for consistency. I think that's the right place; admittedly, I've no idea how things work on darwin, but searching for "darwin" or "macos" in the nixGL repo didn't turn up anything. I have kept the move as a separate commit so that it is easy to undo if needed. But that does mean that half of the commit messages refer to the module under a different name. I can rebase and reword if needed.Checklist
Change is backwards compatible.
Code formatted with
nix fmtornix-shell -p treefmt nixfmt deadnix keep-sorted --run treefmt.Code tested through
nix run .#tests -- test-allornix-shell --pure tests -A run.all.Test cases updated/added. See example.
Commit messages are formatted like
See CONTRIBUTING for more information and recent commit messages for examples.
If this PR adds a new module
If this PR adds an exciting new feature or contains a breaking change.