fetchFromGitHub: use passthru instead of // to expose rev and friends; fetchgit: support adding extra passthru values

[TomaSajt]

Closes #370418

Other than the main change about passing some values with passthru instead of // there are some minor extra changes which should not cause any issues:

• Rename passthruAttrs to extraFetcherAttrs to not have two meanings of passthru in the same file.
• inherit name is now a bit earlier in the construction of the combined attrset.
  Since we don't filter name out from @attrs the // extraFetcherAttrs will automatically include the name value if it is passed to the function.
  This means the only thing inherit name; does is that it sets the default value.
  I could replace inherit name; with name = "source"; and everything would be the same.
  • Should I do this? Then we could remove the name ? "source" from the function args

Note: I did not update fetchFromGitLab and other fetchers.

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/)
• 25.05 Release Notes (or backporting 24.11 and 25.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.

[philiptaron]

I really like this direction, and I'd love to see the other fetchers brought into line. I have a few questions about order of application and expectations from these derivation factory functions. They're more curious than hard opinions.

[philiptaron]

I really like how this allows the caller to provide values for passthru. I don't like how the caller can override gitRepoUrl and tag, since I can't really come up with good usecases for those that aren't just the user shooting themselves in the foot. What do you think?

The other way would let the user provide whatever, but would assure users that gitRepoUrl and tag would be canonical from this function... so this:

passthru = passthru // {
  gitRepoUrl = url;
  inherit tag;
};

I could also see rev (aka revWithTag) being on this list for similar reasons.

[TomaSajt]

My personal opinion is this: if the consumer sets a value, it should be set or respected in some way (e.g. override default value, append to a default list, etc.). A NOOP is much more surprising than a broken package.

[philiptaron]

Yes, I also don't like NOOP. Maybe I'm trying to construct a static language out of a dynamic one, but I do like this:

nixpkgs/pkgs/build-support/replace-vars/replace-vars-with.nix

Lines 75 to 79 in 5e4cd3d

	optionalAttrs =
	if (builtins.intersectAttrs attrs forcedAttrs == { }) then
	builtins.removeAttrs attrs [ "replacements" ]
	else
	throw "Passing any of ${builtins.concatStringsSep ", " (builtins.attrNames forcedAttrs)} to replaceVarsWith is not supported.";

The idea being that the parameter is there to supplement the set of values in passthru but not override values which are expected to have a definite meaning computed in the function.

[pbsds]

gitRepoUrl is IIRC used in nix-update, but i believe you should be allowed to clobber it if you really want to.

[philiptaron]

Same update ordering question here.

[philiptaron]

And here.

[philiptaron]

Adding @Atemu and @pbsds, both of whom have been in the zone recently, for their ideas and guidance also.

[TomaSajt]

Also note that even if this fixes calling overrideAttrs on derivation, you still won't be able to utilize the fixed point finalAttrs feature, since it doesn't use it internally. You would only be able to change values non-reactively.

---

Sidenote: I really wish the main attrset of stdenv.mkDerivation acted like passthru by default (a.k.a. not evaluated and not passed to the builder as shell variables), as it would make supporting fixed point overrideAttrs stuff much easier, (and maybe even be able to replace .override)

[pbsds]

This does some of what I drafted as patch#2 in Mic92/nix-update#281 and forgot about 👍

One property I'd like for this PR to achieve is for src.passthru to be the same for both forceFetchGit=false; and forceFetchGit=true;

[TomaSajt]

One property I'd like for this PR to achieve is for src.passthru to be the same for both forceFetchGit=false; and forceFetchGit=true;

We could do that, though I'm not 100% it's for the best. passthru is only used here to expose variables through drv.* and not through drv.passthru.*.

---

What's interesting too is the fact that we could use .overrideAttrs { rev = "new-rev"; } on fetchgit-based stuff and it would work, since rev is only used as a bash variable.
However, if we also passed passthru.rev it would still allow the above behaviour, but when getting .rev of the derivation, it would get the passthru.rev value, which is still the old value.

(I'm not sure if you'd ever want to .overrideAttrs the rev value since there's a better .override interface, but still good to keep in mind)

[pbsds]

Ah, i didn't consider that. Your approach is sound!

[SuperSandro2000]

also see #294329