top-level: simplify stdenv calling of top-level: fewer inferred and re-inferring arguments #15043

Ericson2314 · 2016-04-27T20:07:18Z

New summery

Ok, this is last general purpose cleanup from me before going into the nitty-gritty details of cross compiling. The basic issue is despite my changes so far almost but not quite break the top-level stdenv cycle. This breaks the cycle, and cuts down on the partial application magic that can make it hard to figure out where an argument comes from.

This PR has been tested with my usual gist

nix-instantiate --eval -E 'import (builtins.fetchTarball https://gist.github.com/Ericson2314/fc745290db68b20235bdc457961b3658/archive/master.tar.gz) {}' -A testsPass

now extended to also check ghc's linux and darwin hashes in addition to cross + custom stdenv stuff. It should also pass travis--I ran the nox script locally.

In the process of cleaning up the default args used for testing, I added a builtins trace as they should never be triggered in production.

Original Post

This grew out of the, ahem, tamer of my ideas in #10874. The basic idea is that top-level/default.nix is now only responsible for building a single bootstrapping phase, and since stdenvs control the bootstrapping, they are solely responsible for calling top-level/default.nix (well, with the exception of the final phase, since that isn't composed in a stdenv-specific manner). Additionally config's replaceStdenv and the cross stdenv are now composed more like other stdenvs than the ad-hoc way they were before.

While this doesn't really solve any problem on its own, it should lay a better foundation for feature work. See the comments in #14965 for how the phasing for the cross stdenv helps for example. Also, some of the more ambitious proposals of #10874 should similarly be assisted.

I tested (a few arbitrary) package paths stayed the same on Nixos. I verified the same for cross-compiling before I rebased onto the newly-bumped nixos-unstable, but post-rebase, cross-compiling seems to be broken for reasons unrelated to this.

The biggest problem with this is now the pkg hierarchy makes less sense: top-level isn't really the top-level and theirs a few more wrapper files. I'd gladly move things around (phase-top-level?) but not sure what the best way forward is.

I view this the spiritual successor of #14000, and as such, I tried to mimic @nbp's method of breaking this up into little commits to make it easier to review.

CC @vcunat @Mathnerd314 @nbp

[PS: @nbp while you weren't involved in the #10874 discussion, this (hopefully just syntactically) conflicts a fair amount with @10874 hence the CC.]

mention-bot · 2016-04-27T20:07:29Z

By analyzing the blame information on this pull request, we identified @nbp, @edolstra and @gridaphobe to be potential reviewers

domenkozar · 2016-04-28T09:46:43Z

If we keep improving top-level functions (which I fully support), we should also document the flow so that we can reason about the improvements.

It's really hard to evaluate and follow the logic without spending considerable amount of time on it.

Ericson2314 · 2016-04-28T14:36:32Z

Yeah I fully agree. I tried to add some documentation as I went along, but it would be good to move things into saner locations and add more.

Mathnerd314 · 2016-04-28T21:16:44Z

maintainers/scripts/dep-licenses.sh

-  defaultStdenv = allStdenvs.stdenv;
-+  defaultStdenv = traceDrvLicenses allStdenvs.stdenv;
+-  defaultStdenv = vanillaStdenv;
+  defaultStdenv = traceDrvLicenses avanillaStdenv;
 "


Could this script be changed to use nix-instantiate -E? Then this message would not be necessary.
Also, @nbp, is this script still useful?

Technically, I didn't even fix the error message to be a real diff. I was mainly interested in making sure my allStdenvs grep came clean before I removed it :).

Mathnerd314 · 2016-04-28T21:37:15Z

As far as the locations, I think pkgs/default.nix should be merged back into pkgs/top-level/default.nix (pure default arguments are usually helpful), and then pkgs/impure.nix can be moved to top-level as well, or else merged into the default.nix in the root directory (I can't say top-level, because that would be confusing).

Mathnerd314 · 2016-04-28T22:08:19Z

pkgs/top-level/default.nix

@@ -19,83 +25,25 @@
               && system != "x86_64-solaris"
               && system != "x86_64-kfreebsd-gnu")



noSysDirs should be in stdenv (true in stdenvNative and false elsewhere), rather than being a top-level argument.

I like that idea, but I see it less as something that should have been done already than something that's good to do in addition---I haven't messed with any parameters that actual packages receive yet.

That said, I'm fine just making it an additional commit that's part of this rather than a separate PR.

I have a similar idea for crossSystem. Now that crossStdenv is properly staged, we can give each stdenv a host and target system+platform, and set them accordingly in the cross stages.

crossStdenv would remain as a entry-point argument, but all-packages.nix would not have that argument as just look at stdenv.host and stdenv.target.

But that, if not the noSysDirs change, will probably be more contentious and deserving of its own PR.

Ericson2314 · 2016-04-29T02:41:48Z

@Mathnerd314 pkgs/default.nix composes the whole thing, while pkgs/top-level/default.nix composes just one stage --- its not just a matter of default arguments.

How about (edit wrote wrong things):

pkgs/top-level/default.nix -> pkgs/top-level/stage.nix
pkgs/default.nix -> pkgs/top-level/default.nix

which, like yours, keeps the "entry point" in the same location.

I do like the idea of just sticking the impure stuff in /defaut.nix :)

