stdenv: remove assumption of exactly two cross stages #251299

ghost · 2023-08-25T05:50:09Z

If you want to see an example of the problem this PR solves, cherry pick just the "stdenv: cross: insert no-op stages" commit and nix-instantiate . -A pkgsCross.<anything>.

Motivation

This removes one of the last obstacles to unifying our native and cross bootstraps, which will allow to eliminate all the libcCrossChooser and simply reuse the relevant stages from pkgs/stdenv/linux/default.nix instead.

Description of changes

The native stdenv bootstrap (e.g. pkgs/stdenv/linux/default.nix) is very elegantly structured as a sequence of arbitrarily-many stages. This makes it easy to refactor and modularize and reuse, because if a stage gets too complicated we can just split it up into two separate sequential stages. We can also insert optional stages.

Unfortunately we can't do the same thing with cross-compiled stdenvs, because adjacentPackages includes the hardwired assumption that there are exactly two stages after the end of the native bootstrap stage sequence:

The (build==host)!=target stage
The build!=(host==target) stage

This PR eliminates that assumption.

Instead of resolving things like pkgsBuildHost to prevStage, we instead search backwards for the most recent stage which creates packages on our buildPlatform and for our hostPlatform.

This PR should not affect eval.

Note that pkgsTargetTarget, which becomes targetPackages still has hardwired assumptions. I think setting pkgsTargetTarget to nextStage is just flat-out wrong for every stage except the "main" stage (the build!=(host==target) stage), but I fear that there may be code that relies on the particular way that it is wrong. It looks like pkgsTargetTarget and targetPackages exist exclusively in order to provide access to the postStage hack; I intend to submit a separate PR that throws if it is used in any other way, to see what breaks.

Things done

pkgs/stdenv/booter.nix

Artturin · 2023-08-25T18:56:17Z

nix-diff $(nix eval --raw ".?rev=$(git merge-base master HEAD)#pkgsCross.aarch64-multiplatform.sway.drvPath") $(nix eval --raw ".#pkgsCross.aarch64-multiplatform.sway.drvPath")

nix-diff $(nix eval --raw ".?rev=$(git merge-base master HEAD)#pkgsCross.aarch64-multiplatform.python3Packages.cryptography.drvPath") $(nix eval --raw ".#pkgsCross.aarch64-multiplatform.python3Packages.cryptography.drvPath")

nix-diff $(nix eval --raw ".?rev=$(git merge-base master HEAD)#pkgsCross.aarch64-multiplatform.rmlint.drvPath") $(nix eval --raw ".#pkgsCross.aarch64-multiplatform.rmlint.drvPath")

no eval changes, gobject-introspection not broken.

Artturin · 2023-08-27T23:19:24Z

Checking if these stats are correct

EDIT:

EDIT after removing extra no-op stages:

roberth · 2023-08-28T02:40:45Z

I don't think ofborg does a lot of cross eval, so 1% extra work seems rather significant.

Maybe the new items in the stage list are significant (multiple // on a whole pkgs attrset - or maybe I'm mistaken), or maybe some binding is accidentally not shared anymore.

1% extra work is based on the total allocation stat, which is a decent proxy for the amount of work done.
Could you try locally with NIX_SHOW_STATS=1 nix-build -A?

Artturin · 2023-09-05T19:06:12Z

removed 2 of the no-op stages, left 1 in.

Artturin · 2023-09-05T20:37:18Z

It was the extra stages

NIX_SHOW_STATS=1 nix-instantiate -A pkgsCross.aarch64-multiplatform.bash

3c3
<   "cpuTime": 0.19761300086975098,
---
>   "cpuTime": 0.19300399720668793,
5,7c5,7
<     "bytes": 7536160,
<     "elements": 374460,
<     "number": 283780
---
>     "bytes": 8005344,
>     "elements": 394312,
>     "number": 303178
11c11
<     "totalBytes": 104882656
---
>     "totalBytes": 116688640
14,24c14,24
<     "bytes": 1682440,
<     "concats": 7054,
<     "elements": 210305
<   },
<   "nrAvoided": 393536,
<   "nrFunctionCalls": 255442,
<   "nrLookups": 113863,
<   "nrOpUpdateValuesCopied": 3048123,
<   "nrOpUpdates": 17927,
<   "nrPrimOpCalls": 175909,
<   "nrThunks": 681368,
---
>     "bytes": 1986528,
>     "concats": 7060,
>     "elements": 248316
>   },
>   "nrAvoided": 412934,
>   "nrFunctionCalls": 274757,
>   "nrLookups": 114224,
>   "nrOpUpdateValuesCopied": 3363935,
>   "nrOpUpdates": 18007,
>   "nrPrimOpCalls": 176018,
>   "nrThunks": 719493,
26,28c26,28
<     "bytes": 63105280,
<     "elements": 3841922,
<     "number": 102158
---
>     "bytes": 71197280,
>     "elements": 4328578,
>     "number": 121252
38c38
<     "number": 22681
---
>     "number": 22683
41,42c41,42
<     "bytes": 19680096,
<     "number": 820004
---
>     "bytes": 21508920,
>     "number": 896205

