lib.extendMkDerivation: init

[ShamrockLee]

Description of changes

This PR introduces a unified approach to implementing build helpers that support fixed-point arguments and bring such support to existing build helpers.

The fixed-point arguments support in stdenv.mkDerivation is introduced in #119942, and there are ongoing attempts to make other build helpers support such syntax (buildRustPackage refactor #194475, buildGoModule refactor #225051). The overlay style overrideAttrs brought by the stdenv.mkDerivation change can be used to implement the functionality, which is adopted by the buildRustPackage refactor and the previous version of the buildGoModule refactor. The challenge of such an approach is that the whole set pattern matching the input arguments are degenerated into a single identifier, making it hard to see from the source code which attributes to the build helper accepts.

The new Nixpkgs Library function, lib.extendMkDerivation accepts a base build helper and an attribute overlay (an overlay in the form finalAttrs: args: <attrsToUpdate>), and returns a new build helper by extending the base build helper with the attribute overlay via its <pkg>.overrideAttrs.

The following is the definition of an example build helper, mkLocalDerivation:

lib.extendMkDerivation {
  constructDrv = stdenv.mkDerivation;
  extendArgs =
    finalAttrs:
    {
      preferLocalBuild ? true,
      allowSubstitute ? false,
      ...
    }@args:

    # No need of `// args` here.
    # The whole set of input arguments is passed in advance.
    {
      # Attributes to update
      inherit preferLocalBuild allowSubstitute;
    };
}

For existing build helpers with arguments that cannot be passed to the base build helper, lib.extendMkDerivation provides an attribute removedAttrNames to specify arguments not to pass down to the base build helper.

lib.extendMkDerivation {
  constructDrv = stdenv.mkDerivation;
  extendArgs =
    finalAttrs:
    {
      preferLocalBuild ? true,
      allowSubstitute ? false,
      specialArg ? (_: false),
      ...
    }@args:

    {
      # Arguments to pass
      inherit preferLocalBuild allowSubstitute;
      # Some expressions involving specialArg
      greeting = if specialArg "hi" then "hi" else "hello";
    };

  removedArgNames = [
    "specialArg"
  ];
}

Aside from removedAttrNames lib.extendMkDerivation, other optional arguments are used to manipulate their behaviors. For example, lib.adaptMkDerivation { transformDrv = toPythonModule; } applies the function toPythonModule to the derivation produced by the derived build helper.

