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

pkgs.extend for adding overlays #47430

Merged
merged 4 commits into from Oct 24, 2018
Merged

Conversation

roberth
Copy link
Member

@roberth roberth commented Sep 27, 2018

Motivation for this change

This is something that I have found useful in a test.
For example, you have one pkgs at your disposal and you want to make sure that it gets passed around correctly by some code. So you add an overlay and run the thing in a way that depends on the extra overlay. Now you know it gets passed around properly.

In practice I recommend that people call Nixpkgs once for performance and
simplicity. The inline documentation mentions this too.
That doesn't mean that pkgs.extend shouldn't be an option for those who need it.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@roberth
Copy link
Member Author

roberth commented Sep 27, 2018

@Ericson2314 you added a comment in the code that indicates nixpkgsFun should be avoided. Is this a good replacement?

@roberth roberth mentioned this pull request Oct 2, 2018
9 tasks
@roberth roberth changed the title Add Nixpkgs functions for adding overlays ad-hoc pkgs.extend for adding overlays Oct 11, 2018
Copy link
Member

@lukateras lukateras left a comment

Choose a reason for hiding this comment

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

Amazing!

@lukateras
Copy link
Member

lukateras commented Oct 13, 2018

Would like to merge this in a few days. cc @nbp

nbp
nbp previously requested changes Oct 15, 2018
Copy link
Member

@nbp nbp left a comment

Choose a reason for hiding this comment

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

This patch sounds good!
Please provide Nixpkgs documentation.

pkgs/top-level/stage.nix Show resolved Hide resolved
@nbp
Copy link
Member

nbp commented Oct 15, 2018

By Nixpkgs documentation, I mean the documentation which appears on https://nixos.org/ .

This is something that I have found useful in tests. In practice
I recommend that people call Nixpkgs once for performance and
simplicity. The inline documentation mentions this too.
Copy link
Member

@nbp nbp left a comment

Choose a reason for hiding this comment

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

Thanks, for the Nixpkgs documentation, however I think some paragraph should be simplified and reordered as described below.

These overriding functions let you focus on one part of Nixpkgs and give you
back the requested variation. This is orthogonal but related to overlays and
the extending functions. Those also let you make modifications but return the
whole package set instead of just what you modified. When used together, the
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph is not cristal clear to me when I read it. I think you meant something along the lines of: "These functions are used to make changes to a single package, while overlays can combine these changes across the entire set of packages of Nixpkgs"

Also, consider adding a link to the overlay section within the documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I have improved the writing and added a link.

doc/overlays.xml Outdated
<literal>import &lt;nixpkgs> { overlays = [ overlay1 overlay2 ];
}</literal>.
</para>
<para>
Further overlays can be added by calling the
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think this deserve its own listitem, instead of being a note on one way to install overlays.

Copy link
Member Author

Choose a reason for hiding this comment

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

A listitem didn't work, because the list was about the way nixpkgs looks up its overlays, so I have shuffled some things around to make it fit.

@Ericson2314
Copy link
Member

Ericson2314 commented Oct 18, 2018

Can we do like

stage-fixpoint // {
  extend = ...;
  appendOverlays = ...;
}

so packages in nixpkgs cannot refer to this?

@Ericson2314
Copy link
Member

Ericson2314 commented Oct 18, 2018

Only other concern is for cross compilation, this cannot be used to change build tools, but maybe that's a feature not a bug :).

Also, sorry for taking so long to review to this.

@roberth
Copy link
Member Author

roberth commented Oct 19, 2018

@Ericson2314 no problem, thanks for reviewing :)

Can we do like

stage-fixpoint // {
  extend = ...;
  appendOverlays = ...;
}

so packages in nixpkgs cannot refer to this?

I recognize your concern about this feature. However, doing this would make pkgs.extend harder to use. For example, pkgs.pkgs.extend will not exist, breaking the pkgs passed by callPackage. It will also break (pkgs.extend f).extend g, and it will break my motivating use case, which is a test in Nixpkgs that checks whether pkgs is passed around properly.

Besides, if pkgs.extend doesn't 'just work', it could steer users to solutions that are worse, like calling Nixpkgs again but forgetting to set the system or config argument.

If it really is a problem, I suggest disabling it for Hydra only. You can use extend to kill itself:

nix-repl> (pkgs.extend (self: super: { extend = abort "Don't use pkgs.extend inside Nixpkgs"; })).extend
error: evaluation aborted with the following error message: 'Don't use pkgs.extend inside Nixpkgs'

However, I do not recommend disabling it.

Only other concern is for cross compilation, this cannot be used to change build tools, but maybe that's a feature not a bug :).

Then isn't that a general characteristic of overlays?

Copy link
Member

@nbp nbp left a comment

Choose a reason for hiding this comment

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

These patches looks good to me as-is. Thanks!

@Ericson2314
Copy link
Member

@roberth well argued. Then instead, could you take a look at the config thing used by ofborg that makes nixpkgs tested without aliases, and try to immitate it here so ofborg can be made to do the same for extend?

roberth added a commit to roberth/ofborg that referenced this pull request Oct 23, 2018
@roberth
Copy link
Member Author

roberth commented Oct 23, 2018

@Ericson2314 I've made a PR to ofborg. Does this satisfy your request?

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Great, thanks!

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

Successfully merging this pull request may close these issues.

None yet

6 participants