Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC 0067] Common override interface derivations #67

Closed
wants to merge 5 commits into from

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Mar 18, 2020

This RFC came after the discussion on NixOS/nixpkgs#46842 and NixOS/nixpkgs#82772.

rendered

@edolstra edolstra changed the title Common override interface [RFC 0069] Common override interface Mar 18, 2020
@edolstra edolstra changed the title [RFC 0069] Common override interface [RFC 0067] Common override interface Mar 18, 2020
[alternatives]: #alternatives

An alternative would be to add a new method for overriding, `.overrideArgs`, thus allowing one to still call `.overrideAttrs` to override `stdenv.mkDerivation`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative could be some mechanism to unify override/overrideAttrs into a single override mechanism supporting multiple (probably named?) levels, like overrideCall pkg "callPackage" (x: {…})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very much this, and it's also what NixOS/nixpkgs#87394 seems to implement. I think this is the only way to not end up with a mess when you have arbitrarily deeply nested function calls.

With this

  • .override becomes .overrideCall "callPackage" { ... }
  • .overrideAttrs becomes .overrideCall "mkDerivation" { ... }
  • .overridePythonAttrs becomes .overrideCall "buildPythonPackage" { ... }
  • .overrideDerivation becomes .overrideCall "derivation" { ... }
  • etc.

With this, there's much less confusion as to how you need to override what. And it's a unified interface for the current override functions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach, but I do want to clarify that from a UI point of view, end users should typically need one and only one function that helps them. That is my motivation for shadowing .overrideAttrs. Of course, if we can provide something like proposed here, as well as a call that is expected to override what is typically expected, then that would be great.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fail to see how this helps to be honest, because the difficult part (at least in my opinion) is understanding the semantics of the different override functions, not their actual name. But all this (I'm using @infinisil's example here because it is concrete) does is rename override to overrideCall "callPackage" etc. but I still have to look at the code to see which of the strings (instead of functions) I have to use to achieve some specific goal.

I also don't know if shoving all these functions under one name will be very helpful documentation wise, as now I have to read more, and more importantly about a wider range of topics, to find out if this one function will solve my problem or not.

I think the problem lies more in the fact that it is difficult to grasp all the mechanics at play in nixpkgs, but I don't think that's solved by overloading a function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we need to be better at documenting the parameters and how they transform the data. In the past there were ideas for attaching docstrings to functions NixOS/nixpkgs#23505.

@zimbatm
Copy link
Member

zimbatm commented Mar 20, 2020

The RFC in its current state is a bit sparse in details. Is it possible to add more details on the implementation and how it would work when builders are nested?

@infinisil
Copy link
Member

Nominating myself as a shepherd

@FRidh
Copy link
Member Author

FRidh commented May 9, 2020

I will clearly need to extend this but just dumping here some thoughts.

Consider e.g. the Python packages set that contains primarily Python packages (.overridePythonAttrs), but also bindings to non-buildPythonPackage packages .overrideAttrs.

In the last years that we've had .overridePythonAttrs there has not been a single case where .overrideAttrs was actually needed.

@7c6f434c
Copy link
Member

7c6f434c commented May 9, 2020

In the last years that we've had .overridePythonAttrs there has not been a single case where .overrideAttrs was actually needed

This, apparently, happens because you have these four lines of code that allow arbitrary overrides of mkDerivation via buildPythonPackage arguments.

I am not sure whether we want to commit to always having this in every package creation function, and not use generic nested overrides. I am not even sure it is better in the case #58 gets unstuck and implemented and released and usable in Nixpkgs in a couple of years.

Separately, if we are to touch the overrides at all, maybe say that the correct way to use overrides is lib.override* x something, not x.override something? I have an impression that if we start optimising the override memory cost, the former approach will leave us more freedom.

@B4dM4n
Copy link

B4dM4n commented May 13, 2020

Since opening NixOS/nixpkgs#87394 and being redirected here I experimented a bit more with different overriding variants.

I'm currently favoring a layered approach over shadowing lower functions to keep the current flexibility of nixpkgs.

The difference might not be a big deal in nixpkgs because it's self contained and developers have full control of the definition and usage of a function, but projects outside nixpkgs or even the configuration.nix (which got me into implementing NixOS/nixpkgs#87394) might want to access any of the provided override functionality for small modifications.

The current set of overriding capabilities by all the . override* functions is hard to understand when using them for the first time. Some of these functions are called with a set, others take a function returning a set and some might also accept both.

Especially when working outside nixpkgs (projects that use nixpkgs) it can sometimes take two or three of these to replace a single dependency, so removing override functions might restrict users from using the nixpkgs package and simply copy paste the code into their own files:

  • .override to replace a input package
  • .overridePythonAttrs to switch a build step
  • .overrideAttrs to change a buildPythonPackage behavior

Sure, it can all be done with one .overrideDerivation, but this wouldn't be very user friendly.

In the last years that we've had .overridePythonAttrs there has not been a single case where .overrideAttrs was actually needed.

Even if .overrideAttrs isn't needed inside nixpkgs, changing it's behavior from override mkDerivation to override the top level builder would probably break most of the code outside of nixpkgs that relies on this.

