fetchurl - add wrapper with output derivation overridable

[virusdave]

Make the output of fetchurl overridable.

Currently, the fetchurl function itself is overridable, but not the output derivation. This change fixes that, so the output derivation itself is also overridable.

Motivating example:
Oftentimes a package uses fetchurl to set the value for the derivation's src attribute. Sometimes the url parameter to fetchurl is computed. As an example, the tk package accepts tcl as a parameter for package dependency, and computes its own url from the version attribute defined in the tcl package. If one passes in a different version of tcl to tk via something like

my_tk = pkgs.tk.override { tcl = my_tcl-8_6; };

it would be very convenient to just update the sha256 for tk's sources without having to muck around with recreating the computed URL for the sources. With this change, it's pretty easy:

my_tk = (pkgs.tk.override { tcl = my_tcl-8_6; }).overrideAttrs (oldAttrs: {
  src = oldAttrs.src.override { sha256 = "<something>"; };
});

Without this change, one has to manually construct the entire fetchurl args, which is much uglier.

Alternatively, this makes it possible to easily substitute an internal mirror URL for a particular package.

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.

[Lassulus]

hmm, ofBorg has some problems. I would guess there is some issue with this in nixpkgs which should be fixed before merging

[virusdave]

hmm, ofBorg has some problems. I would guess there is some issue with this in nixpkgs which should be fixed before merging

Looks like it ran out of memory during execution (exit code 137). I doubt this is something I can configure. How should I proceed @Lassulus ?

[Lassulus]

can you update the usage in your first comment?

[jtojnar]

Most of the time user would want to do something like

