nvidia-container-toolkit: add suppressNvidiaDriverAssertion option
#362197
Conversation
github-actions
bot
added
6.topic: nixos
github-actions
bot
added
10.rebuild-darwin: 0
|
/cc @SomeoneSerge |
ereslibre
added
12.approvals: 1
wegank
removed
the
12.approved-by: package-maintainer
| @@ -37,6 +37,15 @@ | |||
| ''; | |||
| }; | |||
|
|
|||
| noNvidiaDriverAssertion = lib.mkOption { | |||
There was a problem hiding this comment.
Maybe something more specific like useExternalNvidiaDriver or so?
There was a problem hiding this comment.
I don’t have a very strong position on this. I think noNvidiaDriverAssertion is more explicit to what it does. useExternalNvidiaDriver might signal that there’s some kind of logic to handle that case IMO.
There was a problem hiding this comment.
No strong opinion either, certainly not blocking.
There was a problem hiding this comment.
Regarding the current name, there already is nvidia in hardware.nvidia-container-toolkit so you can reduce this noDriverAssertion. Some people also say that verbs work better than nouns, and that active voice is better than passive which would suggest something like disableDriverAssertion, disableDriverCheck. But also there's no need to have a negation in the name, so you could have checkDriver with default = false (just an mkEnableOption). The less specific name is of little concern because we should include the option name in the assertion's message anyway
@wolfgangwalther makes me think along the lines of replacing the hardware.nvidia.package option with a hardware.nvidia.driver = mkOption { ...; type = oneOf [ package (enum [ "WSL" ]) ], so that the computed locations would special-case for WSL and use the /usr/...` path. Might be over-engineering though
There was a problem hiding this comment.
How about suppressNvidiaDriverAssertion? The name would imply that the assertion is enabled by default and the option can suppress the assertion. I think it's clearer than a mkEnableOption whose default value is true.
There was a problem hiding this comment.
checkDriver with default = false (just an mkEnableOption)
Oh sorry I just got lost in words, that would need to have been checkDriver = mkEnableOption ... // { default = true; }
How about suppressNvidiaDriverAssertion?
I'd still make the argument about omitting "Nvidia". As for suppress/disable/enable/do/dont/etc. I've no strong opinion
wegank
removed
the
12.approvals: 1
nixos/modules/services/hardware/nvidia-container-toolkit/default.nix
Outdated
Show resolved
Hide resolved
wegank
added
the
12.approvals: 1
Atry
changed the title
noNvidiaDriverAssertion optionsuppressNvidiaDriverAssertion option
wegank
removed
the
12.approvals: 1
The assertion to ensure the Nvidia driver does not make sense on WSL, where the Nvidia driver is mounted by Windows. See https://github.com/nix-community/NixOS-WSL/blob/a6b9cf0b7805e2c50829020a73e7bde683fd36dd/modules/wsl-distro.nix#L83-L105
This PR adds an option to suppress the assertion so that
nvidia-container-toolkitworks on WSL.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.