Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

express #208478 as assertions #210019

Merged
merged 1 commit into from Feb 21, 2023
Merged

express #208478 as assertions #210019

merged 1 commit into from Feb 21, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jan 10, 2023

Description of changes

PR #208478 added a lot of documentation about which packages were rebuilt in each stage of the stdenv bootstrap. However nothing checks that these comments agree with reality; they can bitrot over time. This PR rewrites those comments as assertions, so they cannot bitrot.

This conversion did expose some ambiguity in our scheme for naming the stages. Suppose that pkgs.stdenv.name=="stdenv-stage4". Which of these is "the stage4 coreutils"?

pkgs.coreutils
pkgs.stdenv.__bootPackages.coreutils

The choice is arbitrary, and both choices have confusing corner cases. We should revisit this at some point.

Things done
  • Built on platform(s)
    • x86_64-linux
    • powerpc64le-linux
    • aarch64-linux
  • Fits CONTRIBUTING.md.

@ghost ghost requested review from matthewbauer and Ericson2314 as code owners January 10, 2023 11:05
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Jan 10, 2023
@ghost ghost mentioned this pull request Jan 10, 2023
4 tasks
@ofborg ofborg bot added 8.has: clean-up 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jan 10, 2023
@ghost
Copy link
Author

ghost commented Jan 13, 2023

@trofi, I think this conversion-to-asserts caught a small mistake in #208478 -- the descriptions of the stages were all correct, but were shifted forward by one stdenv stage. Does this sound right?

@trofi
Copy link
Contributor

trofi commented Jan 13, 2023

It probably depends on how one looks at the stage's description. If stage's description says "resulting stage1 stdenv: binutils from nixpkgs" it can mean 2 things:

  1. this stdenv is constructed with rebuilt binutils in stdenv.cc.bintools (n)
  2. this stage provides a rebuilt by current stdenv binutils (or binutils-unwrapped) attribute for future use in next stdenv.cc.bintools (n+1)

I think I used 1. definition as I validated it by checking stdenv -> $pkg arrow. But might have mixed up with 2. as well.

Just to trace through one example: if we pick bootsrrap-stage2 it says resulting stage2 stdenv: binutils: from nixpkgs.

Checking:

# making sure it's stage2:
$ nix-instantiate -A stdenv.__bootPackages.stdenv.__bootPackages.stdenv.__bootPackages.stdenv
/nix/store/p95wb46d5bwzc9g0sr2i4dli5m9fsmmk-bootstrap-stage2-stdenv-linux.drv

$ nix-store --query --graph $(nix-instantiate -A stdenv.__bootPackages.stdenv.__bootPackages.stdenv.__bootPackages.stdenv) | grep 'stdenv.* -> .*binutils-2.39.drv'
"v14npkxf15hrzzl0a79dqvj5mzcvdc49-bootstrap-stage1-stdenv-linux.drv" -> "psjzwzbiawhg02h2by803micrd6n2y2m-binutils-2.39.drv" [color = "black"];

I think that checks out fine: stage2 itself pulls in new binutils here. If we validate bootstrap-stage1 stdenv closure it will not be there. Looks OK.

@ghost
Copy link
Author

ghost commented Jan 14, 2023

It probably depends on how one looks at the stage's description.

Oh I see. If pkgs.stdenv.name=="stdenv-stage4", then which of these is "the stage4 coreutils"?

pkgs.coreutils
pkgs.stdenv.__bootPackages.coreutils

The choice is arbitrary, and both choices have confusing corner cases.

I've updated the commit message.

@ghost
Copy link
Author

ghost commented Jan 14, 2023

The choice is arbitrary

Somehow this reminds me of @trofi's avatar artwork. 😄

@ghost
Copy link
Author

ghost commented Jan 15, 2023

Resolved merge conflict.

@ghost
Copy link
Author

ghost commented Jan 16, 2023

This PR has been absorbed into #209870, which is now ready for review.

@ghost ghost closed this Jan 16, 2023
@ghost ghost reopened this Feb 13, 2023
@ghost ghost marked this pull request as draft February 13, 2023 21:29
@ghost ghost marked this pull request as ready for review February 13, 2023 21:54
pkgs/stdenv/linux/default.nix Show resolved Hide resolved
PR #208478 added a lot of documentation about which packages were
rebuilt in each stage of the stdenv bootstrap.  However nothing
checks that these comments agree with reality; they can bitrot over
time.  This PR rewrites those comments as assertions, so they cannot
bitrot.

This conversion did expose some ambiguity in our scheme for naming
the stages.   Suppose that `pkgs.stdenv.name=="stdenv-stage4", then
which of these is "the stage4 coreutils"?

```
pkgs.coreutils
pkgs.stdenv.__bootPackages.coreutils
```

The choice is arbitrary, and both choices have confusing corner
cases.  We should revisit this at some point.
@ghost
Copy link
Author

ghost commented Feb 21, 2023

Squashed.

@ghost ghost requested review from trofi and removed request for matthewbauer and Ericson2314 February 21, 2023 07:27
@trofi trofi merged commit 3057968 into NixOS:staging Feb 21, 2023
@SuperSandro2000
Copy link
Member

I just wanted to say that I really that we add more safe guards to the bootstrapping process to make it less error prone for changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: stdenv Standard environment 8.has: clean-up 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants