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

Add pkgs.overrideWithScope #44196

Open
wants to merge 1 commit into
base: master
from
Open

Add pkgs.overrideWithScope #44196

wants to merge 1 commit into from

Conversation

@nbp
Copy link
Member

@nbp nbp commented Jul 29, 2018

Add pkgs.overrideWithScope. This function is similar to pkgs.override, except that it filters the arguments like callPackage does.

While this might seems like a small addition, this is actually a really really big change for the future of Nixpkgs. This function can help us simplify a lot the manipulation of emacsPackages, pythonPackages, haskellPackages, linuxPackages, ... This can also help us rewrite the bootstrapping done before all-packages and move it within all-packages.nix. and on a simpler note, this will help solving the #43755 as explained in #43828.

The reason which makes this function is a revolution for the implementation of package sets is that it make it simple to ""recursively"" (dependencies of dependencies of dependencies) and conherently override the input of a set of packages with a simple function, which does not appear in future overlay.

To make it clearer, here is a set of overlay:

import <nixpkgs> { overlays = [
  # Add an interpreter and its package set.
  (self: super: 
  let interpreterPackages = withSet: {
      foo = super.lib.callPackageWith withSet ./pkgs/interpreter/foo { /* interpreter libxml */ };
      bar = super.lib.callPackageWith withSet ./pkgs/interpreter/bar { /* interpreter foo */ };
      baz = super.lib.callPackageWith withSet ./pkgs/interpreter/baz { /* interpreter foo bar qt */ };
      qux = super.lib.callPackageWith withSet ./pkgs/interpreter/qux { /* interpreter bar baz */ };
    };
  in
  rec {
    interpreter27 = super.callPackage ./pkgs/interpreter_27.nix {};
    interpreter35 = super.callPackage ./pkgs/interpreter_35.nix {};
    interpreter27Packages = let withSet = self // { interpreter = interpreter27; } // self.interpreter27Packages; in
      super.lib.mapAttrs (n: v: v.overrideWithScope withSet) (interpreterPackages withSet);
    interpreter35Packages = let withSet = self // { interpreter = interpreter35; } // self.interpreter35Packages; in
      super.lib.mapAttrs (n: v: v.overrideWithScope withSet) (interpreterPackages withSet);
  })
  # 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; };
    };
  })
  # Duplicate a package set with a custom interpreter.
  (self: super: {
    interpreter27ProfilePackages = let withSet = self // self.interpreter27ProfilePackages; in
      super.lib.mapAttrs (n: v: v.overrideWithScope withSet) (super.interpreter27Packages // {
      interpreter = super.interpreter27.override { withProfiler = true; };
    });
  })
]; }

What is important to note, is that:

  • Changing a package within an existing attribute set, except for the attribute set manipulation, is like any ordinary package.
  • Creating a forked configuration (with a profiler for example), is as simple as changing the interpreter in a new set which is isolated with overrideWithScope.

To make it simple, pkgs.overrideWithScope make it possible to leverage the Nixpkgs fix-point without the burden of writing too many pkgs.override by hand (which need to have no-more than the expected set of arguments).

As it leverage the Nixpkgs fix-point, there is no need to unwrap the function to override packages within a given set, like is done with package sets today (Haskell being infamous due to the stacking of multiple fix-points).

Ideally, we would want to have the pkgs.overrideWithScope evaluated before the resolution of the arguments of a given package, which would not be possible until we add some post-processing of all packages at the end of Nixpkgs fix-point. This enforces that we have to provide the withSet argument to the interpreter set, and implies that we cannot have a raw set of packages which lives independently of a given interpreter version. (Note, one could cheat by providing a fake version of missing packages interpreter = null; libxml = null; qt = null; )

Another issue, which already exists but is made explicit here, is that self // self.interpreterPackages is inefficient in terms of memory because attribute sets are not lazy. This does not worry me much as this is a fixable issue in Nixpkgs, as long as the expression is strictly used as argument of callPackagesWith and overrideWithScope

Other suggestions:

  • Renaming overrideWithScope to overrideWith.
  • Replacing override by overrideWithScope.
…macs, haskell, …) package sets.
@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jul 29, 2018

I'm not quite sure about this idiom. Say we had this function:

let
  # injectOverride
  # :  (name: String)
  # -> ({ a } -> { a } -> { a })
  # -> ({ name: {a}; ..b } -> { name: {a}; ..b } -> { name: {a}; ..b })
  injectOverride = name: f: self: super: { 
    "${name}" = super.${name} // f self.${name} super.${self} // {
      __overlay__ = lib.composeExtensions (super.__inner__ or (_: _: {})) f;
    };
  }; 

Then we can just do:

import <nixpkgs> { overlays = [
  # Add an interpreter and its package set.
  (self: super: 
  let 
    pkgs = self;
    interpreterPackages = self: {
      callPackage = pkgs.newScope self;
      foo = self.callPackage ./pkgs/interpreter/foo { /* interpreter libxml */ };
      bar = self.callPackage ./pkgs/interpreter/bar { /* interpreter foo */ };
      baz = self.callPackage ./pkgs/interpreter/baz { /* interpreter foo bar qt */ };
      qux = self.callPackage ./pkgs/interpreter/qux { /* interpreter bar baz */ };
    };
  in
  rec {
    interpreter27 = self.callPackage ./pkgs/interpreter_27.nix {};
    interpreter35 = self.callPackage ./pkgs/interpreter_35.nix {};
  } 
  // injectOverride "interpreter27Packages"
       (lib.extend (_: _: { interpreter = interpreter27; }) interpreterPackages)
  // injectOverride "interpreter35Packages"
       (lib.extend (_: _: { interpreter = interpreter35; }) interpreterPackages))
  # Change a package dependency. Like any ordinary package, as opposed as today.
  (injectOverride "interpreter27Packages" {
      # Will change foo within bar, baz, qux (as expected).
      foo = super.interpreter27Packages.foo.override { libxml = null; };
  })
  # Duplicate a package set with a custom interpreter.
  (self: super: injectOverride "interpreter27ProfilePackages"
    super.interpreter27Packages.__overlay__ self super)
]; }
@nbp
Copy link
Member Author

@nbp nbp commented Jul 30, 2018

@Ericson2314
The problem with the idiom you are suggesting is that users of a given sub-set of packages will have to know about the injectOverride function, and know when to use it. As opposed to overrideWithScope which would only be required at the creation/cloning of the sub-set.

(Also this should always be super.callPackage)

Honestly, if post-processing of Nixpkgs was an option, and every package would use callPackage (which is far from being the case), then we could make the callPackage function return a functor like object which is derived into a derivation only after going through the Nixpkgs fix-point, and thus we could go with just the following:

import <nixpkgs> { overlays = [
  # Add an interpreter and its package set.
  (self: super: 
  let interpreterPackages = {
      foo = super.callPackage ./pkgs/interpreter/foo { /* interpreter libxml */ };
      bar = super.callPackage ./pkgs/interpreter/bar { /* interpreter foo */ };
      baz = super.callPackage ./pkgs/interpreter/baz { /* interpreter foo bar qt */ };
      qux = super.callPackage ./pkgs/interpreter/qux { /* interpreter bar baz */ };
    };
  in
  rec {
    interpreter27 = super.callPackage ./pkgs/interpreter_27.nix {};
    interpreter35 = super.callPackage ./pkgs/interpreter_35.nix {};
    interpreter27Packages = { interpreter = interpreter27; } // interpreterPackages;
    interpreter35Packages = { interpreter = interpreter35; } // interpreterPackages;
  })
  # 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; };
    };
  })
  # Duplicate a package set with a custom interpreter.
  (self: super: {
    interpreter27ProfilePackages = super.interpreter27Packages // {
      interpreter = super.interpreter27.override { withProfiler = true; };
    });
  })
]; }

As the scope would be determine by the location of the package within Nixpkgs. But this future is not yet there, because not every package uses callPackage, and not every dependency comes from self.

@aszlig
aszlig approved these changes Aug 1, 2018
let
ff = f origArgs;
overrideWith = newArgs: origArgs // (if lib.isFunction newArgs then newArgs origArgs else newArgs);
intersectArgs = if fnArgs != {} then builtins.intersectAttrs fnArgs else (a: a);
overrideWithScope = newScope: overrideWith (intersectArgs newScope);

This comment has been minimized.

@aszlig

aszlig Aug 1, 2018
Member

Very cool, now I can finally get rid of a bit of churn in some of my Nix expressions, thanks :-)

One minor nitpick however:

If my assumption is correct that newScope should always be an attribute set (intersectAttrs would bail out if not), there is no need to go through isFunction in overrideWith above again, so removing intersectArgs and changing overrideWithScope to the following would avoid that:

newScope: origArgs // (if fnArgs != {} then builtins.intersectAttrs fnArgs newScope else newScope)
/* `makeOverridable` takes a function from attribute set to attribute set, a
list of expected arguments and injects `override`, `overrideWithScope` and
`overrideDerivation` attibutes in the result of the function which can be
used to re-call the function with an overrided set of arguments.

This comment has been minimized.

@aszlig

aszlig Aug 1, 2018
Member

Also, just for the sake of completeness (this nitpick is so minor that it isn't even one), maybe add overrideWithScope in the output for the nix repl examples below.

@oxij
Copy link
Contributor

@oxij oxij commented Aug 1, 2018

@twhitehead
Copy link
Contributor

@twhitehead twhitehead commented Aug 9, 2018

For a bit now, I've been thinking it would be nice if the callPackage mechanism could add an override function that would do the equivalent of adding another overlay and then re-evaluate the package.

The idea would be then be that you could also effect changes in all the dependencies of a package as well. Typical examples would be where the dependent packages must also be changed or else you windup with the final executable sucking in two different versions of the same shared library (you see this in some existing expressions with the version of boost or cuda libraries used in its dependencies).

I originally thought this might be what this pull request was, but, upon closer looking, I don't believe so.
I thought I would mention it here though as it does seem there is some significant overlap.

Thanks! -Tyson

PS: Technically, I expect, it could make sense to only apply the overlay to runtime dependencies (i.e., not to the build time ones).

@timokau
Copy link
Member

@timokau timokau commented Nov 1, 2018

Can we move forward with this?

@Ericson2314 Ericson2314 mentioned this pull request Nov 29, 2018
0 of 10 tasks complete
@tomberek
Copy link
Contributor

@tomberek tomberek commented Dec 5, 2018

Does this need review? testers? redesign?

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Dec 5, 2018

I now think #51213 is a better approach than this and my first comment's suggestion.

@danbst
Copy link
Contributor

@danbst danbst commented Jan 18, 2019

In #54266 I make it even simpler to extend existing package sets, so instead of

  (self: super: {
    interpreter27Packages = super.interpreter27Packages // {
      # Will change foo within bar, baz, qux (as expected).
      foo = super.interpreter27Packages.foo.override { libxml = null; };
    };
  })

you can write

  (self: super: { _merge_interpreter27Packages = true; })
  (self: super: {
    interpreter27Packages.foo = super.interpreter27Packages.foo.override { libxml = null; };
  })
danbst added a commit to danbst/nixpkgs that referenced this pull request Jan 19, 2019
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 danbst mentioned this pull request Jan 19, 2019
3 of 10 tasks complete
danbst added a commit to danbst/nixpkgs that referenced this pull request Jan 20, 2019
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
Copy link
Contributor

@danbst danbst commented Feb 21, 2019

@tomberek as I understand it, there are 2 problems here:

  1. Increased memory usage during evaluation (hm, how much?)
  2. People will start using it, so in case Nixpkgs later decides that @Ericson2314 approach is better, it will be harder to deprecate this

But I'd like to merge this as is. It helps a bit with overrides in overlays.

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Aug 16, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/wip-rfc-a-new-nixpkgs-frontend-for-language-infrastructures/3447/9

@timokau timokau mentioned this pull request Aug 25, 2019
0 of 9 tasks complete
@FRidh
Copy link
Member

@FRidh FRidh commented Sep 17, 2019

A year later and I had another look at this, because I need a way to override the Nix packages set from within itself.

@nbp, @Ericson2314 and I had a brief discussion last year on IRC as well about this PR
https://logs.nix.samueldr.com/nixos-dev/2018-07-31#1433188;

The main issue I still have is not having a good way to refer to packages in the main set pkgs. We need a solution there. @nbp proposed renaming e.g. these packages by prepending _ or suffixing '.

@FRidh FRidh added this to the 20.03 milestone Sep 17, 2019
@disassembler disassembler modified the milestones: 20.03, 20.09 Feb 5, 2020
@stale
Copy link

@stale stale bot commented Aug 3, 2020

Hello, I'm a bot and I thank you in the name of the community for your contributions.

Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do:

If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list.

If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past.

If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments.

Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.