pkgs/stdenv/booter.nix

pkgs/stdenv/cross/default.nix

Previously, stdenv used `adjacentPackages==null` to signal that `actuallySplice` should be `false`. Let's pass it explicitly for greater clarity.

It's quite difficult to understand what this `selfBuild` parameter is signaling, but it turns out to simply indicate which stages have any kind of cross compilation (i.e. either build!=host or host!=target). Let's check that condition directly for greater clarity.

The native stdenv bootstrap (e.g. `pkgs/stdenv/linux/default.nix`) is very elegantly structured as a sequence of arbitrarily-many stages. This makes it easy to refactor and modularize and reuse, because if a stage gets too complicated we can just split it up into two separate sequential stages. We can also insert optional stages. Unfortunately we can't do the same thing with cross-compiled stdenvs, because `adjacentPackages` includes the hardwired assumption that there are exactly two stages after the end of the native bootstrap stage sequence: - The (build==host)!=target stage - The build!=(host==target) stage This commit eliminates that assumption. Instead of resolving things like `pkgsBuildHost` to `prevStage`, we instead *search backwards* for the most recent stage which creates packages on our buildPlatform and for our hostPlatform. This commit should not affect eval. Note that `pkgsTargetTarget`, which becomes `targetPackages` still has hardwired assumptions. I think setting `pkgsTargetTarget` to `nextStage` is just flat-out wrong for every stage except the "main" stage (the `build!=(host==target)` stage), but I fear that there may be code that relies on the particular way that it is wrong. It looks like `pkgsTargetTarget` and `targetPackages` exist exclusively in order to provide access to the `postStage` hack; I intend to submit a separate PR that `throw`s if it is used in any other way, to see what breaks. Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

This commit inserts extra stages into the cross-compilation stdenv bootstrap in order to prevent assumptions about the number of stages from creeping back in (see previous commit). If this commit is used without the previous commit, eval will fail badly. The no-op stages added by this commit serve as an example of what wasn't possible before the PR which includes them. Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

ghost · 2023-10-22T06:20:07Z

Squashed, rebased.

There is now only one no-op stage, but with an expanded comment explaining why it thwarts unjustified assumptions, and a mention of the other two (removed) no-op stages in case re-introducing them helps with troubleshooting.

Ericson2314 · 2023-10-25T19:41:52Z

Doesn't this break "identity cross compilation"? I.e. forcing a round of cross bootstrapping even though buildPlatform == hostPlatform.

This is funny to me because I sort of wanted to go in the opposite direction: the native bootstraps do not need all these stages, instead there should be just one "with bootstrapping tools" stage and then one "without bootstrapping tools" stage.

But I am willing to do this because I don't yet have time to do rework things as I have wanted to for years, and @amjoseph-nixpkgs is motivated and does have time for putting in the work.

Ericson2314 · 2023-10-25T19:46:47Z

which will allow to eliminate all the libcCrossChooser and simply reuse pkgs/stdenv/linux/default.nix instead.

This is not describing this PR, but this actually sounds quite bad to me. The linux bootstrap is totally overfit to GCC, and the current cross bootstrapping works with LLVM or GCC.

ghost · 2023-10-26T03:35:10Z

I've revised this to:

eliminate all the libcCrossChooser and simply reuse the relevant stages from stdenv/linux

