Skip to content

udevCheckHook: guard platform #409385

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.

Sign up for GitHub

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

Merged
r-vdp merged 1 commit into from
May 21, 2025
Merged

udevCheckHook: guard platform #409385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkgs/by-name/ud/udevCheckHook/hook.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ udevCheckHook() {
echo Finished udevCheckPhase
}

if [[ -z "${dontUdevCheck-}" ]]; then
if [[ -z "${dontUdevCheck-}" && -n "@udevadm@" ]]; then
echo "Using udevCheckHook"
preInstallCheckHooks+=(udevCheckHook)
fi
12 changes: 10 additions & 2 deletions pkgs/by-name/ud/udevCheckHook/package.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,20 @@
lib,
makeSetupHook,
systemdMinimal,
udev,
stdenv,
}:

let
# udev rules can only be checked if systemd (specifically, 'udevadm') can be executed on build platform
# if udev is not available on hostPlatform, there is no point in checking rules
applyHook =
lib.meta.availableOn stdenv.hostPlatform udev
Copy link
Member

@FliegendeWurst FliegendeWurst May 31, 2025

Choose a reason for hiding this comment

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

If I add udevCheckHook to nativeInstallCheckInputs, wouldn't this hostPlatform be the build platform? So this would really need to check the targetPlatform? (But I'm not certain how the splicing logic works in this case...)

Copy link
Member

@FliegendeWurst FliegendeWurst May 31, 2025

Choose a reason for hiding this comment

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

This would only be a concern for Linux → non-Linux cross, I think.

Copy link
Contributor Author

@LordGrimmauld LordGrimmauld May 31, 2025

Choose a reason for hiding this comment

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

lib.meta.availableOn stdenv.hostPlatform udev won't actually cause build failures if it doesn't check correctly. The only thing that is actually required for the hook to run is systemd being executable on buildPlatform.

Your concern is valid though. However:

nix-repl> pkgsCross.x86_64-darwin.stdenv.hostPlatform.config
"x86_64-apple-darwin"

nix-repl> lib.meta.availableOn pkgsCross.x86_64-darwin.stdenv.hostPlatform udev
false

nix-repl>

(executed on a x86-64-linux machine)
This has me believe the check does actually work correctly.

Copy link
Member

@FliegendeWurst FliegendeWurst May 31, 2025

Choose a reason for hiding this comment

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

That is not the stdenv this package would receive, thanks to splicing.
Since it is a native input, it would be built using

buildPlatform = builder
hostPlatform = builder
targetPlatform = host

(relative to the package using the hook as a native input)

Copy link
Member

@FliegendeWurst FliegendeWurst May 31, 2025

Choose a reason for hiding this comment

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

I suppose the easiest test would be Linux → BSD variant. I'll see if there is a setup that has a cached cross stdenv.

Copy link
Member

@emilazy emilazy May 31, 2025

Choose a reason for hiding this comment

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

I see. But I don’t understand what the scripting thing has to do with the targetPlatform?

We should avoid using targetPlatform whenever we can.

Copy link
Member

@FliegendeWurst FliegendeWurst May 31, 2025

Choose a reason for hiding this comment

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

The only reason we want targetPlatform here is because the hook is added to nativeInstallCheckInputs in a package X, but we would like to know whether udev is available on the hostPlatform for X.

Copy link
Member

@emilazy emilazy May 31, 2025

Choose a reason for hiding this comment

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

Yes, my point is that I don’t see why we care about udev being available on X’s host platform. There’s no harm in checking the files even if they won’t be used. If they’re included at all, they should be correct; if they shouldn’t be included because they won’t be used, then that’s a separate issue.

Copy link
Contributor Author

@LordGrimmauld LordGrimmauld May 31, 2025

Choose a reason for hiding this comment

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

in that case that condition should then be dropped completely, only leaving the buildPlatform check to test whether the hook can actually run

Copy link
Contributor Author

@LordGrimmauld LordGrimmauld May 31, 2025

Choose a reason for hiding this comment

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

Emily has a point, i opened #412708 to just remove this check.

&& lib.meta.availableOn stdenv.buildPlatform systemdMinimal;
in
makeSetupHook {
name = "udev-check-hook";
substitutions = {
udevadm = lib.getExe' systemdMinimal "udevadm";
udevadm = if applyHook then lib.getExe' systemdMinimal "udevadm" else "";
};
meta = {
description = "check validity of udev rules in outputs";
Expand Down