overlays: (partially) recursive merge #54266

danbst · 2019-01-18T11:35:58Z

Motivation for this change

One of the problems newcomers face when working with overlays is
non-expected behavior of overriding packages in package sets. Let's take,
for example, Python. Here's an overlay:

self: super: {
    pythonPackages.my-new-package = super.callPackages ./xxx { };
}

Big supprise is that pythonPackages set is now empty!

$ nix-build -E '(with import ./. { overlays = [
    (self: super: {
        pythonPackages.my-new-package = self.pythonPackages.pip;
    })
]; }; pythonPackages.ipython)'

error: attribute 'ipython' missing, at (string):5:7

It is not empty, but contains only one package. That's because attribute
sets are not merged recursively (like NixOS modules do), but are replaced
on each extends. Recursive merge is tricky, because it has to force
evaluation in lot of places and breaks the knot.

But we can do poor-man recursive merge "whitelist". Say, when overlay
contains attribute _merge_pythonPackages = true;, then we can merge it
recursively:

$ nix-build -E '(with import ./. { overlays = [
    (self: super: {
        _merge_pythonPackages = true;
        pythonPackages.my-new-package = self.pythonPackages.pip;
    })
]; }; pythonPackages.ipython)'

/nix/store/k4wf0244qq8xcml85n45sdwalizy0i6k-python2.7-ipython-5.8.0

We can also support this attribute in left-hand side overlay (for
example, all-packages.nix):

  ...
  _merge_pythonPackages = true;
  pythonPackages = python.pkgs;
  _merge_python2Packages = true;
  python2Packages = python2.pkgs;
  _merge_python3Packages = true;
  python3Packages = python3.pkgs;
  ...

So we get merge behavior by default for pythonPackages. We can also
disable merging in our overlay if desired:

$ nix-build -E '(with import ./. { overlays = [
    (self: super: {
        _merge_pythonPackages = false;
        pythonPackages = self.python3Packages;
    })
]; }; pythonPackages.ipython)'

/nix/store/0yx0q6v6nisx4fq1vj8x2vc51n7aj5pz-python3.7-ipython-7.1.1

cc @nbp @Ericson2314

Things done

[x] my system evaluates

One of the problems newcomers face when working with overlays is non-expected behavior of overriding packages in package sets. Let's take, for example, Python. Here's an overlay: ``` self: super: { pythonPackages.my-new-package = super.callPackages ./xxx { }; } ``` Big supprise is that `pythonPackages` set is now empty! ``` $ nix-build -E '(with import ./. { overlays = [ (self: super: { pythonPackages.my-new-package = self.pythonPackages.pip; }) ]; }; pythonPackages.ipython)' error: attribute 'ipython' missing, at (string):5:7 ``` It is not empty, but contains only one package. That's because attribute sets are not merged recursively (like NixOS modules do), but are replaced on each `extends`. Recursive merge is tricky, because it has to force evaluation in lot of places and breaks the knot. But we can do poor-man recursive merge "whitelist". Say, when overlay contains attribute `_merge_pythonPackages = true;`, then we can merge it recursively: ``` $ nix-build -E '(with import ./. { overlays = [ (self: super: { _merge_pythonPackages = true; pythonPackages.my-new-package = self.pythonPackages.pip; }) ]; }; pythonPackages.ipython)' /nix/store/k4wf0244qq8xcml85n45sdwalizy0i6k-python2.7-ipython-5.8.0 ``` We can also support this attribute in left-hand side overlay (for example, `all-packages.nix`): ``` ... _merge_pythonPackages = true; pythonPackages = python.pkgs; _merge_python2Packages = true; python2Packages = python2.pkgs; _merge_python3Packages = true; python3Packages = python3.pkgs; ... ``` So we get merge behavior by default for `pythonPackages`. We can also disable merging in our overlay if desired: ``` $ nix-build -E '(with import ./. { overlays = [ (self: super: { _merge_pythonPackages = false; pythonPackages = self.python3Packages; }) ]; }; pythonPackages.ipython)' /nix/store/0yx0q6v6nisx4fq1vj8x2vc51n7aj5pz-python3.7-ipython-7.1.1 ```

Mic92 · 2019-01-18T11:43:15Z

lib/fixed-points.nix

+    let
+      merge = name: value:
+        if builtins.hasAttr name lhs
+            && (rhs."_merge_${name}" or lhs."_merge_${name}" or false)


Could we have a function instead similar to recurseIntoAttrs instead of this top-level attributes?

but then we have to define all sets in one place (in that function), right? In my implementation it is possible to hijack anything from overlay, without the need to extend function.

But maybe you can show an example of what you expect that "merge-approve" function should look like

I mean something like:

