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

Overlay/overriding improvement plan #99100

Open
infinisil opened this issue Sep 29, 2020 · 14 comments
Open

Overlay/overriding improvement plan #99100

infinisil opened this issue Sep 29, 2020 · 14 comments
Labels
0.kind: bug architecture Relating to code and API architecture of Nixpkgs

Comments

@infinisil
Copy link
Member

Nixpkgs overlays/overrides are way too hard to get right. Here's a seemingly innocent example:

self: super: {
  python2Packages = super.python2Packages.override (old: {
    overrides = pself: psuper: {
      myPythonPkg = pself.callPackage ./pkg.nix {};
    };
  });
}

But it has problems:

  • pkgs.pythonPackages and pkgs.python2.pkgs won't contain the package, even though it should point to pkgs.python2Packages
  • Multiple such overlays will override each other, only applying the latest one.

Here's a Haskell example:

self: super: {
  haskellPackages = super.haskellPackages.extend (hself: hsuper: {
    myHaskellPkg = hself.callPackage ./pkg.nix {};
  });
}

But it has problems:

And this is just two package sets. There's also:

  • The pkgs.linuxPackages set, which uses lib.makeExtensible for allowing overrides with .extend, but not with .override
  • The lib.makeScope mechanism, which creates a new scope that can be extended with .overrideScope', which works very much the same as .extend. Don't use .overrideScope though, that has its arguments flipped around.
  • The nixpkgs function pkgs.appendOverlays which works very much the same as pkgs.extend, except for multiple overlays
  • pkgs.perlPackages doesn't support overriding with scoped overlays at all. You need to use super.perlPackages.<pkg> to refer to a previous perl package, or self.perlPackages.<pkg> for final ones
  • Pretty much every package set uses its own way to provide overrides (and some don't allow that at all), and it's impossible to know how it should be used by looking at the source code
  • How derivations themselves get overridden, there's both .override, .overrideAttrs and .overrideDerivation, and it's hard to know which one is right
  • Overriding derivations can be misleading. People try to use .overrideAttrs (old: { version = "1.2"; }) and wonder why it doesn't override the version
  • Knowing when to use self and super is very tricky. And sometimes very intuitive. In fact, using self to avoid infinite recursion can sometimes even introduce inconsistencies.

It's just a big mess!


This issue should serve as a place to track this problem and how it can be improved. I myself have thought about this a bunch, and will soon elaborate my idea to fix all these problems in a subsequent comment.

@infinisil
Copy link
Member Author

For an ideal overlay interface, a change like NixOS/nix#4090 would be very beneficial

@worldofpeace
Copy link
Contributor

It's just a big mess!

I totally agree, there's way to many things to get wrong and little corners. When I first started using nix there were overlays and I had a pretty confused time.

I will also add that on nixUnstable there isn't self: super: anymore.
It's final: prev:, which I think makes it a bit more obvious with the proper documentation.

@worldofpeace
Copy link
Contributor

Might I add there was no helpful message in nixUnstable for overlays that used self: super:...

@infinisil
Copy link
Member Author

Those are just arbitrary arguments btw, they can even be glitter: sparkles:! The super/self is just a convention really

@worldofpeace
Copy link
Contributor

Those are just arbitrary arguments btw, they can even be glitter: sparkles:! The super/self is just a convention really

Yep, they can be arbitrary arguments. But in nixUnstable I notice that they're not anymore because of the check https://github.com/NixOS/nix/blob/b721877b85bbf9f78fd2221d8eb540373ee1e889/src/nix/flake.cc#L260

@infinisil
Copy link
Member Author

Ahh that's only for flakes though. But neat, didn't know that.

@infinisil
Copy link
Member Author

infinisil commented Sep 30, 2020

Anyways, I should start explaining how I think this can be solved:

One of the main insights I had while thinking about this was that currently the override author is responsible for getting overrides correct. This is just how it is for how overlays currently work, you need to know how to modify the original super to include what you need.

However it doesn't have to be this way: By redesigning overlays slightly, we can shift this responsibility to the package set author. This means that whoever defines the package set will be specifying in Nix code how it should be overridden, such that the people that actually write overrides won't have to know about it.


How can this be implemented though? You see, currently overlays are just applied by essentially using super // overlay. This means that any attributes in super are just overridden completely. However super actually contains the original package set definition, the one specified by the package set author. If we want to allow them to control how overrides are applied, we need to allow them to specify how the // operation should work for their attribute!

So what we do is to change the // for overlays to instead do something like this:

left: right:
  left // builtins.mapAttrs (name: rval:
    # If the left attribute defines a custom merging strategy
    if left ? ${name}.overlayMerge
    # use that to merge the two values together
    then left.${name}.overlayMerge left.${name} rval
    # Otherwise override completely (old behavior)
    else rval
  ) right

Now with this, you can have an initial overlay that defines a package set:

self: super: {
  set = {
    foo = 10;
    # Multiple definitions of this set should be merged together
    overlayMerge = lval: rval: lval // rval;
  };
}

And another one that extends this set how you'd expect it:

