-
-
Notifications
You must be signed in to change notification settings - Fork 14.9k
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.
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
nixos-build-vm: Remove Nixpkgs path dependency #372492
nixos-build-vm: Remove Nixpkgs path dependency #372492
Conversation
4ce241c switched `substituteAll` for `replaceVars`. The latter stringifies the replacements, which leads to Nixpkgs filesystem path being embedded in the output. This is disallowed by nixpkgs-basic-release-checks.nix. NixOS#371501 (comment)
replacements = { | ||
inherit runtimeShell; | ||
buildVms = ./build-vms.nix; | ||
buildVms = "${./build-vms.nix}"; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether we should fix replaceVars to do that automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am split on that, hence just the quick workaround. Especially when it did not look like any other replacement in your PR used paths.
It might be useful to have real file system paths sometimes. Though, I guess one can always use toString
in the caller, and derivation
does copy paths passed to it to the store, so it might be better to do the same for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially when it did not look like any other replacement in your PR used paths.
Yes, but I have a few 100 more replacements coming up eventually - the goal is to replace substituteAll entirely. So there might be more paths coming up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally did toString
on the inputs to replaceVars
that and was counseled against in in the original replaceVars
PR. I'm fine with this quick workaround; let's see if there are other instances like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick grep only came up with one other potential candidate:
nixpkgs/pkgs/development/tools/build-managers/gradle/default.nix
Lines 214 to 218 in 3f14b85
(substituteAll { | |
src = ./setup-hook.sh; | |
# jdk used for keytool | |
inherit (gradle) jdk; | |
init_script = ./init-build.gradle; |
4ce241c switched
substituteAll
forreplaceVars
.The latter stringifies the replacements, which leads to Nixpkgs filesystem path being embedded in the output.
This is disallowed by nixpkgs-basic-release-checks.nix.
#371501 (comment)
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-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.