Since NixOS/nixpkgs#87394 I have tried to implement a layered variant of makeOverridable.

The first attempt used the shadowing approach for .overrideAttrs and included a overrideAttrsNested [ ] function to override deeper levels.
This wan't very ergonomic, because even a simple task as override passthru in mkDerivation (from pythonPackages.toPythonModule) wasn't as trivial to implement as before, because the function had to check how many layers has the builder of this derivation.

The second attempt (B4dM4n/nixpkgs@2793c4a) uses a "named layers" approach where each named layer can be specifically overridden and also allows to override layers bottom up without naming them. Unnamed layers (produced by makeOverridable) can be inserted in between without influencing the layers, but providing the .override function.

It currently adds two new functions to provide it's features:

  • .overrideLayerName to override a specific layer by name. This can be use do replace functions like .overridePythonAttrs (B4dM4n/nixpkgs@34d6cc5)
  • .overrideLayers allows overriding multiple layers at the same time either by name or as a list from bottom to top.

Theses two examples are equivalent (with the current set of named layers):

nixpkgs-fmt.overrideLayers {
  derivation = (old: {
    buildInputs = old.buildInputs ++ [ hello ];
  });
  rust = {
    version = "0.9.1";
    cargoSha256 = "1wybvm9qckx9cd656gx9zrbszmaj66ihh2kk6qqdb6maixcq5k0b";
  };
}
nixpkgs-fmt.overrideLayers [
  (old: {
    buildInputs = old.buildInputs ++ [ hello ];
  })
  {
    version = "0.9.1";
    cargoSha256 = "1wybvm9qckx9cd656gx9zrbszmaj66ihh2kk6qqdb6maixcq5k0b";
  }
]

As a side effect from moving all override handling into one place, this also allows to use attribute sets and functions interchangeably as override arguments.

With this approach .overrideDerivation and .overrideAttrs could in theory be deprecated as they can be replaced by named layers and with free functions for convenient access.

@Mic92
Copy link
Member

Mic92 commented Jun 25, 2020

We still don't have enough shepherds here to move this RFC to the next phase.

@Taneb
Copy link

Taneb commented Jun 25, 2020

I'd like to nominate myself to help shepherd this PR.

@FRidh FRidh changed the title [RFC 0067] Common override interface [RFC 0067] Common override interface derivations Jul 1, 2020
@FRidh
Copy link
Member Author

FRidh commented Jul 1, 2020

Clarified the scope is overriding of derivations, not package sets.

@Mic92
Copy link
Member

Mic92 commented Feb 18, 2021

I think someone else could also provide doodle link.

@FRidh
Copy link
Member Author

FRidh commented Mar 13, 2021

Hooks often rely on environment variables. When a builder builds upon that hook, it means the environment variables used by the hook need to be declared by the builder as well.

@Mic92
Copy link
Member

Mic92 commented Mar 18, 2021

@FRidh can you organize a meeting? https://www.when2meet.com/

@Mic92
Copy link
Member

Mic92 commented May 27, 2021

Is this RFC stalled? @infinisil, @edolstra, @Taneb, @7c6f434c
It looks like no one is interested in organizing a meeting.

@7c6f434c
Copy link
Member

Hm. I am not sure, @infinisil posted a pretty nice proposal as intermediate and I am not sure if there was any later changes to it not shared or should we start from there and try to move forward (I still hold the opinion expressed in the shpeherd meeting notes that the approach needs some changes, indeed in the direction @infinisil has tried)

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/help-with-overrideattrs/13667/3

@spacekookie
Copy link
Member

@infinisil, @edolstra, @Taneb, @7c6f434c since this RFC has stalled we will close it for the time being. Please feel free to re-open it again once work on this continues

@roberth
Copy link
Member

roberth commented Jan 25, 2022

NixOS/nixpkgs#119942 (which, I should add, is fully backwards compatible and has merit in its own right) changes the landscape a bit. It could probably simplify the python expressions by making overridePythonAttrs unnecessary, or even an alias for overrideAttrs, defined via passthru.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/using-nix-today-a-usability-quagmire/22323/2

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/yants-composable-nix-types-with-polymorphism-structs-enums-matching-and-more/3666/9

@roberth
Copy link
Member

roberth commented Jul 11, 2023

I think it's worth noting that if you can separate your overriding pipeline into separate functions each with a name, you'r basically one step away from putting those attrsets in a module together, with the additional benefit of having access to all layers without even having to think about layers.

I suppose either a module-like solution or the more constrained interface proposed here can solve the issue of callPackage having to be aware of mkDerivation's returned overriding functions.

As a final thought, having to serialize the chain of overriding methods doesn't generalize too well. Perhaps we should first have a pattern where each overridable thing has a function as its parameter that specifies how its creator can be overridden. That may solve aforementioned architectural problem when, say, a package defined an extra overriding methods using passthru.extraOverrideAttrs = args: finalAttrs.finalPackage.overrideAttrs (foo args). I don't see how the pipeline of named functions idea would allow the equivalent of pkg.extraOverrideAttrs.override to work.
Probably the definition should look somewhat like passthru.extraOverrideAttrs = args: (finalAttrs.finalPackage.overrideAttrs (foo args)) // callerOverrides to support that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.