self: super: {
  # Just like that!
  set.bar = 20;
}

Which with this new merging strategy will not give you { bar = 20; } like before but

{
  bar = 20;
  foo = 10;
  overlayMerge = <LAMBDA>;
}

instead! So just by changing the // for overlays, we're able to have a much better user interface. Previously to achieve the same you'd have to do

self: super: {
  set = super.set // {
    bar = 20;
  };
}

Now this is just a single insight I had. This allows us to solve the problem of overlays being very hard to get right, given that the package set author provides a good enough overrideMerge function. However there are still problems with this, which I'll get to in future comments.

@chreekat
Copy link
Contributor

chreekat commented Nov 2, 2020

@infinisil I think you have a typo in your examples - or I'm really not following how you ended up with bar = 20 and foo = 10, since those values don't show up anywhere else. :)

@infinisil
Copy link
Member Author

@chreekat Oh yes, fixed now, thanks!

@acarrico
Copy link
Contributor

acarrico commented Dec 15, 2020

I ended up here after trying to track down broken haskell overrides. I finally realized that haskellPackages.override is just broken if you use overlays. I'm debating how to work around this now. To test, I just created three overlays (test..nix, test2.nix, test3.nix) with this pattern, but obviously changing the number to 1, 2, and 3 in the corresponding overlay:

self: super:
{
  haskellPackages = super.haskellPackages.override  {
    overrides = hself: hsuper: {
      test1-override = "seen";
     };
  };
  test1-overlay = "seen";
}

Then I look in the repl:

  nix-repl> pkgs.test1-overlay
  "seen"
  nix-repl> pkgs.test2-overlay
  "seen"
  nix-repl> pkgs.test3-overlay
  "seen"
  nix-repl> pkgs.haskellPackages.test1-override
  error: attribute 'test1-override' missing, at (string):1:1
  nix-repl> pkgs.haskellPackages.test2-override
  error: attribute 'test2-override' missing, at (string):1:1
  nix-repl> pkgs.haskellPackages.test3-override
  "seen"

Intuitively, all six should be "seen". I'm debating how to work around this (I'm definitely not a nix expert). Does anyone have a patched or alternate override available for Haskell?

This situation is frustrating, because overlays seem like a great way to organize local packages on top of nixpkgs, but it requires robust merging procedures to be in place.

UPDATE

To answer my own question, I've realized that I've been through this maze before, and there are a cluster of issues and blog posts addressing this point, with various recommendations for using override/overrides/extend/composeExtensions, most of which is undocumented. Also, along the way, the existing haskell docs have apparently been pulled from the nixpkgs manual and moved to a spam site. Ugh.

UPDATE 2

The definitive answer seems to be in issue 44718. Here is my test written that way:

self: super:
{
  haskellPackages = super.haskellPackages.override (oldArgs: {
    overrides = self.lib.composeExtensions (oldArgs.overrides or (_: _: {}))
      (hself: hsuper: {
        test1-override = "seen";
      });
  });
  test1-overlay = "seen";
}

This works, but that issues seems to have been closed without being updating the documentation, which, in any case (as I indicated above) seems to have been removed from the manual. Also, to me this is just boilerplate, I'd love an explanation of what this is actually doing.

Thanks.

@acarrico
Copy link
Contributor

@infinisil now that I've found a solution for overlaying overrides in the current system, I'm looking at your proposal. I think I understand what you are proposing. How would this look for haskellPackages?

It seems like if haskellPackages.callPackage, haskellPackages.override, haskellPackages.override.overrides, haskellPackages.extend, and composeExtensions were documented, and that documentation recognized overlays as the new normal, then people could achieve successful overlays. I guess I'm saying that your proposal will give authors more license to avoid telling us what is going on. For example, what really is the specification for haskellPackages.callPackage?

We are in a situation where the creation and evolution of the nix language and nix programming practice has polluted the space in which we define packages with magic attributes that control the process. If we are going to continue down that road, then your solution has merit... but it makes me uncomfortable.

In summary, I think the overlay mechanism is a game-changing achievement for packaging, but nothing will work properly if the authors of magic hooks assume that people can intuit the proper use of several layers of untyped higher order functions without documentation within the nixpkgs manual. As it stands, I think we all need to read the source code to gain a firm understanding. Maybe that is the reality? That would be a solution, but in that case the nixpkgs documentation should link directly to source code and call that out as the specification.

I do appreciate that you are trying to solve the issue override/overlay issue, so please take my comments constructively. A big thumbs up for putting your ideas out and working toward a solution.

@stale
Copy link

stale bot commented Jun 16, 2021

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

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 16, 2021
@aviallon
Copy link
Contributor

aviallon commented Oct 2, 2022

Not stale imho.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 2, 2022
@infinisil
Copy link
Member Author

Note that I started the Nixpkgs Architecture Team which should add some traction to this issue. It is being tracked here

@infinisil infinisil added the architecture Relating to code and API architecture of Nixpkgs label Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug architecture Relating to code and API architecture of Nixpkgs
Projects
None yet
Development

No branches or pull requests

5 participants