freshBootstrapTools: rename from stdenvBootstrapTools

[ghost]

Description of changes

This is a handoff of #182058

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • powerpc64le-linux
  • mips64el-linux
• 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
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[vcunat]

Are you sure that the performance of unionOfDisjoint won't be a bother here? This is used on jobsets with up to around 150k jobs.

[ghost]

Are you sure that the performance of unionOfDisjoint won't be a bother here?

No; in fact, I'm sure its performance is quite awful.

I will fix that.

[ghost]

Alright, 32ca89a351f9d86e525d130dd879b708dc081e40 should get us back to O(1) the same complexity as Bindings::get() and be as lazy as possible.

[vcunat]

I'm afraid that there's no practical point in doing the checks, even if they were for free.

• throw just adds to the huge unusable list that noone reads: https://hydra.nixos.org/jobset/nixpkgs/trunk#tabs-errors
  It would be good after we cleaned it up, but given how long it's been an issue...
• abort would be possible, but I'm not convinced that we want to stop building everything in case a collision happens

EDIT: ah, maybe this throw will drop everything, but that's the same point like for abort. Though maybe it will be OK-ish, if it's cheap enough.

[ghost]

EDIT: ah, maybe this throw will drop everything, but that's the same point like for abort. Though maybe it will be OK-ish, if it's cheap enough.

Won't ofborg CI also fail loudly for any PR that creates a collision?

[ghost]

• just adds to the huge unusable list that noone reads

I'm really not surprised that nobody reads it; that's not a job for humans. Hydra used to push notifications to the maintainers of the package from which they were thrown. I still can't figure out why that functionality was removed.

[ghost]

Won't ofborg CI also fail loudly for any PR that creates a collision?

Yep, it does:

ofborg-eval — Complete, with errors
ofborg-eval-nixpkgs-unstable-jobset — nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="b9909c1"; rev="b9909c13cfbe570edbe6990a64eded07d84bad7e"; } ./pkgs/t

error: attribute collision: stdenvBootstrapTools

       … while evaluating anonymous lambda

       at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/ofborg-evaluator-6/lib/attrsets.nix:630:14:

          629|     x // (mapAttrs
          630|       (name: val:
             |              ^
          631|         if hasAttr name x

[vcunat]

The Hydra notifications didn't work. Let me post a real example:

The status of Hydra job ‘nixos:release-18.03:nixpkgs.supercollider.x86_64-linux’ has changed from "Success" to "Dependency failed". For details, see

https://hydra.nixos.org/build/70685138

This may be due to 16697 commits by ...

And no, it was not notifying maintainers. Well the above was extreme case, but normally we have evals after a hundred commit or so, so this approach just can't work with the current popularity of NixPkgs.

Note that most of the throw messages in that list are completely legitimate and not errors, so there's even no point in notifying. (marked as broken or otherwise not supported on a given platform)

[vcunat]

TL;DR: someone would have to implement a new notification system that behaves reasonably in current NixPkgs.

[vcunat]

Oh, my bad, (mostly) it really was sent just to meta.maintainers, at least those that I received. Why did I think otherwise? Some huge cases happened by mistake or misconfiguration of jobsets, which was IIRC the last drop for Eelco killing the feature on Hydra.nixos.org (in 2018).

Since then I've been watching https://hydra.nixos.org/dashboard/vcunat@gmail.com instead, though it's less comfortable. I'm not sure if there's some better way.

[vcunat]

I've never been maintainer of supercollider and still I got that e-mail. I think hydra SW also has capability of notifying committers or maybe it was notifying both at once. There certainly are checkboxes for that in jobset config.

[toonn]

This looks pretty good to me. Seems like it doesn't revert the cross-build improvements.

This does not implement the loud warning when name collisions are introduced as was discussed in the previous PR, right?

Is there a way to test whether this fixes Hydra's bootstrap-tools job on a local machine (without Hydra)?

[ghost]

This does not implement the loud warning when name collisions are introduced as was discussed in the previous PR, right?

It does implement the loud warning. Actually it is more than a warning: it will throw, which will cause OfBorg to fail CI on any PR which attempts to introduce a collision:

https://github.com/NixOS/nixpkgs/pull/183072/files#diff-9d1ef0419a63492a93e8223ffbe653714958b8f791e68b72d4dbf8a26f67551aR209

Edit: apparently github doesn't provide any way to link to a specific line in an unmerged PR? Anyways search the "files changed" for unionOfDisjoint.

Is there a way to test whether this fixes Hydra's bootstrap-tools job on a local machine (without Hydra)?

I added instructions explaining how to do that in #181157

[ghost]

Force-push to 6d025a6 rebases to latest staging and squashes the history to be more readable. The overall diff created by the PR is unchanged.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-macos-monthly/12330/32

[toonn]

@amjoseph-nixpkgs, I'm testing this and unfortunately it turns out this is based on a commit after #185297 and that broke binutils on Darwin, which is a dependency of stdenv. Fix in #189894 though so if you base this off of a newer staging commit it should be fixed again and we can move this forward.

[toonn]

So, unionOfDisjoint is where the loud warning comes from?
Does this mean it'll give a single warning for the first job in the intersection that Hydra tries to evaluate, or does Hydra not stop anyway?

[ghost]

So, unionOfDisjoint is where the loud warning comes from?

Yes.

Does this mean it'll give a single warning for the first job in the intersection that Hydra tries to evaluate, or does Hydra not stop anyway?

OfBorg will block the merge by failing CI.

But yes, if for some reason we stop using OfBorg, the second line of defense is Hydra, which will fail any build in which evaluation touches an attr in the intersection (which is supposed to be empty) of the two attribute sets.

I'm investigating your previous comment.

[ghost]

Okay, rebased onto 2a452a4.

Is there some way to rebase a github PR without doing a force-push? Force-pushes are so error-prone. I've started draftifying my PRs before every force-push so I don't accidentally trigger mass notifications...

[toonn]

Ok, so I've verified that both the Hydra job and the freshBootstrapTools attribute do what they say on the tin. Looking good!

One question, Hydra will be fine with the separate attributes failing IIUC. It'll just show a bunch of failed jobs, right? But how does OfBorg deal with this. Does it only display the first attribute that fails to evaluate?

[ghost]

But how does OfBorg deal with this.

Like this.

I deliberately triggered the OfBorg failure there to make sure it worked and to capture an example of the error output.

[toonn]

And what if there's multiple colliding attributes. What I'm getting at is that I want to avoid this becoming a tennis match, where you fix a collision, re-run, fix the next collision, etc.

It would be great if the failure could list all collisions at once. Can't we merge all the failing attributes into one attrset that has all of them and then throw the full list?

[ghost]

It would be great if the failure could list all collisions at once.

Please read the discussion earlier in this PR; what you suggest is exactly what the original commit 90ab9cb2ef0fc434817d47fd3725e34951476b38 did. It has overhead and laziness problems, as discussed earlier in this PR.

[ghost]

@toonn, sorry, my comment was not only abrupt but also wrong. I apologize on both counts.

There is a way to print all collisions while still being maximally lazy and incurring only constant overhead. And it is, in fact, very slightly more efficient than 99da193 due to the way the Nix interpreter works (the complete set of names in an attrset is evaluated strictly whenever any of them is accessed for the first time).

Please see my latest push 037cf2f.

The trick is to use builtins.intersectAttrs builtin which can intersect attrset names in O(n) time (they are stored internally as sorted lists) rather than the O(nlogn) best we could hope for using interpreted Nix code, or the O(n^2) in 90ab9cb. builtins.intersectAttrs is C++ code, so the constant factors are better too.

Here's an example of what happens if you revert bf4312d and add an attribute named toilet to nonPackageJobs in release.nix:

$ nix-build pkgs/top-level/release.nix -A stdenvBootstrapTools.x86_64-linux
error: unionOfDisjoint: collision on stdenvBootstrapTools; complete list: stdenvBootstrapTools toilet

[toonn]

Really cool, I'll rebuild both the Hydra job and the attribute and then cause some collisions and then this is ready to be merged IMO.

[toonn]

Awesome, tested and this seems to solve the problem. Let's get Hydra building bootstrap-tools again : )

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-macos-monthly/12330/33