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

In lib/trivial/makeExtensible* allow extenders to refer to refixer … #20927

Closed
wants to merge 1 commit into from

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Dec 5, 2016

Motivation for this change

As reported by @FRidh, pkgs.overridePackages no longer works because
that function is added to the attribute set after fixing. Precisely, that
means the include pkgs in allpackages.nix doesn't pick it up with the
rest of the final fixed package set.

This changes changes makeExtensibleWithCustomName, and by extension plain
makeExtensible so that the refixer function is added as a final extension
before fixing.

Things done

Tested manually using nix-repl. Should we create an automatic test for this?

CC @nbp for posterity---I don't expect you to have any time for this.

@mention-bot
Copy link

@Ericson2314, thanks for your PR! By analyzing the history of the files in this pull request, we identified @cstrahan, @peti and @edolstra to be potential reviewers.

@Ericson2314 Ericson2314 assigned Ericson2314, cstrahan and nbp and unassigned nbp Dec 5, 2016
…function

As [reported][1] by @FRidh, `pkgs.overridePackages` no longer works because
that function is added to the attribute set *after* fixing. Precisely, that
means the `include pkgs` in `allpackages.nix` doesn't pick it up with the
rest of the final fixed package set.

This changes changes `makeExtensibleWithCustomName`, and by extension plain
`makeExtensible` so that the refixer function is added as a final extension
before fixing.

[1]: NixOS#19496 (comment)
@FRidh
Copy link
Member

FRidh commented Dec 5, 2016

My comment on the issue #19496 (comment)

@nbp
Copy link
Member

nbp commented Dec 13, 2016

I honestly think we should get rid of pkgs.overridePackages, because the only use of it is to re-evaluate the Nixpkgs fix-point with a different/additional set of packageOverride-like. Currently we have a path attribute inside Nixpkgs, and this path can be used to re-evaluate Nixpkgs the same way.

Otherwise, we should change pkgs.overridePackages to just create a new fix-point instead of re-using the current one.

@FRidh , could you describe your use-case of pkgs.overridePackages?

@Ericson2314
Copy link
Member Author

Currently we have a path attribute inside Nixpkgs, and this path can be used to re-evaluate Nixpkgs the same way.

The crucial difference is that will reinfer a bunch of parameters, while with overridePackages one can modify a nixpkgs without worrying about what arguments were elsewhere provided.

For a concrete usecase, think how big nix blobs like https://github.com/reflex-frp/reflex-platform/blob/develop/default.nix#L1-L19 are currently not very composable, but could be with overridePackages. [Ideally such things wouldn't need to exist, but in practice they often do and pave the way to changes in nixpkgs proper.]

@FRidh
Copy link
Member

FRidh commented Dec 14, 2016

@nbp if I understand correctly packageOverrides can only be used in your ~/.nixpkgs/config.nix. I need to override the package set on a per-project base, e.g. to add additional Python packages that other packages use as well. Most of the time just overriding the Python package set is sufficient, but I think it should be possible to override the top-level set as well.

I've added the following example to the Nixpkgs manual

let
  pkgs = import <nixpkgs> {};
  newpkgs = pkgs.overridePackages ( pkgsself: pkgssuper: {
    python27 = let
      packageOverrides = self: super: {
        numpy = super.numpy_1_10;
      };
    in pkgssuper.python27.override {inherit packageOverrides;};
  } );
in newpkgs.inkscape

@FRidh
Copy link
Member

FRidh commented Dec 14, 2016

Also, I don't know whether it is possible, but there should be a 'straightforward' way to exclude configuration. E.g., if I test something locally it might not work elsewhere because of some setting in ~/.nixpkgs/config.nix. This is out of scope for this issue, but still something to keep in mind.

@nbp
Copy link
Member

nbp commented Dec 15, 2016

if I understand correctly packageOverrides can only be used in your ~/.nixpkgs/config.nix.

True, but you can provide a custom one as argument of import <nixpkgs>:

let
  newpkgs = import <nixpkgs> {
    config = {
      packageOverrides = pkgs: { ... };
    };
  };
in
  newpkgs.inkscape

This example will also exclude the per-user configuration, as the packageOverrides used to do.

@FRidh
Copy link
Member

FRidh commented Dec 16, 2016

@nbp so that then also covers my second comment, right?

Also, I don't know whether it is possible, but there should be a 'straightforward' way to exclude configuration. E.g., if I test something locally it might not work elsewhere because of some setting in ~/.nixpkgs/config.nix. This is out of scope for this issue, but still something to keep in mind.

Anyway, I'm fine using the snippet you show. The docs just need to be updated to include this, because it is quite an important feature I would argue.

@Ericson2314
Copy link
Member Author

That works for 1 customization, but not for the composition-of-customizations usecase I describe above.

@nbp
Copy link
Member

nbp commented Dec 17, 2016

I guess for the composition we could just accept multiple new overlays (as called in gentoo) which are new self: super: phases in the pipeline. Adding these overlays next to the config argument should help at extending nixpkgs is a modular-ish fashion.

I guess we could add both environment variable / list of paths, and load all overlays in the order in which they are listed. Of course, if some argument are provided to nixpkgs, these overlays would take precedence.

This way, we have options to get rid of packageOverrides, and make nixpkgs slightly more modular and customizable by external repositories.

I will prototype this idea tomorrow.

@nbp nbp mentioned this pull request Dec 17, 2016
4 tasks
@cstrahan
Copy link
Contributor

cstrahan commented Jul 11, 2017

@Ericson2314 With this PR, if one were to use

makeExtensible (self: <... some expr involving self.extend ...>)

that would be no different than the current approach of

let topSelf = makeExtensible (self: <... some expr involving topSelf.extend ...>; in <...>

right?

Actually, now that I've thought about it some more, I suppose the difference is that topSelf in the above example captures the fixpoint of a given set of extensions, so topSelf.extend is not identical to (topSelf.extend (self: super: <...>)).extend, and therefore composition breaks down in comparison to feeding extend into the attrs before fixing (as this PR does).

If we were worried about shadowing extend as a potential package name we could, alternatively, expose __fix__ before fixing, so for the (probably rare/esoteric) cases where one might otherwise want to use self.extend, they could instead use self.__fix__ directly (less convenient, but doesn't use up another attr name). The idea being that we'd change fix' to:

# current def:
# fix' = f: let x = f x // { __unfix__ = f; }; in x;

# new def:
fix' = f: let x = f (x // { __unfix__ = f; }) // { __unfix__ = f; }; in x;

EDIT: No need to change fix' -- that's already how it behaves. Need more caffeine.

Another thought: worrying about shadowing extend is a little silly: we're already doing that from the point of view of the person using the package set (e.g. if the package set defines an extend package, no one from outside can make use of it anyway -- though other packages inside can still refer to it). That's pretty inconsistent, and this PR addresses just that.

TL;DR: I'm thinking we should merge this PR. Thoughts, @Ericson2314?

Sorry for the winding, stream of consciousness style prose. I figure if anyone is as sleepy as I am, they too might miss some of the finer details, so I figured I'd leave this comment as is.

@mmahut
Copy link
Member

mmahut commented Aug 1, 2019

Any update on this pull request?

@FRidh
Copy link
Member

FRidh commented Aug 1, 2019

This is no longer important. Since then we have other recommended methods that work fine so I am closing this.

@FRidh FRidh closed this Aug 1, 2019
@FRidh
Copy link
Member

FRidh commented Aug 1, 2019

(note I did not go through @cstrahan 's message. Feel free to reopen if needed)

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

Successfully merging this pull request may close these issues.

None yet

6 participants