top-level: Preserve config and other arguments when recurring, while preventing information leak #19940

Ericson2314 · 2016-10-28T04:26:08Z

Current there is a few times that nixpkgs recurs, and when it does config/etc is discarded which seems...wrong. Additionally, we take advantage of the separation of the pure and impure generation of default nixpkgs arguments: only the pure generation is repeated on recur.

At the same time, this means that all-packages.nix and stdenv.nix need not more but less arguments, because some were just used when recurring and mkPackages will close over those instead. This prevents people from accidentally inspecting those arguments complicating the fixpoint. [mkPackages should have a bad smell, we don't the same things to happen without the odor.]

This is part of my #15043, and also part of #18386

This entire patch is fairly small, but is again broken up into puny commits for easy reviewing.

CC @DavidEGrayson because the patch overlaps with your first commit
CC @zimbatm because you've helped me merge that PR piece-by-piece before :D

mention-bot · 2016-10-28T04:26:19Z

@Ericson2314, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nbp to be a potential reviewer.

domenkozar · 2016-10-28T09:38:12Z

cc @nbp

domenkozar · 2016-10-28T09:38:42Z

@Ericson2314 we have to be careful here to also have in mind upcoming changes for security updates, so if you're changing top-level stuff also cc Nicolas

zimbatm

As long as it doesn't interfere with @nbp's work it looks good

DavidEGrayson · 2016-10-28T16:22:55Z

Thanks for CCing me, @Ericson2314! I approve of this pull request for the reasons that @Ericson2314 listed and also because it gets rid of the mutually recursive imports between top-level/default.nix and top-level/stdenv.nix, and it gets rid of the misleading names self and super in top-level/stdenv.nix.

As far as I can tell, the pull request is good to merge right now, except for what I think is a minor regression. It looks like you are forgetting to pass noSysDirs to top-level/stdenv.nix when you import it in top-level/default.nix. Perhaps you should make the arguments to stdenv.nix like this so that nothing is forgotten in the future?

args // { inherit mkPackages lib; }

I also have some comments that I think would improve the pull request but shouldn't prevent it from being merged:

I think that top-level/stdenv.nix doesn't need both pkgs and mkPackages arguments. The one place where it uses pkgs can be replaced by mkPackages {}, which should be totally equivalent.

This is just my personal taste, but I think the 9-line comment in top-level/default.nix for a one-line function is excessive. Since the comment is entirely about comparing the old design to the new design and justifying why the new design is OK, I would move all of it to a commit message.

Ericson2314 · 2016-10-28T17:12:16Z

@DavidEGrayson Thanks for the review and enthusiasm!

It looks like you are forgetting to pass noSysDirs to top-level/stdenv.nix when you import it in top-level/default.nix.

Huh? That file doesn't use it. mkPackages is the only way that stdenvs build phases, and that argument is either re-inferred, or closed over mkPackages if it is manually specified.

I think that top-level/stdenv.nix doesn't need both pkgs and mkPackages arguments. The one place where it uses pkgs can be replaced by mkPackages {}, which should be totally equivalent.

It is slightly faster to not rebuild pkgs. Later in my original PR, I remove the need for pkgs altogether.

This is just my personal taste, but I think the 9-line comment in top-level/default.nix for a one-line function is excessive. Since the comment is entirely about comparing the old design to the new design and justifying why the new design is OK, I would move all of it to a commit message.

After I'm all done rewriting this stuff, I plan on making a new manual section "tying nixpkgs together" or something like that. I'll probably move/rewrite the comment there then.

DavidEGrayson · 2016-10-28T17:14:28Z

Oh ok, never mind about noSysDirs.

nbp

These changes looks good, except for the removal of the topLevelArguments set, and the trivialBuilders, stdenvDefault, allPackages modifications.

nbp · 2016-10-31T17:52:26Z

pkgs/top-level/default.nix


-  stdenvDefault = self: super: (import ./stdenv.nix topLevelArguments) {} pkgs;


What the hell is pkgs doing here?!

Its been there all along, ~~but I remove it in this PR~~ a follow up PR. See https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/default.nix#L74 and your original ad31783 .

Right, the old version was very confusing, passing pkgs in as an argument and naming the argument super.

Using pkgs here, whatever it is named is a terrible choice, as stdenvCross which depends on it rely on the fix-point result and can cause much more trouble. I would prefer if we could replace this pkgs by either self or super.

@nbp stdenvCross ultimately needs an additional bootstrapping phase. I make this change in my follow-up PR #15043, in particular 6e4e6d5

The idea is

final normal phase: build = host = target

cross compiler phase: build = host != target

final cross packages phase: build != host = target

The current stuff is broken because it divvies up 2 arbitrarily between 1 and 3.

@nbp So yeah, my hope is to get this one merged as is, as multiple people have reviewed it positively, and your only complaint is procedural. Then the next one sets up cross-compiling correctly, but its probably still broken in practice (it's subtly broken already). Finally using @DavidEGrayson's and @vcunat's good work atop my foundation, we can actually fix cross-compiling.

Yeah, in my pull request (#18386), in top-level/stdenv.nix, if crossSystem is not null, then I make the final stdenv using the default stdenv like this:

(mkPackages { bootStdenv = defaultStdenv; }).stdenvCross

So I think that's what @Ericson2314 means when he says there will be another level of bootstrapping. So ultimately stdenv.nix does not need to use pkgs, but it will need mkPackages.

@DavidEGrayson What you say is true. But I'd like to do more things with that extra phase as well. In my (currently out of date) rebase of @vcunat's work, in that same phase I also build the cross tools Ericson2314@ee2a484#diff-1640a202e3ce4fd5a204d6c39e252787R110, not just stdenvCross. Then those get simply included in stdenvCross without needing to force-native or anything.

The current .nativeDrv .crossDrv is IMO a weird hack trying to fake an extra phases.

nbp · 2016-10-31T17:58:53Z

@zimbatm , Even if they are a few issues, these should be easy to merge.

Ericson2314 · 2016-10-31T19:17:05Z

@nbp it would be nice to hear why you don't like the other changes. For me they are all related--with mkPackages we can hide information stdenv.nix and allPackages.nix don't need to directly observe. That of course means the argument lists grow apart.

nbp · 2016-10-31T19:29:39Z

@Ericson2314 That's not that I don't like them, but more that they are not related to this pull request.

Ericson2314 · 2016-10-31T19:32:47Z

@nbp ...Can I rename the pull request then?

Also more seriously travis correct found a real bug. The package in all-packages that now uses mkPackages is broken.

Ericson2314 · 2016-10-31T20:12:24Z

Fixed cross compiling, and addressed the description the full scope of this PR including why I see the changes as all being related.

DavidEGrayson · 2016-11-02T21:38:03Z

Great! I will make a new pull request to put my cross-compiling fixes on top of this.

Ericson2314 · 2016-11-02T21:42:20Z

@DavidEGrayson err hold up if you don't mind. My cleanups got moved to other PR for @nbp.

DavidEGrayson · 2016-11-02T21:44:07Z

Okay, sounds good.

Ericson2314 · 2016-11-02T21:45:17Z

Cleanup commit #20108

zimbatm approved these changes Oct 28, 2016

View reviewed changes

nbp suggested changes Oct 31, 2016

View reviewed changes

nbp reviewed Oct 31, 2016

View reviewed changes

Ericson2314 force-pushed the preserve-config-on-recur branch from 1eb0a47 to 22aeda0 Compare October 31, 2016 18:51

Ericson2314 changed the title ~~top-level: Preserve config and other arguments when recurring~~ top-level: Preserve config and other arguments when recurring, while preventing information leak Oct 31, 2016

Ericson2314 force-pushed the preserve-config-on-recur branch from 22aeda0 to a1b870a Compare October 31, 2016 20:08

Ericson2314 force-pushed the preserve-config-on-recur branch 3 times, most recently from f4cb25e to 88ecfaa Compare November 2, 2016 20:39

top-level: Use nixpkgsFun to avoid import ../..

09198c5

Ericson2314 force-pushed the preserve-config-on-recur branch from 88ecfaa to 09198c5 Compare November 2, 2016 20:46

nbp approved these changes Nov 2, 2016

View reviewed changes

nbp merged commit a113382 into NixOS:master Nov 2, 2016

Ericson2314 deleted the preserve-config-on-recur branch November 2, 2016 21:40

Ericson2314 mentioned this pull request Nov 2, 2016

Move stdenvCross and customStdenv to pkgs/stdenv #20108

Merged

DavidEGrayson mentioned this pull request Dec 1, 2016

Make cross-compiling with the crossSystem argument work #18386

Closed

7 tasks

DavidEGrayson mentioned this pull request Dec 24, 2016

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

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

top-level: Preserve config and other arguments when recurring, while preventing information leak #19940

top-level: Preserve config and other arguments when recurring, while preventing information leak #19940

Ericson2314 commented Oct 28, 2016 •

edited

mention-bot commented Oct 28, 2016

domenkozar commented Oct 28, 2016

domenkozar commented Oct 28, 2016

zimbatm left a comment

DavidEGrayson commented Oct 28, 2016 •

edited

Ericson2314 commented Oct 28, 2016

DavidEGrayson commented Oct 28, 2016

nbp left a comment

nbp Oct 31, 2016

Ericson2314 Oct 31, 2016 •

edited

DavidEGrayson Oct 31, 2016

nbp Nov 1, 2016

Ericson2314 Nov 1, 2016 •

edited

Ericson2314 Nov 1, 2016

Ericson2314 Nov 1, 2016

DavidEGrayson Nov 1, 2016 •

edited

Ericson2314 Nov 1, 2016 •

edited

Ericson2314 Nov 1, 2016

nbp commented Oct 31, 2016

Ericson2314 commented Oct 31, 2016 •

edited

nbp commented Oct 31, 2016

Ericson2314 commented Oct 31, 2016 •

edited

Ericson2314 commented Oct 31, 2016 •

edited

DavidEGrayson commented Nov 2, 2016 •

edited

Ericson2314 commented Nov 2, 2016

DavidEGrayson commented Nov 2, 2016

Ericson2314 commented Nov 2, 2016


		stdenvDefault = self: super: (import ./stdenv.nix topLevelArguments) {} pkgs;

top-level: Preserve config and other arguments when recurring, while preventing information leak #19940

top-level: Preserve config and other arguments when recurring, while preventing information leak #19940

Conversation

Ericson2314 commented Oct 28, 2016 • edited

mention-bot commented Oct 28, 2016

domenkozar commented Oct 28, 2016

domenkozar commented Oct 28, 2016

zimbatm left a comment

Choose a reason for hiding this comment

DavidEGrayson commented Oct 28, 2016 • edited

Ericson2314 commented Oct 28, 2016

DavidEGrayson commented Oct 28, 2016

nbp left a comment

Choose a reason for hiding this comment

nbp Oct 31, 2016

Choose a reason for hiding this comment

Ericson2314 Oct 31, 2016 • edited

Choose a reason for hiding this comment

DavidEGrayson Oct 31, 2016

Choose a reason for hiding this comment

nbp Nov 1, 2016

Choose a reason for hiding this comment

Ericson2314 Nov 1, 2016 • edited

Choose a reason for hiding this comment

Ericson2314 Nov 1, 2016

Choose a reason for hiding this comment

Ericson2314 Nov 1, 2016

Choose a reason for hiding this comment

DavidEGrayson Nov 1, 2016 • edited

Choose a reason for hiding this comment

Ericson2314 Nov 1, 2016 • edited

Choose a reason for hiding this comment

Ericson2314 Nov 1, 2016

Choose a reason for hiding this comment

nbp commented Oct 31, 2016

Ericson2314 commented Oct 31, 2016 • edited

nbp commented Oct 31, 2016

Ericson2314 commented Oct 31, 2016 • edited

Ericson2314 commented Oct 31, 2016 • edited

DavidEGrayson commented Nov 2, 2016 • edited

Ericson2314 commented Nov 2, 2016

DavidEGrayson commented Nov 2, 2016

Ericson2314 commented Nov 2, 2016

Ericson2314 commented Oct 28, 2016 •

edited

DavidEGrayson commented Oct 28, 2016 •

edited

Ericson2314 Oct 31, 2016 •

edited

Ericson2314 Nov 1, 2016 •

edited

DavidEGrayson Nov 1, 2016 •

edited

Ericson2314 Nov 1, 2016 •

edited

Ericson2314 commented Oct 31, 2016 •

edited

Ericson2314 commented Oct 31, 2016 •

edited

Ericson2314 commented Oct 31, 2016 •

edited

DavidEGrayson commented Nov 2, 2016 •

edited