Proposal: move values from derivation attributes to function arguments

[vojta001]

TL;DR

I propose to move most hardcoded values from derivation attributes to default-valued function arguments.

Example (simplified ipxe expression)

Go from this

{ stdenv, lib, fetchFromGitHub, unstableGitUpdater
, gnu-efi, mtools, openssl, perl, xorriso, xz
, syslinux ? null
, embedScript ? null
, additionalTargets ? {}
}:
stdenv.mkDerivation rec {
  pname = "ipxe";
  version = "unstable-2022-04-06";

  src = fetchFromGitHub {
    owner = "ipxe";
    repo = "ipxe";
    rev = "70995397e5bdfd3431e12971aa40630c7014785f";
    sha256 = "SrTNEYk13JXAcJuogm9fZ7CrzJIDRc0aziGdjRNv96I=";
  };

  enabledOptions = [
    "PING_CMD"
    "IMAGE_TRUST_CMD"
    "DOWNLOAD_PROTO_HTTP"
    "DOWNLOAD_PROTO_HTTPS"
  ];

  configurePhase = ''
    runHook preConfigure
    for opt in ${lib.escapeShellArgs enabledOptions}; do echo "#define $opt" >> src/config/general.h; done
    substituteInPlace src/Makefile.housekeeping --replace '/bin/echo' echo
  '' + lib.optionalString stdenv.hostPlatform.isx86 ''
    substituteInPlace src/util/genfsimg --replace /usr/lib/syslinux ${syslinux}/share/syslinux
  '' + ''
    runHook postConfigure
  '';
…

to this

{ stdenv, lib, fetchFromGitHub, unstableGitUpdater
, gnu-efi, mtools, openssl, perl, xorriso, xz
, syslinux ? null
, embedScript ? null
, additionalTargets ? {}
, rev ? "70995397e5bdfd3431e12971aa40630c7014785f"
, sha256 ? "SrTNEYk13JXAcJuogm9fZ7CrzJIDRc0aziGdjRNv96I="
, enabledOptions ? [
    "PING_CMD"
    "IMAGE_TRUST_CMD"
    "DOWNLOAD_PROTO_HTTP"
    "DOWNLOAD_PROTO_HTTPS"
  ]
}:
stdenv.mkDerivation rec {
  pname = "ipxe";
  version = "unstable-2022-04-06";

  src = fetchFromGitHub {
    owner = "ipxe";
    repo = "ipxe";
    inherit rev sha256;
  };

  configurePhase = ''
    runHook preConfigure
    for opt in ${lib.escapeShellArgs enabledOptions}; do echo "#define $opt" >> src/config/general.h; done
    substituteInPlace src/Makefile.housekeeping --replace '/bin/echo' echo
  '' + lib.optionalString stdenv.hostPlatform.isx86 ''
    substituteInPlace src/util/genfsimg --replace /usr/lib/syslinux ${syslinux}/share/syslinux
  '' + ''
    runHook postConfigure
  '';
…

Reasoning

I have always been annoyed by the fact upgrading a package using an override required specifying the whole src attribute again instead of just passing the new rev and sha256, however once I learnt how to do it properly, I simply forgot about it. I have however spent multiple hopeless hours recently trying to figure out why using overrideAttrs in the ipxe derivation to override enabledOptions didn’t work. Sure, an experienced Nixer will quickly realize configurePhase is computed before overrideAttrs takes place and changing the enabledOptions attribute does not trigger a seemingly unrelated function recomputation. That explains this is not a proper way to do it, but does not help with finding the proper one. Inlining both enabledOptions and configurePhase into the overrideAttrs is not a good option either as the duplication rapidly increases maintenance burden and somewhat kills the idea of using Nix to comply with the DRY principle.

I therefore propose to move as many values as possible into default-valued function arguments as demonstrated in the TL;DR section. It would fix the mentioned problems, but still keep the original behavior possible – overrideAttrs behaves the same, but override begins to be useful.

I was thinking about publishing this as an RFC, but I doubt I am capable of pushing it to the end. There also must be a reason we are currently doing it the way we are. What it is? Why are we putting configuration into derivation attributes? Is there any catch in my approach?

[ElvishJerricco]

I don’t think it’s a crazy way to write packages. It would make some things easier. I don’t think we need a concerted effort to migrate everything to this, but doing it gradually would be reasonable. i.e. new packages and updates could be encouraged (though probably not required) to do this.

[jonringer]

src points to a different derivation. rev and sha256 doesn’t really relate to the package, but the source.

As for enabledOptions, I’m not sure if override of overrideAttrs would be a better “api”. However, moving something to the inputs always comes at the cost that there may be a package by the same name, and your input logic is destroyed unintentionally.

The place where putting something into the inputs makes the most sense, is if it needs to be reference in multiple locations. There you do want to trigger re-evaluation through override as overrideAttrs would create invariance.

[ElvishJerricco]

@jonringer

However, moving something to the inputs always comes at the cost that there may be a package by the same name, and your input logic is destroyed unintentionally.

Oh, this is a really good point…

[vojta001]

@jonringer

However, moving something to the inputs always comes at the cost that there may be a package by the same name, and your input logic is destroyed unintentionally.

Oh, that’s a good point. We could then prefix them with _ or something

[tejing]

There’s already a solution to this in place, packages just have to start using it. Instead of passing an attrset to mkDerivation, you pass a function, and you pull your self references out of the argument to the function instead of using rec. If you do that, overrideAttrs will properly change derivative values of what you override. You can attach extra (useless) attributes to the main derivation for the url & hash, just so they can be more easily overridden as well, if you want. See this section of the nixpkgs manual.

[vojta001]

Thanks, I didn’t know about it. What is the advantage/difference over my proposed solution?

[Sandro]

@vojta001

, enabledOptions ? [
    "PING_CMD"
    "IMAGE_TRUST_CMD"
    "DOWNLOAD_PROTO_HTTP"
    "DOWNLOAD_PROTO_HTTPS"
  ]

If you want to append something you need to repeat all the defaults which is not nice.

@jonringer

src points to a different derivation. rev and sha256 doesn’t really relate to the package, but the source.

Also it is a fixed output derivation that’s why only overwriting rev is not working.

@jonringer

your input logic is destroyed unintentionally.

and silently

[amjoseph]

@vojta001

Thanks, I didn’t know about it.

It’s pretty new; it merged only three months ago. It’s a very cool feature, I use it a lot now.

@vojta001

What is the advantage/difference over my proposed solution?

I think it’s just a cleaner strategy for implementing your idea: the changes are much smaller and more localized. In the version above, overrideAttrs { enabledOptions = ...}; should work as expected.

For overriding src.rev you still need to replace the entire src attribute (i.e. overrideAttrs { src = fetchFromGithub ... }), so there is still merit in your proposal. Maybe rephrase your example to use finalAttrs: where possible and pitch based on the remaining benefit in the “deep attribute replacement” case?

Keep in mind, however, that there are a lot of maintainers who are opposed to the idea of people being able to compile packages differently than the way they compile them (without a lot of effort). For example, this two-line PR has languished for five months.

[amjoseph]

Oh yeah, one more reason why I like #119942: “please replace stdenv.mkDerivation rec { with stdenv.mkDerivation (finalAttrs:” is a nice short and very unambiguous review comment.

Previously, figuring out how to eliminate stdenv.mkDerivation rec { was not always obvious or simple, and could require significant restructuring of the expression. So requests to make overrideAttrs work properly for a package could devolve into debates about uglification of the code.

[amjoseph]

@Sandro

@jonringer

your input logic is destroyed unintentionally.

and silently

Indeed. In light of this, how about:

1. use stdenv.mkDerivation (finalAttrs: {... instead of rec
2. move inputs to any nested derivations into passthru. For the vast majority of packages, the src fetcher is the only nested derivation.

Here’s a concrete example.

Having access to the src subderivation is independently useful – sometimes one package needs to grab a file out of another package’s source code.

[Sandro]

@amjoseph

use stdenv.mkDerivation (finalAttrs: {... instead of rec

If this is just used in src there is no improvement. Just overwrite src in the overlay.

@amjoseph

Here’s a concrete example.

This should be done in universally in fetchFromGitHub.

[jonringer]

@amjoseph

use stdenv.mkDerivation (finalAttrs: {... instead of rec

I’ve also found that usage of finalAttrs is not great in cross compilation

[amjoseph]

@jonringer

I’ve also found that usage of finalAttrs is not great in cross compilation

How so? I do a lot of cross compilation. Have you found any problems that aren’t fixed by #185077?

[jonringer]

finalAttrs.package won’t exist last time I tried it.

Not sure about the PR. I just reverted back to using a let block and assigning the package to self fixed point

[amjoseph]

@jonringer

finalAttrs.package won’t exist last time I tried it.

It still exists. It was renamed to finalPackage. Not sure what this has to do with cross-compilation.

@jonringer

I just reverted back to using a let block and assigning the package to self fixed point

This breaks overlays.

[amesgen]

Wow, that seems like a very cool feature! One thing that comes to mind: mkDerivation-like functions such as buildRustPackage or buildGoModule do not yet support this new finalAttrs style. The initial proposal in this thread requires no changes to those functions; but how difficult/subtle would it be adapt these functions to support finalAttrs? Or is it actually straightforward, but this change is new enough that nobody has gone through with it?

[nrdxp]

@amjoseph

Having access to the src subderivation is independently useful – sometimes one package needs to grab a file out of another package’s source code.

src is already available without having to explicitly dump it in passthru, but I do like the example.

[tejing]

Isn’t the fix for sub-derivations to just make fetchers sensibly overrideable like everything else?

I’m envisioning a version override process for most packages that looks like

pkg.overrideAttrs (old: {
  version = "new_version";
  src = old.src.overrideAttrs (_: {
    sha256 = "new_sha256";
  });
})

where the url/rev/etc are typically calculated from version, and if not, you’ll need to add more attributes to the src override.

[tejing]

@amesgen

Or is it actually straightforward, but this change is new enough that nobody has gone through with it?

I’m not aware of any reason it would be difficult. It just hasn’t been done yet afaik.