freshBootstrapTools: rename from stdenvBootstrapTools #182058

trofi · 2022-07-19T06:59:37Z

stdenvBootstrapTools was defined in both nixpkgs and releases
jobsets. This makes hydra view a bit confusing:

$ git grep -F 'stdenvBootstrapTools =' | cat
pkgs/top-level/all-packages.nix:  stdenvBootstrapTools = if stdenv.hostPlatform.isDarwin then
pkgs/top-level/release.nix:      stdenvBootstrapTools = with lib;

The change renames jobset to be distinct. At least it improves grep
experience.

Description of changes

Things done

ghost · 2022-07-19T08:59:02Z

The change renames jobset to be distinct.

If name collisions cause hydra.nixos.org to silently delete jobs we really ought to have CI prevent (or at least warn on) merges that would create collisions. Otherwise this will happen again...

trofi · 2022-07-20T21:51:22Z

Looking at https://hydra.nixos.org/jobset/nixpkgs/trunk#tabs-configuration trunk configuration I suspect it's a nix expression level collision where both release.nix and all-packages.nix define the same attribute name and it somehow gets merged incorrectly.

trofi · 2022-07-20T21:57:31Z

@ofborg eval

trofi · 2022-07-20T22:00:55Z

After the change we can see both present in top level:

$ nix repl -f pkgs/top-level/release.nix
Welcome to Nix 2.10.3. Type :? for help.

Loading installable ''...
Added 16795 variables.
nix-repl> stdenvBootstrapTools
{ aarch64-linux = { ... }; i686-linux = { ... }; x86_64-darwin = { ... }; x86_64-linux = { ... }; }

nix-repl> freshBootstrapTools
{ }

Not sure why freshBootstrapTools is empty. Possible another bug.

UPDATE: possibly intended: all attrsets are scrubbed down to empty (including pkgsCross).

`stdenvBootstrapTools` was defined in both `nixpkgs` and `releases` jobsets. This makes hydra view a bit confusing: $ git grep -F 'stdenvBootstrapTools =' | cat pkgs/top-level/all-packages.nix: stdenvBootstrapTools = if stdenv.hostPlatform.isDarwin then pkgs/top-level/release.nix: stdenvBootstrapTools = with lib; The change renames jobset to be distinct. At least it improves grep experience.

trofi · 2022-07-20T22:23:04Z

Looks like plain rename does not work anymore and fails eval. I wonder if 5643714 somehow relies on new meaning of stdenvBootstrapTools. I don't see how exacly how they are related. Maybe @alyssais knows?

I suspect things were broken for a while. Before this change:

# command from pkgs/os-specific/linux/busybox/default.nix:# nix build -f pkgs/top-level/release.nix stdenvBootstrapTools.x86_64-linux.dist
$ nix build -f pkgs/top-level/release.nix stdenvBootstrapTools.x86_64-linux.dist
error: attribute 'x86_64-linux' in selection path 'stdenvBootstrapTools.x86_64-linux.dist' not found

After the change:

$ nix build -f pkgs/top-level/release.nix stdenvBootstrapTools.x86_64-linux.dist
error: anonymous function at /home/slyfox/dev/git/nixpkgs-master/pkgs/stdenv/linux/make-bootstrap-tools.nix:1:1 called with unexpected argument 'localSystem'

       at /home/slyfox/dev/git/nixpkgs-master/pkgs/top-level/release.nix:169:16:

          168|             inherit
          169|               (import ../stdenv/linux/make-bootstrap-tools.nix {
             |                ^
          170|                 localSystem = { inherit system; };

trofi · 2022-07-20T22:31:45Z

I suspect we need to adapt below to avoid removed localSystem parameter:

      stdenvBootstrapTools = with lib;
        genAttrs systemsWithAnySupport
          (system: {
            inherit
              (import ../stdenv/linux/make-bootstrap-tools.nix {
                localSystem = { inherit system; };
              })
              dist test;
          })

This reverts commit 5643714. The change was based on e663518 ("all-packages.nix: add bootstrapTools to top-level.nix"). Unfortunately that change had intended side-effect in releases.nix that turned stdenvBootstrapTools into a no-op. As a result 5643714 "stdenvBootstrapTools: inherit {cross,local}System" change dropped the locaSystem argument without evaluation failure. Once the accidental change was fixed unexpected `localSystem` parameter started failing evals: $ nix build -f pkgs/top-level/release.nix stdenvBootstrapTools.x86_64-linux.dist error: anonymous function at pkgs/stdenv/linux/make-bootstrap-tools.nix:1:1 called with unexpected argument 'localSystem' at /home/slyfox/dev/git/nixpkgs-master/pkgs/top-level/release.nix:169:16: 168| inherit 169| (import ../stdenv/linux/make-bootstrap-tools.nix { | ^ 170| localSystem = { inherit system; }; Let's roll it back cleanly to restore stdenvBootstrapTools builds on hydra.

trofi · 2022-07-20T22:54:18Z

Looks like plain rename does not work anymore and fails eval. I wonder if 5643714 somehow relies on new meaning of stdenvBootstrapTools. I don't see how exacly how they are related. Maybe @alyssais knows?

Added a rollback CL to this PR (don't know how to fix it forward). Rollback restores nix build -f pkgs/top-level/release.nix stdenvBootstrapTools.x86_64-linux.dist evaluation for me. WDYT?

ghost · 2022-07-24T05:47:52Z

I have a stupid question: are the configuration files for the Hydra instance running on hydra.nixos.org posted somewhere (minus signing keys, of course) where I can view them? Those files seem like the natural starting point for figuring out the answer to problems like this one.

I think I've run into this problem before.

trofi · 2022-07-24T07:04:41Z

[caveat: I never created hydra jobsets] I think the job definition is clicked on hydra UI directly: you pick a repository, branch, expression and possibly a set of platforms to run on. https://hydra.nixos.org/jobset/nixpkgs/trunk#tabs-configuration

ghost · 2022-07-24T08:20:40Z

you pick a repository, branch, expression and possibly a set of platforms to run on

Yes, but where is the list of repository/branch/expressions that are run automatically? In other words, from your link:

Nix expression: pkgs/top-level/release.nix in input nixpkgs

Where did pkgs/top-level/release.nix come from?

Similarly, on this page, where did pkgs/top-level/release-cross.nix come from?

There must be a list of "entry points into nixpkgs" somewhere in the hydra.nixos.org configuration. I've never been able to find that list.

I suspect that if the root cause of the problem in this PR is some kind of name-collision, it would be in whatever merges these entry points together.

trofi · 2022-07-24T08:39:45Z

@trofi, I think you edited my comment instead of replying to it...

Gah! My apologies! I hope I did not butcher it while restoring it. No idea why I cleackied edit instead of Reply. Trying again as a reply:

Yes, but where is the list of repository/branch/expressions that are run automatically? In other words, from your link: Where did pkgs/top-level/release.nix come from?

Nix expression: pkgs/top-level/release.nix in input nixpkgs

I think people typed it in using web UI. I would guess administrator privileges give you that. json API exposes it as well: https://github.com/NixOS/hydra/blob/master/hydra-api.yaml#L148

trofi · 2022-07-24T08:45:26Z

I suspect that if the root cause of the problem in this PR is some kind of name-collision, it would be in whatever merges these entry points together.

I think all of merging happens right within pkgs/top-level/release.nix expression. It's a let jobs = { stdenvBootstrapTools = ... } // (mapTestOn ((packagePlatforms pkgs) // { ... } in jobs expression.

I guess (mapTestOn ((packagePlatforms pkgs) is what overrides initially locally defined stdenvBootstrapTools.

ghost · 2022-07-26T05:00:58Z

Personally I don't care what the package is called.

But I think the bug here isn't the package name, it's the fact that name collisions cause jobs to be dropped silently. Simply renaming the package is a band-aid; this will happen again somewhere else. The way it got this name in the first place is out of a desire to have the same name for jobs in both release and release-cross.

trofi · 2022-07-26T06:38:32Z

Personally I don't care what the package is called.

But I think the bug here isn't the package name, it's the fact that name collisions cause jobs to be dropped silently. Simply renaming the package is a band-aid; this will happen again somewhere else. The way it got this name in the first place is out of a desire to have the same name for jobs in both release and release-cross.

Is your suggestion to not rename the package and make it work? Or have an eval to fail loudly? (that would still require a rename).

Failing loudly should be easier by slightly changing the way attrs are merged. Say, some equivalent of

nix-repl> let as = { a = 1; }; in as // { a = 2; }
{ a = 2; }

nix-repl> let as = { a = 1; }; bs = { a = 2; }; in { inherit (as) a; inherit (bs) a; }
error: attribute 'a' already defined at (string):1:56

       at «string»:1:72:

            1| let as = { a = 1; }; bs = { a = 2; }; in { inherit (as) a; inherit (bs) a; }
             |

ghost · 2022-07-26T06:45:00Z

Is your suggestion to not rename the package and make it work? Or have an eval to fail loudly? (that would still require a rename).

Having it fail loudly and then changing the name to unfail it sounds great.

I'm still astonished that Hydra doesn't send notifications of any kind and people seem to be okay with that. That is pretty demotivating.

…local jobs This change exposes symbol override that accidentally caused job loss on hydra: $ nix repl ./release.nix error: jobs: Unexpected attribute collision between 'jobs' and 'pkgs': stdenvBootstrapTools Added assert makes sure attribute clashes would not be re-introduced.

trofi · 2022-07-26T21:30:26Z

Is your suggestion to not rename the package and make it work? Or have an eval to fail loudly? (that would still require a rename).

Having it fail loudly and then changing the name to unfail it sounds great.

Added a pkgs/top-level/release.nix: disallow symbol clash between 'pkgs' and local jobs change. When applied on master it fails as:

$ nix repl ./release.nix
error: jobs: Unexpected attribute collision between 'jobs' and 'pkgs': stdenvBootstrapTools

I'm still astonished that Hydra doesn't send notifications of any kind and people seem to be okay with that. That is pretty demotivating.

That's probably worth discussing with infrastrusture team. Maybe in NixOS/infra#51? I personally would prefer an RSS feed for package state changes I care about. I don't trust email delivery.

trofi · 2022-07-26T21:32:14Z

pkgs/top-level/release.nix: disallow symbol clash between 'pkgs' and local jobs is easier to review in "ignore whitespace" mode (git show -w):

--- a/pkgs/top-level/release.nix
+++ b/pkgs/top-level/release.nix
@@ -31,6 +31,7 @@ let
   ] (arch: builtins.elem "${arch}-darwin" systemsWithAnySupport);

   jobs =
+    let nonPackageJobs =
       { tarball = import ./make-tarball.nix { inherit pkgs nixpkgs officialRelease supportedSystems; };

         metrics = import ./metrics.nix { inherit pkgs nixpkgs; };
@@ -193,7 +194,19 @@ let
               };
             };

-    } // (mapTestOn ((packagePlatforms pkgs) // {
+       };
+    # Do not allow attribute collision between jobs inserted in
+    # 'nonPackageAttrs' and jobs pulled in from 'pkgs'.
+    # Conflicts usually cause silent job drops like in
+    #   https://github.com/NixOS/nixpkgs/pull/182058
+    nonPackageAttrs = lib.attrNames nonPackageJobs;
+    packageAttrs = lib.attrNames pkgs;
+    attributeCollisions = lib.intersectLists nonPackageAttrs packageAttrs;
+    in assert lib.assertMsg
+              (attributeCollisions == [])
+              "jobs: Unexpected attribute collision between 'jobs' and 'pkgs': ${lib.concatStringsSep " " attributeCollisions}";
+
+    nonPackageJobs // (mapTestOn ((packagePlatforms pkgs) // {
       haskell.compiler = packagePlatforms pkgs.haskell.compiler;
       haskellPackages = packagePlatforms pkgs.haskellPackages;
       idrisPackages = packagePlatforms pkgs.idrisPackages;

ghost

We really shouldn't revert commit 5643714 by @alyssais -- it was made for a good reason.

Please drop the revert (9c8e384) and cherry-pick e76b10c3acd2f7347b9131c175d54045063e026e instead.

Otherwise LGTM.

ghost · 2022-07-27T03:35:25Z

pkgs/top-level/release.nix

+              (attributeCollisions == [])
+              "jobs: Unexpected attribute collision between 'jobs' and 'pkgs': ${lib.concatStringsSep " " attributeCollisions}";
+
+    nonPackageJobs // (mapTestOn ((packagePlatforms pkgs) // {


If somebody comes along later and adds a third set to this update (a//b) nothing will warn them that they forgot to also add it to the separate check above (see previous comment).

Let's factor out the "union these attrsets while checking that they are disjoint" since it is something nixpkgs ought to use more often. Please consider cherry-picking 8b3eea55b66381a1bf3a3f0baa0ef48a5ed786f9

ghost · 2022-07-27T03:48:55Z

pkgs/top-level/release.nix

-              # Distribution only for now
-              inherit (bootstrap) dist;
-            };
+    let nonPackageJobs =


The multiply-nested lets here are getting hard to read, and not really necessary... please consider cherry-picking 2dd9e12478a3ed69a80ecc986696c7cc6059d4b1

ghost · 2022-07-27T04:42:20Z

I personally would prefer an RSS feed for package state changes I care about.

Any kind of notification would be great. Right now we have zero kinds.

trofi · 2022-07-27T06:24:09Z

I think this PR gets only more bloated code-wise. I'm declaring the defeat. I hope the failure mode is clear enough for others to fix it properly.

ghost · 2022-07-27T07:00:02Z

only more bloated code-wise

Whenever I re-indent code I haven't touched, git always makes me regret it...

trofi mentioned this pull request Jul 19, 2022

all-packages.nix: add stdenvBootstrapTools to top-level.nix #168650

Merged

7 tasks

trofi force-pushed the rename-stdenvBootstrapTools-to-freshBootstrapTools branch from 42be626 to 94f95bf Compare July 20, 2022 22:04

trofi requested a review from vcunat July 20, 2022 22:24

trofi requested a review from alyssais July 20, 2022 22:32

github-actions bot added the 6.topic: stdenv Standard environment label Jul 20, 2022

ofborg bot added 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 Jul 20, 2022

trofi mentioned this pull request Jul 22, 2022

stdenvBootstrapTools: inherit {cross,local}System #175484

Merged

13 tasks

toonn mentioned this pull request Jul 23, 2022

apple-m1 compiler optimisations for aarch64-darwin? #166401

Open

ghost reviewed Jul 27, 2022

View reviewed changes

trofi closed this Jul 27, 2022

trofi deleted the rename-stdenvBootstrapTools-to-freshBootstrapTools branch July 27, 2022 06:24

ghost mentioned this pull request Jul 27, 2022

freshBootstrapTools: rename from stdenvBootstrapTools #183072

Merged

10 tasks

reckenrode mentioned this pull request Aug 8, 2022

cmake: detect libc location at runtime #181431

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

freshBootstrapTools: rename from stdenvBootstrapTools #182058

freshBootstrapTools: rename from stdenvBootstrapTools #182058

trofi commented Jul 19, 2022 •

edited

Loading

ghost commented Jul 19, 2022

trofi commented Jul 20, 2022

trofi commented Jul 20, 2022

trofi commented Jul 20, 2022 •

edited

Loading

trofi commented Jul 20, 2022

trofi commented Jul 20, 2022

trofi commented Jul 20, 2022

ghost commented Jul 24, 2022 •

edited by ghost

Loading

trofi commented Jul 24, 2022

ghost commented Jul 24, 2022 •

edited by trofi

Loading

trofi commented Jul 24, 2022

trofi commented Jul 24, 2022

ghost commented Jul 26, 2022 •

edited by ghost

Loading

trofi commented Jul 26, 2022

ghost commented Jul 26, 2022

trofi commented Jul 26, 2022

trofi commented Jul 26, 2022 •

edited

Loading

ghost left a comment •

edited by ghost

Loading

ghost Jul 27, 2022

ghost Jul 27, 2022

ghost commented Jul 27, 2022 •

edited by ghost

Loading

trofi commented Jul 27, 2022

ghost commented Jul 27, 2022

freshBootstrapTools: rename from stdenvBootstrapTools #182058

freshBootstrapTools: rename from stdenvBootstrapTools #182058

Conversation

trofi commented Jul 19, 2022 • edited Loading

Description of changes

Things done

ghost commented Jul 19, 2022

trofi commented Jul 20, 2022

trofi commented Jul 20, 2022

trofi commented Jul 20, 2022 • edited Loading

trofi commented Jul 20, 2022

trofi commented Jul 20, 2022

trofi commented Jul 20, 2022

ghost commented Jul 24, 2022 • edited by ghost Loading

trofi commented Jul 24, 2022

ghost commented Jul 24, 2022 • edited by trofi Loading

trofi commented Jul 24, 2022

trofi commented Jul 24, 2022

ghost commented Jul 26, 2022 • edited by ghost Loading

trofi commented Jul 26, 2022

ghost commented Jul 26, 2022

trofi commented Jul 26, 2022

trofi commented Jul 26, 2022 • edited Loading

ghost left a comment • edited by ghost Loading

Choose a reason for hiding this comment

ghost Jul 27, 2022

Choose a reason for hiding this comment

ghost Jul 27, 2022

Choose a reason for hiding this comment

ghost commented Jul 27, 2022 • edited by ghost Loading

trofi commented Jul 27, 2022

ghost commented Jul 27, 2022

trofi commented Jul 19, 2022 •

edited

Loading

trofi commented Jul 20, 2022 •

edited

Loading

ghost commented Jul 24, 2022 •

edited by ghost

Loading

ghost commented Jul 24, 2022 •

edited by trofi

Loading

ghost commented Jul 26, 2022 •

edited by ghost

Loading

trofi commented Jul 26, 2022 •

edited

Loading

ghost left a comment •

edited by ghost

Loading

ghost commented Jul 27, 2022 •

edited by ghost

Loading