(Text moved to #263684 since it will remain useful even after this PR is closed).

This is funny to me because I sort of wanted to go in the opposite direction: the native bootstraps do not need all these stages

It's actually the other way around: our native bootstrap has more steps than our cross bootstrap.

The linux bootstrap is totally overfit to GCC, and the current cross bootstrapping works with LLVM or GCC.

Don't worry; I have absolutely no intention of reducing our cross bootstrap's capability. If I can't make it work for every case in tests.cross.sanity, I will not undraftify it. Obviously that includes LLVM!

It's quite likely that the end result will have certain stages skipped as totally unnecessary if isLLVM. Then you'll have a nice concise example to point at in order to show people how much better LLVM's build process is than GCC's!

ghost · 2023-10-26T03:52:58Z

Doesn't this break "identity cross compilation"? I.e. forcing a round of cross bootstrapping even though buildPlatform == hostPlatform.

I don't quite understand what you mean here.

Could you give an example (like pkgsCross.something.something.something or (import <nixpkgs>) { ... }) of where you see this happening?

I don't yet have time to do rework things as I have wanted to for years,

@Ericson2314 I would like to hear more about this! If I haven't already.

github-actions bot added the 6.topic: stdenv Standard environment label Aug 25, 2023

ghost changed the title ~~stdenv: remove assumption that there are exactly two non-native stages~~ stdenv: remove assumption of exactly two cross stages Aug 25, 2023

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Aug 25, 2023

ghost marked this pull request as ready for review August 25, 2023 07:13

ghost requested review from Ericson2314 and matthewbauer as code owners August 25, 2023 07:13

ghost requested a review from Artturin August 25, 2023 07:13

Artturin reviewed Aug 25, 2023

View reviewed changes

pkgs/stdenv/booter.nix Outdated Show resolved Hide resolved

ofborg bot added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Aug 25, 2023

ghost requested a review from Artturin August 25, 2023 20:50

Artturin requested a review from roberth August 28, 2023 00:39

roberth reviewed Sep 6, 2023

View reviewed changes

pkgs/stdenv/booter.nix Outdated Show resolved Hide resolved

roberth reviewed Sep 6, 2023

View reviewed changes

pkgs/stdenv/cross/default.nix Outdated Show resolved Hide resolved

Adam Joseph and others added 4 commits October 21, 2023 23:18

stdenv: pass actuallySplice explicitly

eda7de0

Previously, stdenv used `adjacentPackages==null` to signal that `actuallySplice` should be `false`. Let's pass it explicitly for greater clarity.

This was referenced Oct 24, 2023

make-derivation.nix: lib.warn if drv.__spliced missing #263082

Closed

stdenv: inline and remove actuallySplice (no effect on eval) #263278

Closed

Artturin requested a review from roberth October 25, 2023 17:57

roberth approved these changes Oct 25, 2023

View reviewed changes

roberth requested review from roberth and removed request for roberth October 25, 2023 20:18

ghost closed this Jan 23, 2024

ghost deleted the pr/stdenv/cross/allow-more-than-two-stages branch January 23, 2024 06:50

Artturin mentioned this pull request Jan 29, 2024

List of @amjoseph-nixpkgs Pull requests before account removal #284796

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stdenv: remove assumption of exactly two cross stages #251299

stdenv: remove assumption of exactly two cross stages #251299

ghost commented Aug 25, 2023 •

edited by ghost

Loading

Artturin commented Aug 25, 2023 •

edited

Loading

Artturin commented Aug 27, 2023 •

edited

Loading

roberth commented Aug 28, 2023

Artturin commented Sep 5, 2023

Artturin commented Sep 5, 2023 •

edited

Loading

ghost commented Oct 22, 2023

Ericson2314 commented Oct 25, 2023 •

edited

Loading

Ericson2314 commented Oct 25, 2023

ghost commented Oct 26, 2023 •

edited by ghost

Loading

ghost commented Oct 26, 2023 •

edited by ghost

Loading

stdenv: remove assumption of exactly two cross stages #251299

stdenv: remove assumption of exactly two cross stages #251299

Conversation

ghost commented Aug 25, 2023 • edited by ghost Loading

Motivation

Description of changes

Things done

Artturin commented Aug 25, 2023 • edited Loading

Artturin commented Aug 27, 2023 • edited Loading

roberth commented Aug 28, 2023

Artturin commented Sep 5, 2023

Artturin commented Sep 5, 2023 • edited Loading

ghost commented Oct 22, 2023

Ericson2314 commented Oct 25, 2023 • edited Loading

Ericson2314 commented Oct 25, 2023

ghost commented Oct 26, 2023 • edited by ghost Loading

ghost commented Oct 26, 2023 • edited by ghost Loading

ghost commented Aug 25, 2023 •

edited by ghost

Loading

Artturin commented Aug 25, 2023 •

edited

Loading

Artturin commented Aug 27, 2023 •

edited

Loading

Artturin commented Sep 5, 2023 •

edited

Loading

Ericson2314 commented Oct 25, 2023 •

edited

Loading

ghost commented Oct 26, 2023 •

edited by ghost

Loading

ghost commented Oct 26, 2023 •

edited by ghost

Loading