stdenv.mkDerivation: Support argument being a fixed-point function (self: { … }) instead of rec { … }

[lilyball]

Motivation for this change

This came out of https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293/6?u=lilyball as a proposed way to avoid using rec { … } with mkDerivation. The problem with rec { … } is it makes it harder to override, as any reference to an attribute from the set needs to be overridden. In particular, version tends to be re-used, requiring overriding it in multiple places, especially in the URLs given to src fetchers.

The approach taken here is to have mkDerivation detect if it's given a function instead of an attrset, and if it is a function, then it makes that function into a fixed point, calling it with a self value that's the fully-evaluated arguments (not the result of mkDerivation, just the arguments given to it). Switching to this style produces no changes in the resulting derivation, so it causes no rebuilds. But what it does is make overrideAttrs easier. Instead of saying

hello.overrideAttrs (super: {
  version = "2.9";
  src = fetchurl {
    url = "mirror://gnu/hello/hello-2.9.tar.gz";
    sha256 = "19qy37gkasc4csb1d3bdiz9snn8mir2p3aj0jgzmfv0r2hi7mfzc";
  };
  meta = super.meta // {
    changelog = "https://git.savannah.gnu.org/cgit/hello.git/plain/NEWS?h=v2.9";
  };
})

you can now write

hello.overrideAttrs (super: {
  version = "2.9";
  src = super.src.overrideAttrs (_: {
    outputHash = "19qy37gkasc4csb1d3bdiz9snn8mir2p3aj0jgzmfv0r2hi7mfzc";
  });
})

(sadly none of the fetchers I looked at, including fetchurl, allow you to override the arguments given to them directly, but overriding outputHash works for all the fetchers I inspected (fetchurl, fetchFromGitHub, and fetchzip)).

As a second step, this PR also updates overrideAttrs to support its argument as either super: { … )} or self: super: { … }, in case the overrideAttrs expects that it might be further customized and wants to introduce its own self-referential values.

This works by passing the super value to the argument and checking if it gets a function back; if so, it re-calls the argument with self super instead. I chose this approach because any existing overrideAttrs expects just super and must return an attrset, so if we get a function then it must be the new approach.

I also updated the stdenv adapters, though I haven't actually tested them.

TODO

• Update documentation on stdenv.mkDerivation.
• Update documentation on overrideAttrs.
• Go over all packages I maintain and convert any that use rec { … } to this new style.
• Determine if this impacts performance at all.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[lilyball]

My biggest question here is whether this impacts performance. For existing code, this effectively changes mkDerivation to go from mkDerivation attrs to mkDerivation (lib.fix (_: attrs)) plus a call to lib.isFunction (I'm ignoring overrideAttrs since that's rarely used within nixpkgs itself). I'm hoping there's no measurable difference with this change, but I'm also not sure how to reasonably measure this.

If there is a measurable performance difference, the definition of mkDerivation can be split in two based on the result of lib.isFunction such that one path looks just like the old code. But I'm guessing that lib.fix (_: attrs) is negligible.

[lilyball]

My other thought is that overriding derivations is still annoying when dealing with fetchers. It might be nice to introduce something like lib.recursiveUpdateDrvs that works kind of like lib.recursiveUpdate by merging attrsets, except if it finds a derivation on the lhs and an attrset on the rhs it would call overrideAttrs on it (to be specific, if the lhs is a derivation with an overrideAttrs attr and the rhs is a non-derivation attrset). This way I could write

lib.recursiveUpdateDrvs hello {
  version = "2.9";
  src.outputHash = "…";
}

and it would evaluate to

hello.overrideAttrs (super: {
  version = "2.9";
  src = super.src.overrideAttrs (_: {
    outputHash = "…";
  });
})

This is less obviously helpful with the current state of nixpkgs, because src generally needs to be replaced rather than merely overridden (due to the inability to change the arguments passed to fetchers).

[nixos-discourse]

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

https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293/12

[lilyball]

I guess a problem with the proposed lib.recursiveUpdateDrvs is if fetchers become overridable then I'd actually want to be able to say

lib.recursiveUpdateFoo hello {
  version = "2.9";
  src.sha256 = "…";
}

as something that expands to

hello.overrideAttrs (super: {
  version = "2.9";
  src = super.src.override {
    sha256 = "…";
  };
})

So I'll sit on that thought for a while; it's orthogonal to this PR anyway.

[DamienCassou]

In your example,

hello.overrideAttrs (super: {
  version = "2.9";
  src = super.src.overrideAttrs (_: {
    outputHash = "19qy37gkasc4csb1d3bdiz9snn8mir2p3aj0jgzmfv0r2hi7mfzc";
  });
})

the new derivation doesn't have any way to fetch the sources: they must already be there. Doesn't this make the configuration non-reproducible (in the sense that if I haven't downloaded the sources first, I will get an error)?

[lilyball]

@DamienCassou I don't know what you mean. The new derivation inherits the src attribute from the original, only overriding the outputHash. The whole point of this change is the src attribute from the original will now pick up the overridden version.

With this PR, this code

hello.overrideAttrs (super: {
  version = "2.9";
  src = super.src.overrideAttrs (_: {
    outputHash = "19qy37gkasc4csb1d3bdiz9snn8mir2p3aj0jgzmfv0r2hi7mfzc";
  });
})

produces the exact same resulting derivation as this code

hello.overrideAttrs (super: {
  version = "2.9";
  src = fetchurl {
    url = "mirror://gnu/hello/hello-2.9.tar.gz";
    sha256 = "19qy37gkasc4csb1d3bdiz9snn8mir2p3aj0jgzmfv0r2hi7mfzc";
  };
  meta = super.meta // {
    changelog = "https://git.savannah.gnu.org/cgit/hello.git/plain/NEWS?h=v2.9";
  };
})

but it's a lot less code and doesn't require repeating information from the original derivation such as the URL.

[DamienCassou]

I understand now, thank you.

[schlichtanders]

This looks like a natural improvement of current mkDerivation.
Is it still in progress? I would like to see it becoming part of nix

[lilyball]

I'm not in a position to move this forward, but I would be quite happy to see someone else shepherd this.

[stale]

I marked this as stale due to inactivity. → More info

[roberth]

I've independently come up with the same idea and roughly the same implementation. It is in a mergeable state, adds a public attribute to represent the final package and it updates the documentation. #119942
I've also determined the performance impact to be negligible, especially when inlining fixed-points.nix functions to save on envs and calls.
Please considering reviewing #119942