Conversation
|
cc @yshui, can't review-request you as you are not on the maintainers list. |
|
Adding this hook to |
github-actions
bot
added
10.rebuild-darwin: 0
|
I like the idea, but it would be great if we could somehow include this by default. The issue probably being that we can only include this for packages that aren't in the dependency closure of systemdMinimal. I don't know if there is any way to do that (how do we deal with other hooks that are enabled by default? They will at least depend on some things like bash, coreutils, etc). |
There was a problem hiding this comment.
The files in packages typically live in libdir: https://www.freedesktop.org/software/systemd/man/latest/udev.html#Rules%20Files
There was a problem hiding this comment.
Good catch! But for nix, both is actually valid:
nixpkgs/nixos/modules/services/hardware/udev.nix
Lines 230 to 236 in 5d73626
There was a problem hiding this comment.
Actually, we could maybe even simplify this:
Seems like udev outputs are only in the $bin output, tho i have to figure out how to get only the bin output.
There was a problem hiding this comment.
You can use indirect variable access: ${!outputBin}
There was a problem hiding this comment.
done. Should be good now!
There was a problem hiding this comment.
Though unless this is ensured on package basis (e.g. in
), you probably still want to check all outputs.For example, if the package has out and lib outputs, and the project uses libdir variable to install the files, they will end up in lib output while outputBin will point to out.
Though such packages need to be passed to services.udev.packages with explicit output for lib.getOutput "bin" to find them.
There was a problem hiding this comment.
Grepping, I see at least one package that has the files in non-bin output:
There was a problem hiding this comment.
Actually, I am not even sure why the module is looking to bin output in the first place. I would expect it in out output if:
If package has daemon and bindir programs in separate outputs, they should be in out and bin respectively, and in that case I would expect the udev files in out output.
Edit: Looks like it has been discussed on the commit that introduced the getBin cd5dd9f and a PR was opened but it fizzled out.
There was a problem hiding this comment.
alright, i'll restore the for loop for all outputs!
There was a problem hiding this comment.
(and the module might need a fix later, i too was surprised it looks in $bin first)
I'd agree, but i honestly have no idea how to make that happen either. The doc on check hooks is kinda lacking. |
|
Yeah. But these hooks are also really not very discoverable, so I doubt that many packages will use this. We can of course add it incrementally to packages that caused nixos builds to fail, to avoid regressions, that's very valuable already (but still needs a way to point people to this option). |
How about a follow-up treewide that adds the hook to all packages with udev output? |
LordGrimmauld
force-pushed
the
udev-check-hook
branch
2 times, most recently
from
8964a2d to
267649e
Compare
drupol
left a comment
drupol
left a comment
There was a problem hiding this comment.
Here's the shellcheck output:
❯ shellcheck pkgs/by-name/ud/udevCheckHook/hook.sh
In pkgs/by-name/ud/udevCheckHook/hook.sh line 1:
udevCheckHook() {
^-- SC2148 (error): Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
In pkgs/by-name/ud/udevCheckHook/hook.sh line 9:
if [ -d "${!outputBin}/$path/udev/rules.d" ]; then
^-----------^ SC2154 (warning): outputBin is referenced but not assigned.
For more information:
https://www.shellcheck.net/wiki/SC2148 -- Tips depend on target shell and y...
https://www.shellcheck.net/wiki/SC2154 -- outputBin is referenced but not a...
Perhaps we could ignore1 those two issues with a comment?
Footnotes
LordGrimmauld
force-pushed
the
udev-check-hook
branch
from
267649e to
2a91312
Compare
Usage: ```nix nativeBuildInputs = [ udevCheckHook ]; doInstallCheck = true; ``` This hook executes `udevadm verify --resolve-names=never --no-style` on all outputs that have `/etc/udev/rules.d`. This us a logical part of NixOS#404323 to check packages that supply udev rules. Note this hook introduces a dependency on `systemdMinimal`, meaning this can't check systemdMinimal or its dependencies.
LordGrimmauld
force-pushed
the
udev-check-hook
branch
from
2a91312 to
b3bdbf4
Compare
Yeah we could do a one-off effort like that, but that won't ensure that future packages will have the hook added. But ok, perfect is the enemy of good, these are not arguments against this PR, just reflections triggered by it. |
|
thank you for working on this! ❤️
would it be possible to make it a CI check? i.e. if derivation outputs contain udev rules, check if the check hook is in build inputs? |
not really i think. |
|
What is missing here to get this moved forward / merged? |
|
As to packages requiring the hook: nix-locate -t d --at-root -w --minimal -r '/(etc|lib)/udev/rules.d'The following packages would be affected: |
|
Successfully created backport PR for |
nixpkgs-ci
bot
added
the
8.has: port to stable
|
Uh, hold on - one thing that came to mind. Wouldn't we want to gate udev checking behind |
|
(currently drafting with optionals in the packages using the hook) |
|
Is there a way to easily inject this into many packages? |
Not yet, working on a treewide to add this to all packages providing udev rules. But i got distracted by the platform guarding issue, #409385 is basically a dependency to that treewide. |
|
No hurry, I was just curious if there is a plan :) |
|
WIP state is in #409564, finishing that will take a bit. |
|
The treewide (#409564) is now ready to review. I got most packages for which it is viable to enable the hook. |
6 participants
Usage:
This hook executes
udevadm verify --resolve-names=never --no-styleon all outputs that have/etc/udev/rules.d.This us a logical part of #404323 to check packages that supply udev rules.
Note this hook introduces a dependency on
systemdMinimal, meaning this can't check systemdMinimal or its dependencies.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.