Use GHA eval to assign rebuild labels

[infinisil]

This implements labeling based on the number of changed paths, as follow-up work to #356023.

Ping @Mic92 @Aleksanaa @azuwis @TheGiraffe3 @JohnRTitor @emilazy

Things done

• Tested: Change hello tweag/nixpkgs#102

---

This work is funded by Tweag and Antithesis ✨

Add a 👍 reaction to pull requests you find important.

[Mic92]

Btw. Actionlint should help you to find syntax errors:

actionlint
.github/workflows/eval.yml:165:9: shellcheck reported issue in this script: SC1073:error:5:1: Couldn't parse this if expression. Fix to allow more checks [shellcheck]
    |
165 |         run: |
    |         ^~~~
.github/workflows/eval.yml:165:9: shellcheck reported issue in this script: SC1050:error:8:3: Expected 'then' [shellcheck]
    |
165 |         run: |
    |         ^~~~
.github/workflows/eval.yml:165:9: shellcheck reported issue in this script: SC1072:error:8:3: Expected 'then'. Fix any mentioned problems and try again [shellcheck]
    |
165 |         run: |
    |         ^~~~
.github/workflows/eval.yml:165:9: shellcheck reported issue in this script: SC1133:error:8:3: Unexpected start of line. If breaking lines, |/||/&& should be at the end of the previous one [shellcheck]
    |
165 |         run: |
    |         ^~~~

[JohnRTitor]

Would be nice to get a comment from GitHub actions on PRs:

These packages will be rebuilt:

- hello
- world
- nixos-install-tools

But I am fine with this being worked on a separate PR if this is a scope creep.

[FliegendeWurst]

Would be nice to get a comment from GitHub actions on PRs:

And if that is deemed to be too much clutter in the PR, the same output could also be added to the action log (hidden behind the "details" link).

[infinisil]

It works now! tweag#102 (comment)

[infinisil]

We have a first lucky customer: #359345 (comment) :D

[nixos-discourse]

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

https://discourse.nixos.org/t/infrastructure-announcement-the-future-of-ofborg-your-help-needed/56025/45

[github-actions]

Successfully created backport PR for release-24.11:

• [Backport release-24.11] Use GHA eval to assign rebuild labels #360258

[Atemu]

it's still the only thing that pings package maintainers

This is an idea I had a few days ago:

Why don't we just simply make the maintainers the codeowners of their package's by-name directory? That would ping them immediately via nix-owners without even needing to eval. The by-name structure allows for such simplifications.

We could realise that via an automated script which, given a package name, ensures that the set of code owners is the set of maintainers of the given package. We should have a CI check to assert that too.

For packages not in by-name yet, we could either keep using ofBorg (but disable it for by-name) and/or set code owners manually without CI check (or semi-automatically using meta.location + human check). That would also drastically reduce the load on ofBorg too as it could short-circuit on PRs that only touch by-name. That could perhaps be enough to make ofBorg acceptably fast again.

A much simpler alternative to keeping the codeowners/maintainers record in multiple places and keeping them in sync that just came to me would be to have each package declare its maintainers in a separate file that could be interpreted (or eval'd?) independently of the package by nix-owners. We'd then enable/disable ofBorg simply based on whether the package is in by-name and this maintainers declaration file exists.
This would be a first step towards package-eval-independent meta which we discussed in the contributors matrix channel a while ago.

[infinisil]

I think for now we should also mirror what ofborg does, which turns out to be fairly cheap and easy with all the existing work: tweag@be4026a (this doesn't quite work yet, but that's the gist of it).

Why don't we just simply make the maintainers the codeowners of their package's by-name directory?

Not a fan of duplicating the info in meta.maintainers. Could work but let's avoid it if possible.

That would also drastically reduce the load on ofBorg too

As hinted at, it turns out that figuring out maintainers for changed attributes is very cheap, it only depends on the eval to finish. I think we can live with the 5 minute delay on that.

declare its maintainers in a separate file that could be interpreted (or eval'd?) independently [...]
This would be a first step towards package-eval-independent meta which we discussed in the contributors matrix channel a while ago.

I like that idea, I've wanted a package-independent meta.nix for pkgs/by-name for a while, and this is a great use case for it!

[Atemu]

it turns out that figuring out maintainers for changed attributes is very cheap, it only depends on the eval to finish

What I'm saying is that we don't even need to depend on eval to finish. We can simply look at the files changed and use only that fact to determine who should be pinged; like code-owners.

If someone touches the files of a package I maintain, I'd want to be pinged regardless of whether it changes eval anyways.

On a second thought though, while it'd be nice, we wouldn't actually need package-eval-independent meta for this as we can simply assume that <package>.meta.maintainers exists and we know the attribute name via the by-name path of the changed files.

[infinisil]

If someone touches the files of a package I maintain, I'd want to be pinged regardless of whether it changes eval anyways.

Ohh good point, didn't think of that.

And yeah that should just work! :D

[infinisil]

Something else that I'm just realising: This labeling workflow only adds labels right now, it never removes them. So if the number of rebuilds changes, it will be very confusing..

[infinisil]

Minor problem with this workflow, causing it to fail on some PRs, simple fix though: #360274

[infinisil]

Something else that I'm just realising: This labeling workflow only adds labels right now, it never removes them. So if the number of rebuilds changes, it will be very confusing..

#360277

[FliegendeWurst]

ofborg used to count new packages as rebuilds too: (taken from a run in #356529)

{
  "attrdiff": {
    "added": [
      "siketyan-ghr"
    ],
    "removed": [],
    "changed": []
  },
  "rebuildsByKernel": {},
  "rebuildCountByKernel": {
    "linux": 0,
    "darwin": 0
  },
  "labels": [
    "10.rebuild-linux: 0",
    "10.rebuild-darwin: 0"
  ]
}

[JohnRTitor]

I'm currently working on the follow-up PR that implements that using GitHub Actions though, won't be finished this week though.

Perhaps we can make use of the Nix owners bot.

[wolfgangwalther]

ofborg used to count new packages as rebuilds too:

I just noticed this difference, too. But I think the new way is actually better - to make a decision whether to go into master or staging new packages shouldn't really count, right?

[FliegendeWurst]

It does matter, all packages have to be built.

[infinisil]

#361206 fixes this

[FliegendeWurst]

ofborg used to assign "rebuild: 1" labels too, instead of just "1-10". I had a look and it seems somewhat tricky to add onto the existing script.

[wolfgangwalther]

ofborg used to assign "rebuild: 1" labels too, instead of just "1-10". I had a look and it seems somewhat tricky to add onto the existing script.

It would be just this, right?

+      elif .value == 1 then
+        "1"
       elif .value <= 10 then
-        "1-10"
+        "2-10"

I don't think we need multiple labels, like ofborg does, in this case.

[FliegendeWurst]

Hmm... that would work, yes. It would make it more difficult to filter for "small" PRs though.