Move stdenvCross and customStdenv to pkgs/stdenv by Ericson2314 · Pull Request #20108 · NixOS/nixpkgs

Ericson2314 · 2016-11-02T21:44:55Z

Motivation for this change

Cleanup enabled by #19940. This too leads up to #15043. I pulled some more commits from #15043 so that now top-level/stdenv.nix hardly needs to exist.

Cross compiling still behaviors weird (I don't want to change semantics with this patch), but the weirdness is syntactically isolated from normal behavior: pkgs/stdenv/cross/default.nix won't be imported unless one is cross compiling and the weirdness is confined to there.

~~@zimbatm this should basically be what you reviewed originally, modulo identifier renaming.~~

Things done

I modified @DavidEGrayson's excellent test script at https://gist.github.com/Ericson2314/fc745290db68b20235bdc457961b3658 . It now just checks for all combinations with all combinations: native target, cross compiling, custom stdenvs, and custom stdenvs + cross compiling. You can replicate my results by running

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

CC @nbp @DavidEGrayson

mention-bot · 2016-11-02T21:44:56Z

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

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

pkgs/top-level/default.nix

Indenting by 6 spaces without lining up to something seems odd.

Hmm it's supposed to be beginning of res + 2, but that does look odd. Seeing that

let # note newline foo = ...; bar = ...;

would be IMO proper style for multiple lets, I'll just unindent back to two from let.

DavidEGrayson · 2016-11-02T21:51:37Z

Besides my minor indentation complaint, it looks good to me. You are basically cleaning up the arguments that are given to the top-level stdenv.nix and all-packages.nix.

EDIT: The pull request has changed dramatically since I made this comment and I have not had time to review it fully.

[Trying my best to indent properly while avoiding excessive drift]

…env}.nix

Ericson2314 · 2016-11-02T22:47:42Z

@DavidEGrayson so while you definitely want to rebase on this, you probably also should rebase on top of #15043^ (i.e. all but the last commit, subsuming this), and furthermore also take a look at @vcunat's stdenv which I linked earlier. Lot of indirection there sorry :/.

To make all that less of a pain I rebased my bitrotted rebase of vcunat's stdenv on #15043^: https://github.com/Ericson2314/nixpkgs/tree/vcunat-cross-stdenv . For context, @vcunat's original branch is https://github.com/vcunat/nixpkgs/tree/v/cross-native-rework .

nbp · 2016-11-02T23:42:40Z

I kind of like topLevelArguments, why would that be a mandatory change?

Ericson2314 · 2016-11-02T23:44:32Z

@nbp in the follow up PR they completely diverge https://github.com/NixOS/nixpkgs/pull/15043/files#diff-69df24a05ec3d577cb638113e9837ae1R49; during development (as 1 big PR) the ... just makes things harder.

DavidEGrayson · 2016-11-02T23:58:06Z

I actually think topLevelArguments was misleading since it contained things like lib that were not arguments to top-level/default.nix. Now that were are using @ args, that won't happen again.

Ericson2314 · 2016-11-03T00:12:09Z

I'm working on maybe moving some commits from the original (now followup) PR to this, to motivate (either way, i debug them so no loss). The @vcunat stuff may be needed just to get things working again, but that may swing things in the direction of leave this as is to not over-complicate with semantic change.

Ericson2314 · 2016-11-03T01:58:01Z

Ok, added more commits from the original PR because I believe I figured out how to preserve exact semantics, yay!

Ericson2314 · 2016-11-03T02:34:20Z

@DavidEGrayson thanks for your test script, it gives me much more confidence in this :). I still recommend looking at the my vcunat-cross-stdenv branch, but that branch will be rebased on #15043 in a moment so there's absolutely no reason to wait for that follow-up PR!

DavidEGrayson · 2016-11-03T03:38:22Z

I'm glad you found it useful!

Ericson2314 · 2016-11-03T03:40:56Z

Do you have a moment for IRC?

Ericson2314 · 2016-11-03T04:14:03Z

~~@nbp this should be ready to go.~~

Ericson2314 · 2016-11-04T00:13:42Z

@nbp ok now this is actually ready to go! all the stdenv combinations have been tested. And yes...I did fine a bug in in stdenvCustom, so shame on me for not testing it before.

nbp

Fix the nits, and I would be ok for merging.

The allowCustomOverrides attribute is added as argument of the top-level of nixpkgs set computation to compensate from the fact that the stdenv replacement and the stdenvCross do benefit from the overridden configuration provided by the user, as opposed to the bootstrapping stdenv.

nbp · 2016-11-05T15:41:29Z

pkgs/stdenv/cross/default.nix

nit: Notice the the

nbp · 2016-11-05T16:01:42Z

pkgs/stdenv/cross/default.nix

Do not use with, to prevent issues caused by adding names such as vanillaStdenv & crossSystem at the top-level of nixpkgs set, or from the config override.

