Revert "stdenv: label the ephemeral coreutils-stage4 package"

[ghost]

Description of changes

#169378 (comment)

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.11 Release Notes (or backporting 22.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

This reverts commit 23ea8b3.

[vcunat]

This doesn't look urgent to me, so why target staging-next?

[ghost]

This doesn't look urgent to me, so why target staging-next?

To prevent the commit-being-reverted from making it to master.

If there is no risk of that happening I can change the base branch back to staging.

[ghost]

To prevent the commit-being-reverted from making it to master.

To clarify: if the commit-being-reverted and the revert are merged to master in separate "batches", it will not break eval or any builds. It will:

1. Cause two rebuild-everything's for people working at master instead of just one.
2. Perhaps cause some minor confusion when people see the pname of coreutils briefly change to coreutils-stage4 and then back to coreutils.

Both of these are entirely my fault.

If neither of those is serious enough for a merge into staging-next, just say so and I'll change the base branch. I don't have a good feel for what the threshhold is for targeting staging-next, although I guessed it might be lower in this case since the commit is a revert of something that hasn't yet reached master. I may have guessed wrong there.

[vcunat]

Rebuilds: as for the build farm, the thing is that it already has rebuilt coreutils and very many packages depending on it; for users, basically every staging iteration has a stdenv rebuild anyway.

[vcunat]

So yes, the two choices are staging-next ASAP or – as I suggested – deferring to the next staging-next iteration (e.g. merging to staging, no need to hurry with that), which would mean that master and channels would contain the in-between state for some time.

[ghost]

Thank you for explaining! I have switched the base branch.

[Mindavi]

No worries, it happens :)

[ghost]

In case anybody is wondering, here is the full story on what happened here.

While working through the stdenv bootstrapping process in order to make the powerpc64le bootstrap finish, I discovered that there were five packages which were getting built twice (once during the bootstrap phases and then again as part of all-packages.nix):

gmp mpfr mpc isl coreutils

It was extremely frustrating to always have two of each of these in the output of nix-tree and nix-store -q --requisites! I was constantly getting mixed up about which "gmp" was the special bootstrap-gmp and which was the "real" gmp. So I wrote two commits:

02630180fad510ee877fa51112b7c7b230ef2f13 stdenv: add -stageX markers to gmp, mpfr, libmpc, and isl
23ea8b35dacd9152c9e0e21577d5afe3e39b6255 stdenv: label the ephemeral coreutils-stage4 package

This was extremely helpful to me and got rid of all the confusion. I included these commits in PR #169378 so others could benefit from the reduced confusion as well.

The double-rebuilding of gmp, mpfr, libmpc, and isl is due to the deliberate design of stdenv. The *-stageX versions are compiled statically, in order to prevent stdenv.cc from having references to bootstrap-files.

The double-rebuilding of coreutils, however, was not caused by stdenv. It was not even caused by anything in nixpkgs. It was caused by one of my own site-specific overlays. I did not realize this. So I was "fixing" a problem caused by a commit that only I was using! The "fix" worked great for me -- one of my coreutils builds had pname="coreutils-stage4", and the other one had pname="coreutils".

Unfortunately for everybody else who didn't have my site-specific problem in the first place, the "fix" simply renamed their one-and-only copy of coreutils to coreutils-stage4.

The site-specific overlay I have that was causing a double-build of coreutils was a (self: super: ...) function where the ... body of the function incorrectly used super rather than self.

[ncfavier]

Just here to mention that the home-manager CI tests are currently broken because of the extra -stage4 (but it looks like this will fix it). However the -stage4 isn't there when running the tests from the command line. Any idea why? (I'm dumb, it was simply because my ambient nixpkgs was too old)

[ghost]

Just here to mention that the home-manager CI tests are currently broken because of the extra -stage4 (but it looks like this will fix it).

At what commit? Both the PR which introduced the bug and the PR which fixed it were merged to master in the same commit, 2543ab9. You should never have seen the unfixed code.

[ncfavier]

Yet one is in nixos-unstable and one isn't yet.

The commit you're pointing to only has the fix, so the breakage was merged earlier: 2543ab9#diff-bf918d7f48f6a34a4a1acb33373dee717cff2c3f5503a7ca41d90e5f289faf1e

[ghost]

The commit you're pointing to only has the fix, so the breakage was merged earlier: 2543ab9#diff-bf918d7f48f6a34a4a1acb33373dee717cff2c3f5503a7ca41d90e5f289faf1e

The fix (1b94ad9) is in 2543ab9:

git merge-base --is-ancestor 1b94ad9 2543ab9 && echo yes

[ghost]

I can't find any commit in master which has the bug but doesn't have the fix. @ncfavier, exactly which master commit are you referring to?

[ncfavier]

The current nixos-unstable, as I said. e4d49de

[ghost]

The current nixos-unstable, as I said. e4d49de

Yes, you are totally right. Thank you for being so patient with me. When I screw up, which I did here, it's important for me to fully understand what went wrong.

Merge b39924f brought the bug (23ea8b3) but not the fix (1b94ad9) from staging(-next) to master.

My more serious mistake was thinking that changing the pname of a package would tend not to break automated things. I have always assumed that pnames are mainly to make nix-env/nix profile more convenient for humans to use, so we don't have to reference installed packages by their deriving derivations. But now I see from the CI failure you linked that home-manager has some neat mechanism for referencing store paths by pname, independent of their output address:

ExecReload=/nix/store/00000000000000000000000000000000-coreutils-stage4/bin/kill

Totally fascinating. But how do you deal with the situation where the user has two different versions of coreutils installed? That's why I assumed that pnames were basically just comments -- because they aren't unique within the store. When they aren't, nix-env and nix profile force you to reference them by the store-path of the deriving derivation.

I'll be more careful with pnames in the future now that I know they aren't just comment fields for human use. If I had known that it was possible to reference a store-path by pname I would have insisted on the fix going to staging-next instead of switching the base branch. At that point it still had at least six days to land and still prevent the bug from being visible on master.

I apologize for the breakage I caused here.