Cc:
Python maintainers: @FRidh @mweinelt @jonringer
Author of the buildRustPackage refactor PR @amesgen
Reviewer of the buildGoModule refactor PR @zowoq
Author of the merged recursive stdenv.mkDerivation PR @roberth
People who mention 119942 in Python application definition: @LunNova

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/)
• 23.05 Release Notes (or backporting 22.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
• Fits CONTRIBUTING.md.

[ShamrockLee]

I should have checked more carefully.

There's a function called makeOverride that deprives the input builder of their recursive attributes support, and buildPythonPackage and buildPackageApplication happens to be makeOverrided. While the change keeps the current python packages unchanged, it still doesn't bring the recursive attributes support to buildPythonModule and buildPythonApplication unless we fix makeOverridable.

Updat: fixed, see below.

[ShamrockLee]

Fix lib.makeOverridable and makeOverridablePythonPackage. Now buildPython* accepts recursive attributes.
Turn a Python application pyspread into the recursive attributes style.

[nixos-discourse]

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

https://discourse.nixos.org/t/proposal-move-values-from-derivation-attributes-to-function-arguments/20697/21

[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/18

[roberth]

A mix of incremental and fundamental suggestions, because I can't decide for the nixpkgs-python maintainers.

I'm concerned about the increasing complexity, while the goal can be achieved through simplification instead: removing the python-specific level of overriding and turning it into an "overlay" on the mkDerivation arguments instead.

The alternative strategy is to

1. Provide this python layer, which contains the relevant mkPyton* logic in a way that works with overlay-style overriding. This can be done by reading the existing code, attribute for attribute, and adding the logic to the python layer.
2. Change the python packages to use that layer in combination with mkDerivation, instead of the current mkPython* functions.
3. Perhaps make the mkPython functions reuse the overlay so that they don't literally reimplement the same logic. I don't know if this would be worthwhile.
4. Eventually deprecate the mkPython* functions.

I have played around with step 1 of the alternative strategy. It is feasible, but it requires a bit of migration for each package. I'm not a nixpkgs-python maintainer, so I don't feel like I should be the one to make the decision whether to accept the complexity of this PR, or refactor to actually simplify the python logic by fitting it into a mkDerivation layer.

[roberth]

It's worth noting that finalAttrs contains the final arguments to mkDerivation attrs, and not the final arguments to the function you're constructing.

[ShamrockLee]

Now the arguments after applying the modifier function is also considered.

IMO, the finalAttrs is meant to be the final state of the attributes passing to the base builder (mkDerivation). That's how user could access finalAttrs.finalPackage and other goodies. If we just want to get the input argument set manually, the user could just lib.fix the function themselves.

Input arguments that doesn't mean to be passed to the base builder are subject to special care, and should never enter the recursion. Those not-to-pass arguments are also the reason why builder overlays are not drop-in replacement of current builders.

[ShamrockLee]

I tried to add those special arguments to finalAttrs, but that greatly increase the complexity of the code and makes behavior surprising (the specified result will be different from what is got through finalAttrs by design).

A better way would be to encourage passing all the arguments. When all the arguments are passed, they will all be available inside finalAttrs, and we could then switch to the overlay-based workflow specification (from the current, build-helper-based one).

[roberth]

This feels too custom or repeated (not sure yet which). Does this add mkDerivation-like fixpoint logic at the python attrs level?

I believe we should merge the python attrs level into the mkDerivation attrs, so that the interface and implementation become simpler. Having multiple levels of overriding has a huge complexity cost, so getting rid of an unnecessary level would be a huge win. We'd get rid of overridePythonAttrs and all the user facing complexity, implementation complexity and bugs that come with it.

The python-specific attrs can almost be implemented as an "overlay" on the mkDerivation arguments. When I tried this, I think only like 3 attributes had the same name but a slightly different meaning. That made it a breaking change, but migrating those attributes is feasible and would vastly simplify the python/mkDerivation wiring.

[FRidh]

With same names that is generally speaking because the Python builder will extend the lists with some "defaults". That's really the only value of the custom builder over just plain hooks.

[ShamrockLee]

This feels too custom or repeated (not sure yet which). Does this add mkDerivation-like fixpoint logic at the python attrs level?

I personally prefer passing most of the attributes into mkDerivation, encourage the use of overrideAttrs and gradually deprecates overridePythonAttrs as well as other builder-specific override methods.

That will be a mass rebuild, so I just work around the makeOverridablePythonPackage obstacle in order to demonstrate the possibility to add the recursive attributes support without rebuild.

[infinisil]

Recently the Packages Modules Working Group started investigating whether something like the module system could be used to evaluate packages. We're tracking all work in this repository, meeting weekly, but working mostly asynchronously. It would be great if you could join the Matrix room of the WG and chat with us, or even join the team yourself to work on such issues!

[ShamrockLee]

Thank you for taking time reviewing this!

I'm concerned about the increasing complexity, while the goal can be achieved through simplification instead: removing the python-specific level of overriding and turning it into an "overlay" on the mkDerivation arguments instead.

The idea to shift workflow-specific overlays sounds neat, and that could also be friendlier when packaging multi-language projects. Nevertheless, there are some issue on the way to the switch:

1. Current builders rely more or less on arguments they won't pass into mkDerivation attribute set.
   Developers would need to change the way they handle the arguments / attributes, either by passing them directly or through passthru. The impl input argument of the proposed function is essentially an overlay that allows passing not-to-pass-to-mkDerivation arguments through the previous set.

2. Current attempt to try to implement a builder that supports the (finalAttrs: { }) through an overlay passing to .overrideAttrs, such as rustPlatform.buildRustPackage: support finalAttrs style #194475, discards the set pattern of input arguments ({ a, b ? "some default", ... }@args) completely and degenerate it into previousAttrs.
   It is a pity, since the set patterns itself serves as an interface of the builder, providing information about the expected input arguments to both and the Nix interpreter and people reading the Nix expression.
   The solution would be rather simple -- replace the previousAttrs with the set pattern, and then the remaining issue will be point 1.

Overall, the goal of the proposed function is to add (finalAttrs: { }) input support with minimum changes to the existing expressions, and raise the discussion about general approaches to bring such supports to builders.

[fricklerhandwerk]

Where do we find information about extendDerivation?

[ShamrockLee]

lib.customisation is currently not presented in the Nixpkgs manual. We'll need to add the documentation for those functions before referring to them.

[ShamrockLee]

Besides, the Nixpkgs Library Functions documentation automatically generated from the comment don't have anchors. So there's no way to link against them so far.

[ShamrockLee]

My fault. The anchor in the form function-library-<long attribute path> is also automatically generated. E. g. function-library-lib.customisation.extendMkDerivation.

This usage is very hard to discover, as the documentation is missing, and the manual provides no hyperlink in the title of each function document.

[ShamrockLee]

I addressed this review in the changes to add a new section about using lib.extendMkDerivatien to define build helpers. Please take a look.

[LunNova]

Just reviewing your change to input-remapper, I like how much this cleaned up its definition. :)

[ShamrockLee]

@philiptaron, how do you think about leaving only lib.extendMkDerivation but adding the optional removeAttrs functionality, as mentioned in #234651 (comment) ?

If people prefer the current implementation, I could still revert.

[philiptaron]

Couple curious questions.

[philiptaron]

What do you think about leaving only lib.extendMkDerivation but adding the optional removeAttrs functionality?

I like adding the removeAttrs functionality. I agree with the linked comment that having a way to remove attributes, not just add them, is needed for these adapters and extenders.

[philiptaron]

I think this is now as simple as it needs to be. I'm approving, trusting that you'll finish dotting the i's and crossing the t's before merge.

[philiptaron]

I found your worked example of a writeTextFile helper very helpful. Thanks for sharing that.

requested change ("make as simple as possible, one function") completed

[ShamrockLee]

I addressed @philiptaron's naming suggestions and simplified the documentation.

As the transition process to reduce excluded/removed arguments and limit custom overriders like overridePythonAttrs is not lib.extendMkDerivation-specific and requires consensus of community members, I removed it from the documentation added by this PR.

[ShamrockLee]

@fricklerhandwerk, IIRC, the changes you requested concern mainly the documentation. If you are available, could you kindly review the (now greatly simplified) documentation?

addressed a long time ago

[philiptaron]

I'm going to give this a final review then merge today, barring any late objections. Thanks for your two years of work on this, @ShamrockLee.

[philiptaron]

A few thoughts on the implementation. The most important one is about the resulting attrset, since that'd be hard to roll back.

[nixos-discourse]

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

https://discourse.nixos.org/t/is-it-possible-to-override-cargosha256-in-buildrustpackage/4393/21

[PerchunPak]

So this does not do anything that would replace overridePythonAttrs?

[ShamrockLee]

So this does not do anything that would replace overridePythonAttrs?

This PR alone does not. Replacing overridePythonAttrs with overrideAttrs involves a lot of work, some part of it would also make it easier to convert to extendMkDerivation.

[ShamrockLee]

@PerchunPak The transition from overridePythonAttrs to overrideAttrs is at #376372, and buildPython* (finalAttrs: { ... }) at #271387.

#375921 helps shorten the diffs of the above two PRs. Reviews and feedback are welcome!