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.

Sign up for GitHub

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

overlays: (partially) recursive merge #54266

Closed
wants to merge 3 commits into from
Closed

overlays: (partially) recursive merge #54266

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
82c3a17
overlays: (partially) recursive merge
danbst Jan 18, 2019
47517c0
add overriden python packages to fixpoint
danbst Jan 18, 2019
b09b8c2
python: use self in one more place
danbst Jan 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion lib/fixed-points.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,19 @@ rec {
# = self: { foo = "foo"; bar = "bar"; foobar = self.foo + self.bar; } // { foo = "foo" + " + "; }
# = self: { foo = "foo + "; bar = "bar"; foobar = self.foo + self.bar; }
#
extends = f: rattrs: self: let super = rattrs self; in super // f self super;
mergeUpdate = lhs: rhs:
let
merge = name: value:
if builtins.hasAttr name lhs
&& (rhs."_merge_${name}" or lhs."_merge_${name}" or false)
Copy link
Member

@Mic92 Mic92 Jan 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@Mic92 Mic92 Jan 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

then mergeUpdate lhs.${name} value
else value;
in lhs // (builtins.mapAttrs merge rhs);

extends = f: rattrs: self:
let super = rattrs self;
in mergeUpdate super (f self super);
#in super // (f self super); # <-- NON RECURSIVE

# Compose two extending functions of the type expected by 'extends'
# into one where changes made in the first are available in the
Expand Down
3 changes: 3 additions & 0 deletions pkgs/top-level/all-packages.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

camelCase please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 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?

Copy link
Member

@Mic92 Mic92 Jan 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

pythonInterpreters = callPackage ./../development/interpreters/python {};
Expand Down