Mathnerd314 · 2016-04-29T05:59:34Z

pkgs/default.nix composes the whole thing, while pkgs/top-level/default.nix composes just one stage --- it's not just a matter of default arguments.

I see, but that's still not a barrier to merging them. I did so here (full diff), along with my other suggested changes.

Ericson2314 · 2016-04-29T06:17:58Z

I like it! Part of me is still partial to a default.nix phase.nix separation, but it hardly matters. Note that @nbp purposely kept the throw-away-everything semantics for pkgsWithOverrides rather than using __unfix__ for backwards compat, but I'm not at all sure that's preserving a feature rather than a bug. Thanks for taking a good look at this!

Ericson2314 · 2016-05-02T05:41:14Z

Anybody else else want to review this (and @Mathnerd314's additions) soon? In some cases I imagine it might make sense to interleave/squash our commits---let me know where and I'll be happy to do it.

zimbatm · 2016-05-09T10:39:28Z

default.nix

@@ -6,4 +6,4 @@ if ! builtins ? nixVersion || builtins.compareVersions requiredVersion builtins.

 else

-  import ./pkgs/top-level
+  import ./pkgs/impure.nix


~~I'm not sure about the location and name of this-one but don't have a better recommendation at the moment. Actually it should probably stay in the top-level folder.~~ After reading the rest of the diff is actually makes more sense to be there.

zimbatm · 2016-05-09T13:35:01Z

Looking like a good cleanup

Ericson2314 · 2016-06-15T22:54:34Z

Hello reviewers! :)

pkgs/top-level/default.nix -> pkgs/top-level/stage.nix

pkgs/default.nix -> pkgs/top-level/default.nix

I rebased on top of latest channels/nixpkgs-unstable, did that rename I proposed, and added @Mathnerd314's commits to make it easier to review everything.

zimbatm · 2016-06-15T23:09:38Z

I always wondered, why is that folder called top-level when it's not the top level? Having all the packages listed in pkgs/default.nix would mean less typing for me. And all the <lang>-packages.nix could go in pkgs/development/<lang>-modules/default.nix. Especially the python one who just references tons of patches from that folder anyways.

Ericson2314 · 2016-06-16T00:07:08Z

@zimbatm I agree. I tried to conservatively keep things in the same place for now. But nixpkgs is overdue for a big sorting.

bjornfor · 2016-06-16T04:46:41Z

pkgs/top-level/default.nix

-  topLevelArguments = {
-    inherit system bootStdenv noSysDirs config crossSystem platform lib;
-  };
+  # A few packages make make a new package set to draw their dependencies from.


"make make" => "make"

Fixed, thanks!

zimbatm · 2016-06-16T09:20:46Z

@Ericson2314 agreed not in this PR. Speaking of which, is it ready to be merged?

Ericson2314 · 2016-06-16T19:46:29Z

@zimbatm Well, the first n commits are ready, the question is what n is :). I am quite confident that everything through my penultimate commit is a good idea. But I'm a bit unsure whether the last commit makes things more or less convoluted. And I am still triaging @Mathnerd314's commits: the guaranteed good ones I should probably rebase on top of my guaranteed good ones.

Ericson2314 · 2016-06-16T19:48:26Z

Some decisions like "should impure.nix be part of ./default.nix" don't affect complexity one way or the other and our purely stylistic. Whoever has authority to merge this should comment on those (I am guessing @edolstra, as this lies at the heart of nixpkgs).

zimbatm · 2016-06-17T09:49:17Z

default.nix

+            let
+              toPath = builtins.toPath;
+              getEnv = x: if builtins ? getEnv then builtins.getEnv x else "";
+              pathExists = name:


toPath, getEnv and pathExists are available since nix 0.10

Fixed, thanks!

Also see bec28d7 in master

