Normalize bootstrapping #21415

Ericson2314 · 2016-12-25T21:57:02Z

Split out from #21268

This PR introduces a new abstraction for dealing with stdenv bootstrapping. It doesn't really make anything shorter, but at least it enforces a consistent ideom across all of them. I've made all the stdenvs but darwin really embrace the new abstraction. Darwin is a bit bigger, and so I made minimal changes to make it use it, but I'll refactor it to make it embrace the new abstraction too if wanted.

#21268 builds on this to make cross compiling not a dirty hack.

mention-bot · 2016-12-25T21:57:05Z

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

cstrahan · 2016-12-25T22:44:35Z

Hey @Ericson2314, I'd say this on IRC but it looks like you disconnected: thanks a ton for doing all this refactoring work! The cross compiling stuff has looked a little sketchy for a while, and I'm really looking forward to seeing the wrinkles ironed out. 🍻

Oh, and Happy Holidays!

shlevy · 2016-12-26T14:30:36Z

Bit busy with the holidays, but will review this when I get the chance!

Ericson2314 · 2017-01-07T01:05:02Z

@shlevy any sort of ETA?

@nbp If @shlevy or @cstrahan gives this a go I want to merge and then try the single-fixpoint stuff we scratched at in the other PR's thread.

shlevy · 2017-01-07T15:48:40Z

Review started, will probably take a few days to finish going through it all

Ericson2314 · 2017-01-07T16:02:32Z

@shlevy Thanks so much! Oh and to be clear, I've written it with the intention of a commit-by-commit review, which should make it more bights but each one less of a mouthful.

Ericson2314 · 2017-01-13T03:51:11Z

@shlevy friendly ping :)

shlevy

Can you explain a bit more what this new abstraction is and why its preferable?

EDIT: This review is first commit only.

shlevy · 2016-12-26T14:27:18Z

pkgs/stdenv/booter.nix

