Conversation
|
follow-up to #407629 |
|
Hmm, the darwin ci really does not like this one... |
LordGrimmauld
force-pushed
the
udev-check-platform
branch
from
4f339bd to
8638c02
Compare
|
(thanks to emily and Alyssa for sanity-checking this over on matrix) |
github-actions
bot
added
10.rebuild-darwin: 1
|
Right, thanks for catching this! Let's get this in quickly. I am wondering if there is a nicer way that avoids running a loop and invoking I definitely prefer this over needing conditionals at every usage site. |
LordGrimmauld
force-pushed
the
udev-check-platform
branch
from
8638c02 to
45322f0
Compare
|
Alright, some tweaking. |
LordGrimmauld
force-pushed
the
udev-check-platform
branch
from
45322f0 to
5bcd5d7
Compare
|
(this time i am not forgetting the |
It is technically possible to guard all udevCheckHook usages behind `lib.optionals (lib.meta.availableOn stdenv.buildPlatform systemdMinimal)`. However, doing this is hard to read, clunky, and hard to discover. *Not* doing such a guard would mean cross-compilation darwin -> linux breaks. The workaround here is to just accept any udev rules if they can't be properly checked.
LordGrimmauld
force-pushed
the
udev-check-platform
branch
from
5bcd5d7 to
1461a84
Compare
|
Okay, now i think i am happy with this. |
github-actions
bot
added
10.rebuild-linux: 1
|
We probably also should backport the fix |
|
Successfully created backport PR for |
nixpkgs-ci
bot
added
the
8.has: port to stable
| # 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 |
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
This would only be a concern for Linux → non-Linux cross, I think.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I suppose the easiest test would be Linux → BSD variant. I'll see if there is a setup that has a cached cross stdenv.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
in that case that condition should then be dropped completely, only leaving the buildPlatform check to test whether the hook can actually run
There was a problem hiding this comment.
Emily has a point, i opened #412708 to just remove this check.
4 participants
It is technically possible to guard all udevCheckHook usages behind
lib.optionals (stdenv.hostPlatform.isLinux && lib.meta.availableOn stdenv.buildPlatform systemdMinimal).However, doing this is hard to read, clunky, and hard to discover. Not doing such a guard would mean cross-compilation darwin -> linux breaks (or darwin itself breaks, without cross). The workaround here is to just accept any udev rules if they can't be properly checked.
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.