Skip to content

nixos/udev: verify udev rules using udevadm#404323

Merged
leona-ya merged 1 commit intoNixOS:masterfrom
r-vdp:rvdp/verify_udev_rules
May 9, 2025
Merged

nixos/udev: verify udev rules using udevadm#404323
leona-ya merged 1 commit intoNixOS:masterfrom
r-vdp:rvdp/verify_udev_rules

Conversation

@r-vdp
Copy link
Contributor

@r-vdp r-vdp commented May 5, 2025 •
edited
Loading

Things done

I have splitted the fixes for different packages out into different PRs that should be merged first:

  • 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.

@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/` 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. labels May 5, 2025
@r-vdp r-vdp force-pushed the rvdp/verify_udev_rules branch 2 times, most recently from 477b79b to bd63d84 Compare May 5, 2025 10:09
SuperSandro2000
SuperSandro2000 reviewed May 5, 2025
View reviewed changes
@r-vdp r-vdp force-pushed the rvdp/verify_udev_rules branch from bd63d84 to f5c395e Compare May 5, 2025 12:08
misuzu
misuzu reviewed May 5, 2025
View reviewed changes
@r-vdp r-vdp marked this pull request as ready for review May 5, 2025 14:31
@r-vdp r-vdp closed this May 5, 2025
@r-vdp r-vdp force-pushed the rvdp/verify_udev_rules branch from 517e7c9 to 29daeb6 Compare May 5, 2025 14:39
@nix-owners nix-owners bot requested review from MarcWeber, jtojnar and peterhoeg May 5, 2025 14:40
@r-vdp r-vdp reopened this May 5, 2025
@r-vdp
Copy link
Contributor Author

r-vdp commented May 5, 2025

I messed up with extracting the other fixes, sorry for the pings.

@github-actions github-actions bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. and removed 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. labels May 5, 2025
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label May 5, 2025
LordGrimmauld
LordGrimmauld reviewed May 8, 2025
View reviewed changes
@r-vdp r-vdp force-pushed the rvdp/verify_udev_rules branch 2 times, most recently from 2fc7f5f to ff89fcf Compare May 8, 2025 14:44
@LordGrimmauld
Copy link
Contributor

LordGrimmauld commented May 8, 2025 •
edited
Loading

Github is now returning a 500 when trying to submit any review...

Anyways: I'd love to approve once github cooperates.

Message i was about to submit:
Thanks! I successfully built my system configuration against this PR.
We want to check correctness in our builds, not stylistic choices of upstream packages. As such, i feel this is much more appropriate now. I also would feel confident with a merge: Any hard failure caused by this now will point to an actual issue with udev rules (which probably didn't behave correctly to start with).

@r-vdp
Copy link
Contributor Author

r-vdp commented May 8, 2025

I wonder if we should wait until after branch off though, since this is a potentially breaking change.
@NixOS/nixos-release-managers ?

@r-vdp r-vdp force-pushed the rvdp/verify_udev_rules branch from ff89fcf to aa7b078 Compare May 8, 2025 18:33
@r-vdp r-vdp force-pushed the rvdp/verify_udev_rules branch from aa7b078 to 1278d56 Compare May 8, 2025 18:34
RossComputerGuy
RossComputerGuy approved these changes May 8, 2025
View reviewed changes
Copy link
Member

@RossComputerGuy RossComputerGuy left a comment

Choose a reason for hiding this comment

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

LGTM, let's see what @leona-ya thinks before merging.

@wegank wegank added 12.approvals: 3+ This PR was reviewed and approved by three or more persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels May 8, 2025
@leona-ya leona-ya merged commit 18ef621 into NixOS:master May 9, 2025
28 checks passed
@r-vdp r-vdp deleted the rvdp/verify_udev_rules branch May 9, 2025 11:17
@spiage spiage mentioned this pull request May 9, 2025
Closed
3 tasks
@K900 K900 mentioned this pull request May 9, 2025
Merged
@r-vdp r-vdp mentioned this pull request May 11, 2025
Merged
13 tasks
@yshui
Copy link
Contributor

yshui commented May 16, 2025

this verification needs to be done at package level, for any packages that ship udev rules. so failures can be caught early.

otherwise we will see packages building fine, but when someone tries to use it, it blows up in udev-rules.

@LordGrimmauld
Copy link
Contributor

LordGrimmauld commented May 16, 2025

I'll have a poke writing udev-check-hook that checks /etc/udev outputs in a postInstall

@LordGrimmauld LordGrimmauld mentioned this pull request May 16, 2025
Merged
13 tasks
@LordGrimmauld
Copy link
Contributor

LordGrimmauld commented May 16, 2025

Opened #407629

LordGrimmauld added a commit to LordGrimmauld/nixpkgs that referenced this pull request May 16, 2025
@LordGrimmauld
udevCheckHook: init
b3bdbf4 
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.
nixpkgs-ci bot pushed a commit that referenced this pull request May 21, 2025
@LordGrimmauld @github-actions
udevCheckHook: init
e01dd0d 
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 #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.

(cherry picked from commit b3bdbf4)
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented May 21, 2025

Got reverted in #405717 but we should definitely bring it back.

@LordGrimmauld
Copy link
Contributor

LordGrimmauld commented May 21, 2025

Got reverted in #405717 but we should definitely bring it back.

Was readded in #406284

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@misuzu misuzu misuzu approved these changes

@leona-ya leona-ya leona-ya approved these changes

@RossComputerGuy RossComputerGuy RossComputerGuy approved these changes

@LordGrimmauld LordGrimmauld LordGrimmauld approved these changes

@MarcWeber MarcWeber Awaiting requested review from MarcWeber

@peterhoeg peterhoeg Awaiting requested review from peterhoeg

@jtojnar jtojnar Awaiting requested review from jtojnar

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: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 3+ This PR was reviewed and approved by three or more persons.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@r-vdp @LordGrimmauld @yshui @SuperSandro2000 @misuzu @leona-ya @RossComputerGuy @wegank