+rec {
+  # It would be nice to just use `++`, but we can't forget about the old
+  # `basecase`. I (@Ericson2314) am tempted to use a heterogeneous list instead.
+  extendStages = { baseCase, stageFuns }: more: {


Please document this or, if its use should be obvious from context, move it to a let at the top so it's not exported.

shlevy · 2017-01-07T15:46:41Z

pkgs/stdenv/booter.nix

+    # isn't already set.
+    withAllowCustomOverrides = lib.lists.imap
+      (index: stageFun: prevStage:
+        { allowCustomOverrides = index == 1; } # first element, 1-indexed


This doesn't match the comment ("all but the final stage") if the list has the stages in order...

Oh whoops! Yeah originally I had reverse order.

shlevy

Can't seem to approve commits separately, 👍 on "top-level: Inherit system and platform in stage.nix not all-packages.nix"

shlevy

Review of "top-level: Modernize stdenv.overrides giving it self and super"

Please find some place (perhaps the nixpkgs manual changelog?) to document this change.

Otherwise 👍

shlevy

Review of "top-level: Do stdenvOverrides in stage.nix even if crossSystem exists.":

Expand in commit message why the previous comment no longer applies.

shlevy · 2017-01-13T13:09:00Z

pkgs/top-level/stage.nix

-  #
-  # We don't want stdenv overrides in the case of cross-building, or
-  # otherwise the basic overridden packages will not be built with the
-  # crossStdenv adapter.


Why doesn't this comment apply any more?

In commit message, basically the cross stdenv cleans up after itself rather than leaving stage.nix to do it.

shlevy

Review of "linux stdenv: Utilize overrides and prevStage better":

Why does this change gcc.cc to gcc-unwrapped?

shlevy

Review of "linux stdenv: Remove stray use of stage0 to bootstrap more elegantly"

Can you explain the role of ccWrapperStdenv in this commit?

shlevy

Review of "linux stdenv: Inline stage funs to conform to new convention"

Diff is too hard to verify, but if all this is is code shuffling to meet the new standard then 👍

Ericson2314 · 2017-01-13T14:23:33Z

Thanks!! Will get to work on these right away

Ericson2314 · 2017-01-13T14:35:54Z

I'm going to make those changes, make extendStages actually ++ with @nbp's {} base case in other thread suggestion, and then merge!

shlevy · 2017-01-13T14:37:33Z

Well I'd like to see the explanation for why this new abstraction is better first...

Ericson2314 · 2017-01-13T14:45:22Z

@shlevy Sure, I was including that in this list of things to do, but yeah that deserves a re-review.

Document breaking change in 17.03 release notes

Ericson2314 · 2017-01-13T17:34:04Z

@shlevy should be all done. I can't figure out what the hell is causing the travis error. There is no freebsd variable on the line given in my branch or master.

EDIT: nevermind, a different PR (#21858) since I last fetched. Other PR is causing the problem all by itself. let me deal with that.

Ericson2314 · 2017-01-13T17:36:54Z

As a bonus, the big inline commit is now less indented causing less churn.

Ericson2314 · 2017-01-13T17:58:48Z

https://github.com/NixOS/nixpkgs/pull/21415/files#diff-19c9515018ff76dea736b57000a62d5b is the new improved motivation

shlevy

Just the one indexing issue but overall looks good!

shlevy · 2017-01-13T17:58:09Z

pkgs/stdenv/booter.nix

+  # isn't already set.
+  withAllowCustomOverrides = lib.lists.imap
+    (index: stageFun: prevStage:
+      { allowCustomOverrides = index == 1; } # first element, 1-indexed


This seems wrong still...

See the reverse below?, and then foldr not foldl?

👍 Not a big deal, but maybe change the comment on this line?

shlevy · 2017-01-13T17:58:58Z

pkgs/stdenv/booter.nix

+    inherit (args) stdenv;
+  in
+    if args.__raw or false
+    then args


Should maybe document somewhere that this function doesn't add __bootPackages when __raw == true

Oh whoops, that's not intentional. Not supposed to be that raw :).

shlevy · 2017-01-13T18:00:01Z

pkgs/stdenv/cross/default.nix

    crossSystem = null;
-    # Ignore custom stdenvs when cross compiling for compatability
+    # Remove config.replaceStdenv to ensure termination.


No need to change this, but can you explain the termination issue when replaceStdenv is there?

Ooops, old comment should still be there, bad copy paste from custom/default.nix

shlevy · 2017-01-13T18:05:01Z

pkgs/top-level/default.nix

-  stdenvFunc ? import ../stdenv
+, # A function booting the final package set for a specific standard
+  # environment. See below for the arguments given to that function,
+  # the the type of list it returns.


shlevy · 2017-01-13T18:07:13Z

pkgs/top-level/stage.nix

-  #
-  # We don't want stdenv overrides in the case of cross-building, or
-  # otherwise the basic overridden packages will not be built with the
-  # crossStdenv adapter.


Introduce new abstraction, `stdenv/booter.nix` for composing bootstraping stages, and use it everywhere for consistency. See that file for more doc. Stdenvs besides Linux and Darwin are completely refactored to utilize this. Those two, due to their size and complexity, are minimally edited for easier reviewing. No hashes should be changed.

Instead, the cross stdenv will patch up the override field -- the complexity is now confined to the one place it matters.

…ges.nix These are not packages, and so its more elegant to do this outside of all-packages.nix.

`gcc-unwrapped` basically replaces `gccPlain`. It may seem like an ugly polution to stick it in all-packages, but a future PR will enshrine this `*-unwrapped` pattern. In any event, the long term goal is stdenvs might need to tweak how compilers are booted and wrapped, but the code to build the unwrapped compilers themselves should be generic.

Code is just moved around

Ericson2314 · 2017-01-13T18:49:04Z

Linux tests passed (which do eval darwin), darwin job seems to have a backlog, I'm merging 😀!

Ericson2314 requested review from shlevy and nbp December 25, 2016 21:57

Ericson2314 mentioned this pull request Dec 25, 2016

Sane cross-compiling through bootstrapping #21268

Closed

Ericson2314 force-pushed the normalize-boot branch from ffc6957 to 7fdf2ab Compare December 25, 2016 23:16

Ericson2314 added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Dec 25, 2016

Ericson2314 added this to the 17.03 milestone Dec 25, 2016

Ericson2314 added 9.needs: community feedback 8.has: clean-up 0.kind: enhancement Add something new labels Dec 27, 2016

Ericson2314 force-pushed the normalize-boot branch from 7fdf2ab to de2868b Compare December 28, 2016 00:42

Ericson2314 self-assigned this Jan 7, 2017

shlevy reviewed Jan 13, 2017

View reviewed changes

shlevy approved these changes Jan 13, 2017

View reviewed changes

shlevy reviewed Jan 13, 2017

View reviewed changes

shlevy self-requested a review January 13, 2017 13:06

shlevy requested changes Jan 13, 2017

View reviewed changes

shlevy reviewed Jan 13, 2017

View reviewed changes

top-level: Modernize stdenv.overrides giving it self and super

0ef8b69

Document breaking change in 17.03 release notes

Ericson2314 force-pushed the normalize-boot branch 2 times, most recently from 3f71837 to af101e1 Compare January 13, 2017 17:26

Ericson2314 mentioned this pull request Jan 13, 2017

Make cross-compiling with the crossSystem argument work (v2) #21388

Closed

7 tasks

shlevy requested changes Jan 13, 2017

View reviewed changes

Ericson2314 and others added 6 commits January 13, 2017 13:23

top-level: Do stdenvOverrides in stage.nix even if crossSystem exists.

0f33b9f

Instead, the cross stdenv will patch up the override field -- the complexity is now confined to the one place it matters.

top-level: Inherit system and platform in stage.nix not all-packa…

67ebd31

…ges.nix These are not packages, and so its more elegant to do this outside of all-packages.nix.

linux stdenv: Remove stray use of stage0 to bootstrap more elegantly

b10f415

linux stdenv: Inline stage funs to conform to new convention

ff35560

Code is just moved around

Ericson2314 force-pushed the normalize-boot branch from af101e1 to ff35560 Compare January 13, 2017 18:23

stdenv/booter.nix: Add longer note explaining indexing

abaf790

Ericson2314 merged commit 0b8e389 into NixOS:master Jan 13, 2017

Ericson2314 deleted the normalize-boot branch January 13, 2017 21:10

dmmarkov mentioned this pull request Jan 19, 2017

ccacheWrapper broken #21984

Closed

Normalize bootstrapping #21415

Normalize bootstrapping #21415

Conversation

Ericson2314 commented Dec 25, 2016 • edited Loading

mention-bot commented Dec 25, 2016

cstrahan commented Dec 25, 2016

shlevy commented Dec 26, 2016

Ericson2314 commented Jan 7, 2017 • edited Loading

shlevy commented Jan 7, 2017

Ericson2314 commented Jan 7, 2017 • edited Loading

Ericson2314 commented Jan 13, 2017

shlevy left a comment • edited Loading

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

shlevy left a comment

Choose a reason for hiding this comment

shlevy left a comment

Choose a reason for hiding this comment

shlevy left a comment

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

shlevy left a comment

Choose a reason for hiding this comment

shlevy left a comment

Choose a reason for hiding this comment

shlevy left a comment

Choose a reason for hiding this comment

Ericson2314 commented Jan 13, 2017 • edited Loading

Ericson2314 commented Jan 13, 2017

shlevy commented Jan 13, 2017

Ericson2314 commented Jan 13, 2017

Ericson2314 commented Jan 13, 2017 • edited Loading

Ericson2314 commented Jan 13, 2017

Ericson2314 commented Jan 13, 2017

shlevy left a comment • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Jan 13, 2017

Ericson2314 commented Dec 25, 2016 •

edited

Loading

Ericson2314 commented Jan 7, 2017 •

edited

Loading

Ericson2314 commented Jan 7, 2017 •

edited

Loading

shlevy left a comment •

edited

Loading

Ericson2314 commented Jan 13, 2017 •

edited

Loading

Ericson2314 commented Jan 13, 2017 •

edited

Loading

shlevy left a comment •

edited

Loading