pkgs.extend for adding overlays by roberth · Pull Request #47430 · NixOS/nixpkgs

roberth · 2018-09-27T16:37:46Z

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 · 2018-09-27T16:38:48Z

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

lukateras

Amazing!

lukateras · 2018-10-13T14:04:01Z

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

nbp

This patch sounds good!
Please provide Nixpkgs documentation.

pkgs/top-level/stage.nix

nbp · 2018-10-15T10:09:02Z

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

Done. Any other improvements?

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.

…verride section

nbp

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

nbp · 2018-10-18T09:47:16Z

doc/functions/overrides.xml

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.

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

nbp · 2018-10-18T09:49:58Z

doc/overlays.xml

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

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 · 2018-10-18T21:25:03Z

Can we do like

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

so packages in nixpkgs cannot refer to this?

Ericson2314 · 2018-10-18T21:25:52Z

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 · 2018-10-19T08:29:48Z

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

nbp

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

Ericson2314 · 2018-10-23T13:49:08Z

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

See NixOS/nixpkgs#47430 (comment)

roberth · 2018-10-23T15:05:53Z

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

Ericson2314

Great, thanks!

roberth requested review from Ericson2314 and nbp as code owners September 27, 2018 16:37

GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Sep 27, 2018

roberth added the 8.has: documentation This PR adds or changes documentation label Oct 2, 2018

roberth mentioned this pull request Oct 2, 2018

Add pkgs.nixosTest #47684

Merged

9 tasks

roberth changed the title ~~Add Nixpkgs functions for adding overlays ad-hoc~~ pkgs.extend for adding overlays Oct 11, 2018

lukateras approved these changes Oct 13, 2018

View reviewed changes

nbp previously requested changes Oct 15, 2018

View reviewed changes

pkgs/top-level/stage.nix Outdated Show resolved Hide resolved

nbp added 9.needs: documentation This needs to be documented well. and removed 8.has: documentation This PR adds or changes documentation labels Oct 15, 2018

roberth force-pushed the nixpkgs-extend branch from 3adfc45 to 6ac1ed1 Compare October 15, 2018 11:50

GrahamcOfBorg added the 8.has: documentation This PR adds or changes documentation label Oct 15, 2018

roberth added 2 commits October 18, 2018 10:48

Add Nixpkgs functions for adding overlays ad-hoc

ba8b54f

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.

nixpkgs docs: Point to pkgs.extend, pkgs.appendOverlays and improve o…

9b7654f

…verride section

nbp suggested changes Oct 18, 2018

View reviewed changes

doc: Improve overrides and overlays documentation

3c2ae18

roberth force-pushed the nixpkgs-extend branch from 6ac1ed1 to 3c2ae18 Compare October 18, 2018 12:01

doc: Add link from overrides to overlays

a5c4642

nbp removed the 9.needs: documentation This needs to be documented well. label Oct 19, 2018

nbp approved these changes Oct 19, 2018

View reviewed changes

roberth added a commit to roberth/ofborg that referenced this pull request Oct 23, 2018

Prohibit use of nixpkgs.extend from inside Nixpkgs

ccfbd03

See NixOS/nixpkgs#47430 (comment)

roberth mentioned this pull request Oct 23, 2018

Prohibit use of nixpkgs.extend from inside Nixpkgs NixOS/ofborg#258

Closed

Ericson2314 approved these changes Oct 24, 2018

View reviewed changes

Ericson2314 merged commit b71389a into NixOS:master Oct 24, 2018

roberth mentioned this pull request Oct 27, 2018

NixOS: use overlays when nixpkgs.pkgs is set #49256

Merged

9 tasks

roberth mentioned this pull request Sep 9, 2021

Fix interaction between appendOverlays and otherPackageSets #136267

Merged

12 tasks

Uh oh!

Conversation

roberth commented Sep 27, 2018

Motivation for this change

Things done

Uh oh!

roberth commented Sep 27, 2018

Uh oh!

lukateras left a comment

Choose a reason for hiding this comment

Uh oh!

lukateras commented Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nbp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nbp commented Oct 15, 2018

Uh oh!

nbp left a comment

Choose a reason for hiding this comment

Uh oh!

nbp Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

roberth Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

nbp Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

roberth Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roberth commented Oct 19, 2018

Uh oh!

nbp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ericson2314 commented Oct 23, 2018

Uh oh!

roberth commented Oct 23, 2018

Uh oh!

Ericson2314 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lukateras commented Oct 13, 2018 •

edited

Loading

Ericson2314 commented Oct 18, 2018 •

edited

Loading

Ericson2314 commented Oct 18, 2018 •

edited

Loading

nbp left a comment •

edited

Loading