…vel/stage.nix` This is preparation for the latter just building a single stage, and the former building a package set with the bootstrapped stdenv.

This commit changes the dependencies of stdenv, and clean-up the stdenv story by removing the `defaultStdenv` attribute as well as the `bootStdenv` parameter. Before, the final bootstrapping stage's stdenv was provided by all-packages, which was iterating multiple times over the top-level/default.nix expression, and non-final bootstrapping stages' stdenvs were explicitly specified with the `bootStdenv` parameter. Now, all stages' stdenvs are specified with the `stdenv` parameter. For non-final bootstrapping stages, this is a small change---basically just rename the parameter. For the final stage, top-level/default.nix takes the chosen stdenv and makes the final stage with it. `allPackages` is used to make all bootstrapping stages, final and non-final alike. It's basically the expression of `stage.nix` (along with a few partially-applied default arguments) Note, the make-bootstrap-tools scripts are temporarily broken

This makes the flow of data easier to understand. There's little downside because the args in question are already inspected by the stdenvs. cross-compiling in particular is simpler because we don't need to worry about overriding the config closed over by `allPackages`.

Ericson2314 · 2016-12-01T00:31:09Z

@nbp Do you have time to re-review? Basically I went the opposite course of stripping the all the debugging hand-holds. Differences are roughly:

No more default arguments for debugging
crossSystem no longer defaults to null per stage and must be explicitly provided (but this is analogous to system and platform)
The first commit now also makes pkgs/stdenv/default.nix just the default rather than an attrset of them all (the others were already not exposed by that commit)
top-level/default.nix can take a custom stdenv now---one could already override it with config but same goes for platform and that's manually specifiable too.

If you want to diff, github.com/Ericson2314/nixpkgs/tree/cross-debug is the old version, and cross-* are some other ideas I tossed around. (github's comparing of diverged branches is unfortunately totally broken, so you'd have to use the CLI.0

This mirrors stdenv/default.nix

…ult arguments

- Non-cross stdenvs are honest and assert that `crossSystem` is null - `crossSystem` is a mandatory argument to top-level/stage.nix, just like `system` and `platform` - Broken default arguments on stdenvs for testing are gone. - All stdenvs (but little-used stdenvNix) take the same arguments for easy testing.

- The darwin test can now force the use of the freshly-booted darwin stdenv - The linux test now passes enough dummy arguments This may make debugging harder, if so, check out NixOS#20889

Ericson2314 · 2016-12-04T01:27:51Z

OK, so its been many months. @nbp told me he doesn't have time to review again for a while, but this is now exactly in the spirit of his last review.

There was only one part of his review I had reservations about, pertaining to my last commit. While removing the debugging aids (default arguments never used normally) does reduce complexity, it also may make debugging harder, and stdenvs are already pain enough. To alleviate that, I rebased my old final commit on top of this and opened #20889 with it. I really want to see this merged, so my hope is that other PR can be merged by anyone who is adversely impacted by the removal of debugging aids in this.

So, my plan is to wait a day or two and then merge this myself. Speak now, or ~~forever hold your peace~~ merge #20889.

Ericson2314 force-pushed the cross branch 2 times, most recently from 83de0e1 to a064847 Compare April 27, 2016 20:22

Mathnerd314 reviewed Apr 28, 2016
View reviewed changes

Ericson2314 force-pushed the cross branch from a064847 to 081ceae Compare April 29, 2016 02:36

Ericson2314 changed the title ~~WIP: stdenv calls top-level, as opposed to both calling each other~~ [Semi-WIP] stdenv calls top-level, as opposed to both calling each other May 5, 2016

zimbatm reviewed May 9, 2016
View reviewed changes

Ericson2314 force-pushed the cross branch from 081ceae to 4371fbe Compare June 15, 2016 22:51

bjornfor reviewed Jun 16, 2016
View reviewed changes

zimbatm reviewed Jun 17, 2016
View reviewed changes

Ericson2314 force-pushed the cross branch 3 times, most recently from f5c3716 to e870a4a Compare June 19, 2016 07:27

Ericson2314 force-pushed the cross branch 7 times, most recently from 20f2911 to a17fdb2 Compare November 30, 2016 23:55

Ericson2314 and others added 5 commits November 30, 2016 19:03

top-level: Stop exposing all stdenvs

ddeb0d2

Move up inherit binding for consistency

2df9909

top-level: Split some of pkgs/top-level/default.nix to `pkgs/top-le…

07a2b17