mergeInOverlays = attrs: attrs // { mergeInOverlays = true; }; mergeUpdate = lhs: rhs: # ... # this is incorrect because both the value from `lhs` and `rhs` must be checked, # but the idea should be clear. if (builtins.isAttrs value && value ? mergeInOverlays) # ... python2Packages = mergeInOverlays python2.pkgs;

Unfortunately it is not possible, because it requires evaluate.of all attributes (we want to know which attributes are sets, and which of those have mergeInOverlays sub-attribute).

And some of our attributes rely on tying the knot, so we don't want to evaluate them too soon.

In particular, wrapBintoolsWith evaluation relies on stdenv, which itself relies on binutils, which relies on wrapBintoolsWith... I couldn't make it play.

danbst · 2019-01-18T11:57:36Z

Obviously, this leads to performance degradation, 1% for hello.

FRidh

pythonPackages.my-new-package = super.callPackages ./xxx { };

This isn't the documented way to achieve this. Instead, override the interpreter to pass overrides to the package set. Now, these themselves won't compose, and that was a design mistake. If we want to fix this, we should do it properly, and consider #44196.

danbst · 2019-01-18T12:37:11Z

@FRidh oh, this is another problem, which isn't related to this PR. Probably I made a bad example...

You surely have to create python packages correctly, however this change is about to merge namespaces correctly. For example, it allows creating aliases, overrides and new packages conveniently:

self: super: {
   pythonPackages.django = self.pythonPackages.django_2_0;
   pythonPackages.nanomsg = self.pythonPackages.buildPythonPackage {
        name = "nanomsg-1.0";
        buildInputs = with self.pythonPackages; [ ... ];
        ...
    };
}

Can you show me, in which way this should break?

danbst · 2019-01-18T12:49:50Z

@FRidh another toy example, on how deep this can go:

$ nix-build -E '(with import ./. { overlays = [
    (self: super: {
        _merge_python2 = true;
        python2._merge_pkgs = true;
        python2.pkgs.ipython = self.python3.pkgs.ipython;
    })
]; }; [ python2 python2Packages.ipython ])'
/nix/store/l95nkqp7bdimqnz9ixay1aahljzsz7vc-python-2.7.15
/nix/store/0yx0q6v6nisx4fq1vj8x2vc51n7aj5pz-python3.7-ipython-7.1.1

danbst · 2019-01-18T12:59:35Z

I'll copy also example for gst_* package set:

$ code='with import ./. { overlays = [
    (self: super: {
        _merge_gst_all_1 = true;
        gst_all_1.gst-plugins-bad = super.gst_all_1.gst-plugins-bad.overrideAttrs (old: rec {
            buildInputs = old.buildInputs ++ [ self.rtmpdump ];
        });
    })
]; }'

$ nix-build -E "$code; arravis" # <-- will rebuild arravis with our overriden gst-plugins-bad

Override of arravis works!

danbst · 2019-01-18T14:10:13Z

@FRidh I found a problem with deep overriding python packages, but this was because python packages don't use self to apply any package overrides. Added a quick fix, now this builds vine package with pytest overriden in vine-s buildInputs:

nix-build -E '(with import ./. { overlays = [
    (self: super: {
        _merge_python27 = true;
        python27._merge_pkgs = true;
        #python27.pkgs.pytest = self.python27.pkgs.pytest_39; # default
        python27.pkgs.pytest = self.python27.pkgs.pytest_37;
    })
]; }; [ python27.pkgs.vine ])'

danbst · 2019-01-18T14:36:21Z

OTOH, there is no miracle, and you indeed can't override packages via pythonPackages.

    (self: super: {
        pythonPackages.pkgs.pytest = self.python27.pkgs.pytest_39; # doesn't override nixpkgs
        python2.pkgs.pytest = self.python27.pkgs.pytest_37; # same shit
        python27.pkgs.pytest = self.python27.pkgs.pytest_37; # only this works
    })

Probably it requires some clever alias resolution, but can be done in separate PR.

Ericson2314 · 2019-01-18T15:41:57Z

Did you see I did something similar in #51213 ?

danbst · 2019-01-18T15:51:07Z

@FRidh alright, found one more bug in python packages. Fixed. Now example from #26487 (comment) can be simplified down to:

self: super:
with self.python27.pkgs; {

  _merge_python27 = true;
  python27._merge_pkgs = true;

  # nix-shell -p python.pkgs.my_stuff
  # nix-shell -p 'python.withPackages(ps: [ps.my_stuff])'
  python27.pkgs.my_stuff = buildPythonPackage rec {
    pname = "pyaes";
    version = "1.6.0";
    name = "${pname}-${version}";
    src = fetchPypi {
      inherit pname version;
      sha256 = "0bp9bjqy1n6ij1zb86wz9lqa1dhla8qr1d7w2kxyn7jbj56sbmcw";
    };
  };
}