hello = (prev.hello.overrideAttrs (oldAttrs: {
  version = "1.2.3";
  src = oldAttrs.src.override {
    sha256 = "<something>";
  };
});

which will not work unless the package also uses #119942 (if this change is compatible with that) or the user also updates url. So I would prefer if the example showed that use case to reduce potential confusion.

I am also curious if this will have a performance impact. Edit: should be fine now that it is opt-in.

cc @roberth @edolstra

[lilyball]

Given the new version, having to write

{
  tk869 = (pkgs.tk-8_6.override {
    fetchurl = fetchurlOverridableOutput;
    tcl = tcl869; # Something overriding `tcl-8_6 to an older version
  }).overrideAttrs (self: rec {
    src = self.src.override { sha256 = "1d7bfkxpacy33w5nahf73lkwxqpff44w1jplg7i2gmwgiaawvjwg"; };
  });
}

is somewhat awkward, and it doesn't really look any better than

{
  tk869 = (pkgs.tk-8_6.override {
    fetchurl = args: lib.makeOverridable fetchurl args;
    tcl = tcl869; # Something overriding `tcl-8_6 to an older version
  }).overrideAttrs (self: rec {
    src = self.src.override { sha256 = "1d7bfkxpacy33w5nahf73lkwxqpff44w1jplg7i2gmwgiaawvjwg"; };
  });
}

And in particular, this new approach doesn't scale once you start looking at other fetchers, unless we want to have a separate copy of every single fetcher that is named e.g. fetchFromGitHubOverridableOutput.

If we can't just make the fetcher overridable by default, then I would rather see us simply document this override trick in the manual. It could also be documented in comments in pkgs/build-support/fetchurl/default.nix so anyone who writes nix edit nixpkgs#fetchurl can stumble across it.

[roberth]

makeOverridable by default would be the nicest solution. You can measure the cost of your original makeOverridable-by-default solution by changing the PR back to that and opening "Evaluation Performance Report" in the commit statuses. It's available after ofborg eval completes. That will give you a table with stats. The cpu time is too noisy, but the memory ones are good. gc-totalBytes is a reasonable proxy for time. gc-heapSize is the final heap size, but with a lot of rounding. I don't recall seeing an increment of less than 16MB or 0.07%.

If the impact is not acceptable, I agree we should document the current solution. @lilyball's example can be eta-reduced and the unused rec must be removed to avoid confusion. (In fact rec should be removed from the language, subjectively)

{
  tk869 = (pkgs.tk-8_6.override {
    fetchurl = lib.makeOverridable fetchurl;
  }).overrideAttrs (self: {
    src = self.src.override { sha256 = "1d7bfkxpacy33w5nahf73lkwxqpff44w1jplg7i2gmwgiaawvjwg"; };
  });
}

Furthermore, it may be possible to write a helper function that takes care of the general pattern, detecting the fetchers used reflectively, but that may be an abstraction that is too fragile; not sure.

[virusdave]

makeOverridable by default would be the nicest solution. You can measure the cost of your original makeOverridable-by-default solution by changing the PR back to that and opening "Evaluation Performance Report" in the commit statuses. It's available after ofborg eval completes. [...]

Yeah, that's the first thing i did, but per @grahamc the perf impact was... substantial. I'd much prefer making it overridable by default if anyone has any clever ideas how to do so without causing massive perf degradation (and i'll even try to implement the ideas if so), but honestly i am not familiar enough with nix internals and perf consequences to have a good idea how to proceed on my own.

I also agree that the current state (having a named wrapper) really doesn't buy us all that much compared to just manually wrapping the fetcher during override where desired. I actually realized this about 2 minutes after pushing the updated PR, but figured I'd wait to see if people felt similarly or if they found it useful. I guess that's answered :)

If no one has good ideas on how to fix the perf problems in the original, i think i'll just drop this named-wrapper PR instead.

[roberth]

Cost is around 0.5%; see #158968

[piegamesde]

I stongly agree with @lilyball here: The current opt-in solution is a non-starter UX wise.

which will not work unless the package also uses #119942 (if this change is compatible with that) or the user also updates url

@jtojnar Can you please explain why it wouldn't work as-is?

---

I don't understand where the performance regression comes from, since I assume it would only cost us where we actually use override? If makeOverridable has a fixed cost to all calls, then could we reduce it by removing the makeOverridable from the callPackage?

[jtojnar]

which will not work unless the package also uses #119942 (if this change is compatible with that) or the user also updates url

@jtojnar Can you please explain why it wouldn't work as-is?

url would still contain reference to the old version.

If makeOverridable has a fixed cost to all calls, then could we reduce it by removing the makeOverridable from the callPackage?

That would be a very bad BC break, we widely rely on being able to override callPackage arguments in Nixpkgs and recommend it to users liberally so presumably it is also very common outside of Nixpkgs.

[piegamesde]

That would be a very bad BC break, we widely rely on being able to override callPackage arguments in Nixpkgs and recommend it to users liberally so presumably it is also very common outside of Nixpkgs.

It wouldn't be any more breaking than the originally proposed solution, AFAICT.

[lilyball]

It wouldn't be any more breaking than the originally proposed solution, AFAICT.

The originally proposed solution adds an override attribute to the fetcher results, but this attribute does not currently exist, and so the addition of this attribute should not break anything.

I don't understand where the performance regression comes from, since I assume it would only cost us where we actually use override?

From a memory standpoint, making the fetcher overridable means persisting the original arugments to the fetcher so they can't be GC'd. I don't know how much of a difference that should make under normal use though.

@roberth #158968 was measuring the cost of making fetchFromGitHub overridable. This presumably has a lower impact than making fetchurl itself overridable as fetchurl is used for more than just fetchFromGitHub, though I still wouldn't expect it to be awful.

[xaverdh]

{
  tk869 = (pkgs.tk-8_6.override {
    fetchurl = lib.makeOverridable fetchurl;
  }).overrideAttrs (self: {
    src = self.src.override { sha256 = "1d7bfkxpacy33w5nahf73lkwxqpff44w1jplg7i2gmwgiaawvjwg"; };
  });
}

can we document this workaround in the manual?

While long term, we want a more declarative structure for packages anyway, this makes for an almost sane UX, and we can have it right now (at least as far as packages use the overlay style recursion).

I guess we could also add a convenience function along the lines of overrideSrc for this, but maybe that is too much magic.