…vel/stage.nix` This is preparation for the latter just building a single stage, and the former building a package set with the bootstrapped stdenv.

Ericson2314 force-pushed the cross branch from a17fdb2 to 5acef01 Compare December 1, 2016 00:12

Ericson2314 assigned Ericson2314 and nbp Dec 1, 2016

Ericson2314 added this to the 17.03 milestone Dec 1, 2016

Ericson2314 and others added 3 commits December 1, 2016 11:24

top-level: Move default-choosing logic to top-level/platforms.nix

05c12f1

This mirrors stdenv/default.nix

top-level: Document why nixpkgsFun doesn't close over inferred defa…

a55d1ec

…ult arguments

Ericson2314 force-pushed the cross branch 2 times, most recently from ffd1f8d to e23afa6 Compare December 1, 2016 22:33

Ericson2314 mentioned this pull request Dec 4, 2016

[Do not merge*] Restore stdenv debugging at the cost of more complexity #20889

Closed

top-level: Allow manually specifying a stdenv, and fix stdenv tests

5c6234a

- The darwin test can now force the use of the freshly-booted darwin stdenv - The linux test now passes enough dummy arguments This may make debugging harder, if so, check out NixOS#20889

Ericson2314 force-pushed the cross branch from e23afa6 to 5c6234a Compare December 4, 2016 01:21

Ericson2314 merged commit e2ff3f1 into NixOS:master Dec 5, 2016

Ericson2314 deleted the cross branch December 5, 2016 16:01

Ericson2314 mentioned this pull request Dec 7, 2016

Systematize the error-prone stdenv bootstrapping dance #11486

Open

Ericson2314 mentioned this pull request Dec 16, 2016

Separate truely top-level code from per-bootstrapping-stage code #21209

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

top-level: simplify stdenv calling of top-level: fewer inferred and re-inferring arguments #15043

top-level: simplify stdenv calling of top-level: fewer inferred and re-inferring arguments #15043

Ericson2314 commented Apr 27, 2016 •

edited

mention-bot commented Apr 27, 2016

domenkozar commented Apr 28, 2016

Ericson2314 commented Apr 28, 2016

Mathnerd314 Apr 28, 2016

Ericson2314 Apr 29, 2016

Mathnerd314 commented Apr 28, 2016

Mathnerd314 Apr 28, 2016

Ericson2314 Apr 29, 2016

Ericson2314 Apr 29, 2016

Ericson2314 Apr 29, 2016

Ericson2314 commented Apr 29, 2016 •

edited

Mathnerd314 commented Apr 29, 2016

Ericson2314 commented Apr 29, 2016

Ericson2314 commented May 2, 2016

zimbatm May 9, 2016 •

edited

zimbatm commented May 9, 2016

Ericson2314 commented Jun 15, 2016

zimbatm commented Jun 15, 2016 •

edited

Ericson2314 commented Jun 16, 2016

bjornfor Jun 16, 2016

Ericson2314 Jun 19, 2016

zimbatm commented Jun 16, 2016

Ericson2314 commented Jun 16, 2016

Ericson2314 commented Jun 16, 2016

zimbatm Jun 17, 2016 •

edited

Ericson2314 Jun 19, 2016

zimbatm Jun 19, 2016

Ericson2314 commented Dec 1, 2016

Ericson2314 commented Dec 4, 2016 •

edited

		@@ -19,83 +25,25 @@
		&& system != "x86_64-solaris"
		&& system != "x86_64-kfreebsd-gnu")

top-level: simplify stdenv calling of top-level: fewer inferred and re-inferring arguments #15043

top-level: simplify stdenv calling of top-level: fewer inferred and re-inferring arguments #15043

Conversation

Ericson2314 commented Apr 27, 2016 • edited

New summery

Original Post

mention-bot commented Apr 27, 2016

domenkozar commented Apr 28, 2016

Ericson2314 commented Apr 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mathnerd314 commented Apr 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Apr 29, 2016 • edited

Mathnerd314 commented Apr 29, 2016

Ericson2314 commented Apr 29, 2016

Ericson2314 commented May 2, 2016

zimbatm May 9, 2016 • edited

Choose a reason for hiding this comment

zimbatm commented May 9, 2016

Ericson2314 commented Jun 15, 2016

zimbatm commented Jun 15, 2016 • edited

Ericson2314 commented Jun 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimbatm commented Jun 16, 2016

Ericson2314 commented Jun 16, 2016

Ericson2314 commented Jun 16, 2016

zimbatm Jun 17, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Dec 1, 2016

Ericson2314 commented Dec 4, 2016 • edited

Ericson2314 commented Apr 27, 2016 •

edited

Ericson2314 commented Apr 29, 2016 •

edited

zimbatm May 9, 2016 •

edited

zimbatm commented Jun 15, 2016 •

edited

zimbatm Jun 17, 2016 •

edited

Ericson2314 commented Dec 4, 2016 •

edited