I think, this is cool now! Never was working with python packages so easy!

danbst · 2019-01-18T15:56:50Z

@Ericson2314 yep. Though this is much simpler and deprecates nothing. I couldn't compare though, because I don't understand how to build darwin pkgs... However, I imagine that following code:

      darwin = super.darwin // {
        inherit (darwin)
          dyld Libsystem xnu configd ICU libdispatch libclosure launchd CF;

will look like

      _merge_darwin = true;
      darwin = {
        inherit (super.darwin)
          dyld Libsystem xnu configd ICU libdispatch libclosure launchd CF;

with my approach. This all requires checks, just as I found python infra doesn't use self-fixpoint in some cases (fixed those).

edolstra · 2019-01-18T16:15:33Z

pkgs/top-level/all-packages.nix

@@ -7996,8 +7996,11 @@ in
  python37Full = python37.override{x11Support=true;};

  # pythonPackages further below, but assigned here because they need to be in sync
+  _merge_pythonPackages = true;


camelCase please.

🤔 this isn't a package, so it shouldn't follow package naming guidelines. This is an attribute lifted out of it's attribute set to prevent evaluations. Maybe pythonPackages_merge_ = true; would look better?

We are more often use camelCase for functions then for packages.

Profpatsch · 2019-01-19T10:36:56Z

Having a magic boolean flag for that looks like a very ugly hack to me, the opposite of a principled solution.

danbst · 2019-01-19T11:03:59Z

@Profpatsch think of it as gradual transition towards ideal world described in #44196 (comment)

I'll copy an example from that PR:

  # Change a package dependency. Like any ordinary package, as opposed as today.
  (self: super: {
    interpreter27Packages = super.interpreter27Packages // {
      # Will change foo within bar, baz, qux (as expected).
      foo = super.interpreter27Packages.foo.override { libxml = null; };
    };
  })

My proposed "ugly boolean hack" would allow to convert this to

  # Will also override foo within bar, baz, qux
  (self: super: with super.interpreter27Packages; {
    interpreter27Packages.foo = foo.override { libxml = null; };
  })

And, just to refresh memory, this is how it's done nowadays:

self: super: rec {
  interpreter27 = super.interpreter27.override {
    # Careful, we're using a different self and super here!
    packageOverrides = self: super: {
      foo = super.foo.override { libxml = null; };
    };
  };
  interpreter27Packages = interpreter27.pkgs;
}

Think of this "ugly boolean flags" as NixOS options, which guide overlays fixpoint on how to converge to final result. They bring one feature from extensible record proposal - merge of sets - to current nixpkgs. This doesn't remove the need for #44196 (comment), however in this PR I've made it possible actually override python dependencies with simple //

We also have discusses with @ottidmes in IRC, that flag should be renamed to xxxxMergeable. This would please @edolstra .

nbp · 2019-01-19T11:51:13Z

Just a few notes on the merging strategy and the naming aspect.

merge is vague as there is not a simple merge strategy as you might have noted in NixOS. I suggest renaming it to recUpdate or recursiveUpdate.
Instead of naming things _merge_<name> = true, use an attribute set which mirror all the other attributes names, such as _merge.<name> = true;. This way this special attribute set can always be considered as being recursively updated, and aggregates all the names which have to be recursively updated as well.
Use recursiveUpdateUntil.

I still have to think on whether I like this approach or not.

Extracts some useful parts of NixOS#38698, in particular, it's vision that postgresql plugins should be namespaced. Original approach had several problems: - not gonna happen in forseeable future - did lots of deprecations - was all-in-one solution, which is hard to sell to nixpkgs - even that we have postgresqlPackages now, we can't do arbitrary overrides to postgresql and plugins. Several required functions were not exported Here I've fixed all of those problems: - deprecates nothing (though plugins were moved now into `aliases.nix`) - this doesn't touch NixOS at all, and doesn't break anything - hashes for plugins and PGs are not changed (I hope) - no extra fixes to pg itself - default PG is not changed - plugins and PGs are extensible Last one is the most interesting thing, because it introduces novel way to manage `XXX withPackages` problem. It is novel, but has got lots of inspiration from existing approaches: - python, so we have now `postgresql.pkgs.*`, `postgresql_11.pkgs.*` which all contain plugins compiled with correct PG. - python, so we have now `postgresql.withPackages` as well - in contrast to python, there are no `postgresql11Packages`, only `postgresql_11.pkgs` - NixOS#44196, so plugins are referenced starting at self-fixpoint. This allows override/add plugins with mere `//` in overlay. This works for both `postgresqlPackages` (overrides are applied to all postgresql_xx.pkgs) and `postgresql_xx.pkgs` (overrides are specific to this postgresql) sets - I've made it compatible with proposed mergeable overlays (NixOS#54266) however this PR doesn't depend on it - last, but not least, `postgresql/default.nix` is now an overlay! This replaces previous `callPackages` approach with a modern, extensible concept.

