Make fetchFromGitHub & friends overridable

[piegamesde]

Motivation for this change

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• 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/)
• 22.05 Release Notes (or backporting 21.11 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
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[mweinelt]

That would be very neat to have. I'm not aware of the downsides so I'm requesting some people to review this instead of me.

[roberth]

This was also discussed before #158018 was updated to the change that it is now. The performance cost can be estimated as around 0.5%. I think that's not a lot, but it is something we won't get back.
It's based on the eval performance report from the commit statuses, ignoring time (noisy), looking at gc numbers, which are a reasonable proxy for time.

[RaitoBezarius]

Are we still interested into having this?

[piegamesde]

Yes but I won't be the one fighting for it

[RaitoBezarius]

I believe that having this around, even if it does cost +0,5 % in heap size, will save more time for users.

[Mindavi]

What would be a usecase for this?

[RaitoBezarius]

What would be a usecase for this?

Doing: pkgs.x.overrideAttrs (old: { src = old.src.override { sha256 = "xxx"; } }) rather than rebuilding the whole fetchFromX signature.

[mweinelt]

Overriding the rev/hash, but keeping the owner/repo.

[ncfavier]

I don't have very strong opinions on this, although I feel like the intended use case overlaps with the reason #119942 was introduced.

---

I do have comments on implementation though: the makeOverridable calls should be added to pkgs/build-support/foo rather than all-packages.nix. This has two advantages:

• we get to keep fetchFoo.override (which is different from the added (fetchFoo { ... }).override)
• putting less logic in all-packages.nix is always good

For example, fetchgithub/default.nix would look like this:

{ lib, fetchgit, fetchzip }:

lib.makeOverridable (
/* rest of the file */
)

[infinisil]

I'd also like to see some tests for this, to make sure both .override and .overrideAttrs work for both the fetcher itself and the result.

[ncfavier]

(The fetcher itself wouldn't have an overrideAttrs since it's not a derivation.)

[Atemu]

I've just bumped into something where I could use this.

If you yourself have a derivation where you want to make the source overridable, a workaround is to use src = (lib.makeOverridable fetchgit) { ... };.

[piegamesde]

The interactions with #119942 are certainly interesting, although I don't think this PR is completely obsoleted by it. Take the common use case where I want to override version + source hash.

• Formerly: You need to override the version field and the complete source attribute .overrideAttrs (old: rec { version = "42"; src = fetchFromGitHub { inherit version; owner = …; repo = …; hash = …; }; })
• With stdenv.mkDerivation: overlay style overridable recursive attributes #119942: Overriding the version will also override it in the source fetcher, but this does not help you because you still have to update the hash. So same as above
• With this PR: you can override only the source hash and version, but still need to do the version twice .overrideAttrs (old: rec { version = "42"; src = old.src.override { inherit version; hash = …; }; })
• With both: the following should work .overrideAttrs (old: { version = "42"; src = old.src.override { hash = …; }; })

[piegamesde]

I sure hope "ofborg-eval" does a full evaluation, because I have no way of testing all those fetchers.

@ncfavier I updated the design according to your review

[pschyska]

I think this breaks msteen's nix-prefetch.

nix-prefetch fetchFromGitHub --owner hrsh7th --repo nvim-cmp --rev 768548bf4980fd6d635193a9320545bb46fcb6d8
error: anonymous function at /nix/store/pzhdk2xd7k1j1sd2c8fqxgi35w5i9zrl-source/pkgs/build-support/fetchgithub/default.nix:4:1 called without required argument 'repo'

(I've seen equivalent issues with fetchgit instead of fetchFromGitHub)

Pinning nixpkgs in NIX_PATH to 60d3ce9 (this commit's parent) resolve the issue:

export NIX_PATH='nixpkgs=/nix/store/ggpfzfa3v97fnxi734c2160qignm361m-source'

Because https://github.com/berberman/nvfetcher uses this nix-prefetch, it is currently broken (see berberman/nvfetcher#96)

(Edit: parent commit hash)

[piegamesde]

@pschyska I unfortunately don't know much about nix-prefetch and how it works, and I have trouble associating these error messages with an explanation how this patch may have caused them.

I think there already existed a couple of overridable fetchers before this patch, although I have forgotten which ones. You could try what happens with these, as a control.

[ncfavier]

nix-prefetch seems to rely on lib.functionArgs, and this PR makes lib.functionArgs pkgs.fetchFromGitHub empty. Not sure why yet.

EDIT: simply because makeOverridable f doesn't inherit f's functionArgs (it only passes them along to the override attribute). Do we want to change that?

[mweinelt]

I think this breaks msteen's nix-prefetch.

Aside: Last updated in 2021. Time to migrate to nurl.

[pschyska]

I think this breaks msteen's nix-prefetch.

Aside: Last updated in 2021. Time to migrate to nurl.

I would happily migrate, if I could.

[mweinelt]

I think this breaks msteen's nix-prefetch.

Aside: Last updated in 2021. Time to migrate to nurl.

I would happily migrate, if I could.

What prevents you? https://github.com/nix-community/nurl#supported-fetchers

[fbewivpjsbsby]

I think this breaks msteen's nix-prefetch.

Aside: Last updated in 2021. Time to migrate to nurl.

I would happily migrate, if I could.

What prevents you? https://github.com/nix-community/nurl#supported-fetchers

nurl not support fetchurl now.

[pschyska]

I think this breaks msteen's nix-prefetch.

Aside: Last updated in 2021. Time to migrate to nurl.

I would happily migrate, if I could.

What prevents you? https://github.com/nix-community/nurl#supported-fetchers

As I wrote, I'm a nvfetcher user, not a direct nix-prefetch user. It's up to @berberman to make a decision to migrate. I think they are investigating it though.

[piegamesde]

As per @ncfavier's comment this looks fixable, so probably no need to migrate

[ncfavier]

Looks like there's prior art #134272

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/overrideattrs-not-taking-effect-how-to-override-fetchfromgithub-and-more/32298/1