-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
trivial: make symlinkJoin support pname+version alone #344645
base: master
Are you sure you want to change the base?
Conversation
2a88cec
to
cb93e64
Compare
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.
LGTM but I'd wait for another review.
cb93e64
to
290677b
Compare
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.
Yeah, this is fine. We could finesse it a little:
- Better error message if nothing is specified.
- Better error message if name AND pname || version is specified
- Better error message if pname but not version is specified and vice versa
- Don't pass through the pname or version into runCommand's derivationArgs (name already omitted)
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.
+1 on the errors mention by Philip, but the removal of pname
and version
here is a requirement IMO
290677b
to
169d2c6
Compare
Done
This is not an error in mkDerivation
assert lib.assertMsg (args_ ? pname && args_ ? version)
"symlinkJoin requires either a `name` OR `pname` and `version`, found: ${lib.concatStringsSep ", " (lib.filter (lib.flip builtins.elem ["name" "pname" "version"]) (lib.attrNames args_))}";
"${args_.pname}-${args_.version}"
That would mean not setting |
This confuses me, is there a negation missing or is there something i'm missing? |
Here's my attempt, designed to be placed after in the
|
No, I think I'm the source of the issue: I didn't fully understand that getting |
Co-authored-by: Philip Taron <[email protected]>
It does seem to achieve the intended hardening (2 and 3), but the breakage is massive and I assume the eval times will increase, especially in nixos. |
Drafted until the way forward is decided. Do we (1) forge ahead and establish this new hardening convention and patch all violations, or (2) drop 42ab48f ? |
Note the performance result: https://github.com/NixOS/nixpkgs/pull/344645/checks?check_run_id=30790294683 For me, given that there's any eval troubles, I'm more on the drop-and-defer-to-a-future-PR train -- a PR that means to tackle this relatively common idiom of name vs. pname/version, and that makes a stand about how it ought to be represented in nixpkgs. |
Description of changes
inspired by Mic92/nix-update#284
should be 0 rebuilds
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.