danbst · 2019-01-26T17:30:43Z

this was a fun idea, but too unpolished

some lang-packages (like Python) don't work out of box with overlays self-fixpoint
the _merge attributes indeed seem to be too ad-hoc

Now I have an idea that each overlay should have an attribute overlayConfig, which is a module (real module in module system), and self.overlayConfig should contain all the meta-information on how to guide overlays:

which attr keys should be merged instead of replaced
which attr keys should be traversed by hydra
which attr keys should be listed in nix-env
and maybe whether attr key is deprecated

But I'm not an expert in these internals...

nixos-discourse · 2020-05-11T17:03:39Z

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

https://discourse.nixos.org/t/makeextensibleasoverlay/7116/3

danbst requested review from edolstra and nbp as code owners January 18, 2019 11:35

Mic92 reviewed Jan 18, 2019

View reviewed changes

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jan 18, 2019

danbst mentioned this pull request Jan 18, 2019

Python's packageOverrides isn't composable #44426

Open

FRidh requested changes Jan 18, 2019

View reviewed changes

danbst mentioned this pull request Jan 18, 2019

gst_all_1: make extensible #49723

Closed

9 tasks

add overriden python packages to fixpoint

47517c0

GrahamcOfBorg added 6.topic: python 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 and removed 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jan 18, 2019

danbst mentioned this pull request Jan 18, 2019

Add pkgs.overrideWithScope #44196

Closed

python: use self in one more place

b09b8c2

This was referenced Jan 18, 2019

Using python.withPackages with nixpkgs-overlays does not work #26487

Closed

adding python package in overlay via callPackage #30267

Closed

edolstra reviewed Jan 18, 2019

View reviewed changes

danbst mentioned this pull request Jan 19, 2019

postgresql: reorganize package and its extensions #54319

Merged

10 tasks

danbst mentioned this pull request Jan 22, 2019

Add repoOverrides nix-community/NUR#101

Closed

2 tasks

danbst closed this Jan 26, 2019

danbst mentioned this pull request Feb 21, 2019

[Demo, RFC] Gentoo-like use-flags #56110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

overlays: (partially) recursive merge #54266

overlays: (partially) recursive merge #54266

danbst commented Jan 18, 2019

Mic92 Jan 18, 2019 •

edited

Loading

danbst Jan 18, 2019

Mic92 Jan 18, 2019 •

edited

Loading

danbst Jan 18, 2019

danbst commented Jan 18, 2019

FRidh left a comment

danbst commented Jan 18, 2019

danbst commented Jan 18, 2019

danbst commented Jan 18, 2019

danbst commented Jan 18, 2019

danbst commented Jan 18, 2019

Ericson2314 commented Jan 18, 2019

danbst commented Jan 18, 2019

danbst commented Jan 18, 2019

edolstra Jan 18, 2019

danbst Jan 18, 2019

Mic92 Jan 19, 2019 •

edited

Loading

Profpatsch commented Jan 19, 2019 •

edited

Loading

danbst commented Jan 19, 2019 •

edited

Loading

nbp commented Jan 19, 2019

danbst commented Jan 26, 2019

nixos-discourse commented May 11, 2020

overlays: (partially) recursive merge #54266

overlays: (partially) recursive merge #54266

Conversation

danbst commented Jan 18, 2019

Motivation for this change

Things done

Mic92 Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

danbst Jan 18, 2019

Choose a reason for hiding this comment

Mic92 Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

danbst Jan 18, 2019

Choose a reason for hiding this comment

danbst commented Jan 18, 2019

FRidh left a comment

Choose a reason for hiding this comment

danbst commented Jan 18, 2019

danbst commented Jan 18, 2019

danbst commented Jan 18, 2019

danbst commented Jan 18, 2019

danbst commented Jan 18, 2019

Ericson2314 commented Jan 18, 2019

danbst commented Jan 18, 2019

danbst commented Jan 18, 2019

edolstra Jan 18, 2019

Choose a reason for hiding this comment

danbst Jan 18, 2019

Choose a reason for hiding this comment

Mic92 Jan 19, 2019 • edited Loading

Choose a reason for hiding this comment

Profpatsch commented Jan 19, 2019 • edited Loading

danbst commented Jan 19, 2019 • edited Loading

nbp commented Jan 19, 2019

danbst commented Jan 26, 2019

nixos-discourse commented May 11, 2020

Mic92 Jan 18, 2019 •

edited

Loading

Mic92 Jan 18, 2019 •

edited

Loading

Mic92 Jan 19, 2019 •

edited

Loading

Profpatsch commented Jan 19, 2019 •

edited

Loading

danbst commented Jan 19, 2019 •

edited

Loading