nbp · 2016-11-05T16:03:04Z

pkgs/stdenv/cross/default.nix

This is working, because the derivation used to build the stdenvCross are in fact using forceNativeDrv to use the nativeDrv attribute of the resulting derivation build with vanillaStdenv (second argument of makeStdenvCross) So technically you could use the same buildPackages as used in custom.nix.

I think you can fix this comment for now, and make a follow-up patch for that after.

Yeah I originally had a proper fix along those lines, but was getting missing identifier errors. Follow-up patch is the intent, but there is some broken code out there that will make it non-trivial.

By fixing comment, you mean add basically what you wrote explaining why this works at all? Or just the "the the"?

both, the "the the", and explaining why this works.

nbp · 2016-11-05T16:21:13Z

I suggest you rename this pull-request to “Move stdenvCross and customStdenv to pkgs/stdenv.”

Ericson2314 · 2016-11-05T17:48:27Z

@nbp Thanks! I will fix those things momentarily.

...but actually is weird just like the original

Ericson2314 · 2016-11-07T05:31:41Z

@nbp Ok fixed everything (and tested again)! I took part of your review message and also made it a comment. More explanation never hurts!

top-level: Remove redundant parentheses

05977b2

Ericson2314 mentioned this pull request Nov 2, 2016

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

Merged

DavidEGrayson reviewed Nov 2, 2016

View reviewed changes

Ericson2314 added 2 commits November 2, 2016 18:35

top-level: Inline topLevelArugments, and make pkg named arg for stdenv

68a04c1

[Trying my best to indent properly while avoiding excessive drift]

top-level: Remove redundant arguments from top-level/{all-package,std…

eed34bd

…env}.nix

Ericson2314 force-pushed the top-level-cleanup branch from 007c302 to eed34bd Compare November 2, 2016 22:37

Ericson2314 force-pushed the top-level-cleanup branch 2 times, most recently from 336e22a to f822985 Compare November 3, 2016 23:14

nbp approved these changes Nov 5, 2016

View reviewed changes

nbp added 0.kind: enhancement Add something new or improve an existing system. 8.has: clean-up This PR removes packages or removes other cruft labels Nov 5, 2016

nbp added this to the 17.03 milestone Nov 5, 2016

Ericson2314 changed the title ~~Top level cleanup~~ Move stdenvCross and customStdenv to pkgs/stdenv Nov 5, 2016

Ericson2314 force-pushed the top-level-cleanup branch from f822985 to 4783b77 Compare November 7, 2016 05:14

top-level: Make stdenvCross which appears at first glance normal...

e22346c

...but actually is weird just like the original

Ericson2314 added 2 commits November 6, 2016 21:28

top-level: top-level/stdenv.nix no longer needs pkg argument

ea7bf02

top-level: Make config-overriden stdenv bootstrap more normally

6bfe042

Ericson2314 force-pushed the top-level-cleanup branch from 4783b77 to 6bfe042 Compare November 7, 2016 05:29

Ericson2314 merged commit a24728f into NixOS:master Nov 8, 2016

Ericson2314 deleted the top-level-cleanup branch November 11, 2016 03:55

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

Uh oh!

Conversation

Ericson2314 commented Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for this change

Things done

Uh oh!

mention-bot commented Nov 2, 2016

Uh oh!

DavidEGrayson Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

DavidEGrayson commented Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nbp commented Nov 2, 2016

Uh oh!

Ericson2314 commented Nov 2, 2016

Uh oh!

DavidEGrayson commented Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Nov 3, 2016

Uh oh!

Ericson2314 commented Nov 3, 2016

Uh oh!

Ericson2314 commented Nov 3, 2016

Uh oh!

DavidEGrayson commented Nov 3, 2016

Uh oh!

Ericson2314 commented Nov 3, 2016

Uh oh!

Ericson2314 commented Nov 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Nov 4, 2016

Uh oh!

nbp left a comment

Choose a reason for hiding this comment

Uh oh!

nbp Nov 5, 2016

Choose a reason for hiding this comment

Uh oh!

nbp Nov 5, 2016

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Nov 5, 2016

Choose a reason for hiding this comment

Uh oh!

nbp Nov 5, 2016

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Nov 5, 2016 • 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!

nbp Nov 5, 2016

Choose a reason for hiding this comment

Uh oh!

nbp commented Nov 5, 2016

Uh oh!

Ericson2314 commented Nov 5, 2016

Uh oh!

Ericson2314 commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Ericson2314 commented Nov 2, 2016 •

edited

Loading

DavidEGrayson commented Nov 2, 2016 •

edited

Loading

Ericson2314 commented Nov 2, 2016 •

edited

Loading

DavidEGrayson commented Nov 2, 2016 •

edited

Loading

Ericson2314 commented Nov 3, 2016 •

edited

Loading

Ericson2314 Nov 5, 2016 •

edited

Loading

Ericson2314 commented Nov 7, 2016 •

edited

Loading