trivial: make symlinkJoin support pname+version alone

[pbsds]

Description of changes

inspired by Mic92/nix-update#284

should be 0 rebuilds

Things done

• 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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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/)
• 24.11 Release Notes (or backporting 23.11 and 24.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.

[drupol]

LGTM but I'd wait for another review.

[philiptaron]

Yeah, this is fine. We could finesse it a little:

1. Better error message if nothing is specified.
2. Better error message if name AND pname || version is specified
3. Better error message if pname but not version is specified and vice versa
4. Don't pass through the pname or version into runCommand's derivationArgs (name already omitted)

[getchoo]

+1 on the errors mention by Philip, but the removal of pname and version here is a requirement IMO

[pbsds]

1. Better error message if nothing is specified.

Done

2. Better error message if name AND pname || version is specified

This is not an error in mkDerivation

3. Better error message if pname but not version is specified and vice versa

1 already partially covers this. Handling this request in full is possible, but I'm not sure if I like it:

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}"

4. Don't pass through the pname or version into runCommand's derivationArgs (name already omitted)

That would mean not setting finalPackage.pname and finalPackage.version, which is the current behaviour and the entire reason why you want to provide these args

[pbsds]

+1 on the errors mention by Philip, but the removal of pname and version here is a requirement IMO

This confuses me, is there a negation missing or is there something i'm missing?

[philiptaron]

Handling this request in full is possible, but I'm not sure if I like it.

Here's my attempt, designed to be placed after in the let block.

assert lib.assertMsg (args_ ? name -> !(args ? pname || args ? version) "symlinkJoin requires either `name` or `pname` and `version`; to fix this, remove `name` or both of `pname` and `version`"
assert lib.assertMsg (args_ ? pname == args_ ? version) "symlinkJoin requires `pname` and `version` to be specified together";

[philiptaron]

This confuses me, is there a negation missing or is there something I'm missing?

No, I think I'm the source of the issue: I didn't fully understand that getting pname and especially version into the resulting derivationArgs was a key goal of this patch.

[pbsds]

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.

[pbsds]

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 ?

[philiptaron]

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.

[philiptaron]

I'm still on the drop-and-defer-to-a-future-PR train. 🚋

[pbsds]

oh wow it has been two weeks. I was supposed to revert the commit but forgot

[pbsds]

pr is stuck at "processing" after I pushed, might be the current outage https://www.githubstatus.com/incidents/myvz2tsj2dh8

[philiptaron]

I don't think the relevant commit was dropped.

[pbsds]

[philiptaron]

Dang, I think my eyes have never processed that section of the header before. 👀 Thanks for entertaining my blindness. 😁