workflows/eval: Request reviews from changed package maintainers #366046

infinisil · 2024-12-18T02:56:26Z

Request reviews from maintainers of changed packages, mirroring what ofborg does, but without having to wait for it anymore. Part of #355847

Things done

Tested: Change hello Infinisil-s-Test-Organization/nixpkgs#37 (comment)

This work is funded by Tweag and Antithesis ✨

Add a 👍 reaction to pull requests you find important.

.github/workflows/eval.yml

ci/eval/compare/maintainers.nix

ci/eval/compare/utils.nix

Currently we need to rely on ofborg requesting reviews from package maintainers, which takes a while with ofborg's eval queue. Since recently we're doing faster evaluations with GitHub Actions, which contain all necessary information to determine reviewers of changed packages the same way ofborg does. This PR takes advantage of that.

…ged packages The handles can change over time and there's nothing guaranteeing the ones in the maintainer list are up-to-date. In comparison GitHub IDs never change.

infinisil · 2024-12-18T21:26:20Z

Feedback addressed, still works: Infinisil-s-Test-Organization#37 (comment)

JohnRTitor

Eval "Process" seems to be failing.

infinisil · 2024-12-19T00:46:12Z

@JohnRTitor A bit unfortunate but unfixable here, because the workflow for this PR itself uses the version from the base branch, but fetches the code from the PR, which now takes an extra argument not passed in the workflow

Mic92 · 2024-12-19T15:51:27Z

@JohnRTitor A bit unfortunate but unfixable here, because the workflow for this PR itself uses the version from the base branch, but fetches the code from the PR, which now takes an extra argument not passed in the workflow

This is the reference to look at: Infinisil-s-Test-Organization#37 (comment) instead

FliegendeWurst · 2024-12-20T14:01:38Z

.github/workflows/eval.yml

+          jq -r 'keys[]' comparison/maintainers.json \
+            | while read -r id; do gh api /user/"$id"; done \
+            | jq -s '{ reviewers: map(.login) }' \
+            > reviewers.json


Suggested change

jq -r 'keys[]' comparison/maintainers.json \

| while read -r id; do gh api /user/"$id"; done \

| jq -s '{ reviewers: map(.login) }' \

> reviewers.json

jq -r 'keys[]' comparison/maintainers.json \

| head -n15 \

| while read -r id; do gh api /user/"$id"; done \

| jq -s '{ reviewers: map(.login) }' \

> reviewers.json

GitHub apps are limited to 15 reviewer requests anyway, and this way treewide changes are handled better. Adding a head -n15 here should work, but I did not test.

Sounds like a good idea. Should be the list sorted, so it's always the same 15 reviewers?

I see two other options to handle treewides / PRs with more than 15 reviewers:

Don't request reviewers in this case.

Instead of requesting review, post a comment with all maintainers mentioned for this case.

I actually prefer the first option: When you make a treewide change and you get some random 15 reviewers, chances are high that those reviewers:

are not responding anyway

don't have much to say about that treewide change

So why notify many random people?

(Note that for a treewide, we very likely have already requested reviews from a bunch of people via OWNERS!)

For treewide changes it makes much more sense to pick your reviewers individually, specifically for that change.

I think there is also a limit with posting maintainers in comments. But we probably should avoid mass-pinging so many people and leave it to the pr authors/reviewers to select the right set of people.

So, we have now implemented requesting reviewers 1-by-1 to work around failures for some of them.

See #370749 for a run with that. Clearly the limit seems to apply for a single API request - and going 1-by-1 requested reviews from 30 reviewers there.

ofborg had limited itself to 10 per run manually. We'll need to do something about this as well.

.github/workflows/eval.yml

wolfgangwalther · 2024-12-21T12:19:46Z

ci/eval/compare/maintainers.nix

+  relevantFilenames =
+    drv:
+    (lib.lists.unique (
+      builtins.map (pos: lib.strings.removePrefix (toString ../..) pos.file) (
+        builtins.filter (x: x != null) [
+          (builtins.unsafeGetAttrPos "maintainers" (drv.meta or { }))
+          (builtins.unsafeGetAttrPos "src" drv)
+          # broken because name is always set by stdenv:
+          #    # A hack to make `nix-env -qa` and `nix search` ignore broken packages.
+          #    # TODO(@oxij): remove this assert when something like NixOS/nix#1771 gets merged into nix.
+          #    name = assert validity.handled; name + lib.optionalString
+          #(builtins.unsafeGetAttrPos "name" drv)
+          (builtins.unsafeGetAttrPos "pname" drv)
+          (builtins.unsafeGetAttrPos "version" drv)
+
+          # Use ".meta.position" for cases when most of the package is
+          # defined in a "common" section and the only place where
+          # reference to the file with a derivation the "pos"
+          # attribute.
+          #
+          # ".meta.position" has the following form:
+          #   "pkgs/tools/package-management/nix/default.nix:155"
+          # We transform it to the following:
+          #   { file = "pkgs/tools/package-management/nix/default.nix"; }
+          { file = lib.head (lib.splitString ":" (drv.meta.position or "")); }
+        ]
+      )
+    ));


I know this is an extension on top of the current ofborg behavior, but maybe a good time to discuss it:

Can we extend the "relevant file names" for a derivation to include some dependent files like patches or setup hooks?

I remember having to look up maintainers from the derivation manually when changing the setup-hook.sh.

I think some basic rules could be:

if the path ends in /default.nix or the path ends in /package.nix and is in /by-name/, then

add all files in the same directory (recursively) to the list of relevant files.

A random example that would benefit from it:

pkgs/by-name/me/meson/package.nix would include changes to all the patch and .sh files in the same folder.

I feel like we can first start with a minimal feature set and make it more complex later. Now since ofborg has been shutdown and will be re-instantiated with only a smaller feature set.

JohnRTitor

Okay LGTM, let's just merge this initial PR, we can improve later based on feedbacks recieved

nix-backports · 2025-01-02T08:49:30Z

Backport failed for release-24.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-24.11
git worktree add -d .worktree/backport-366046-to-release-24.11 origin/release-24.11
cd .worktree/backport-366046-to-release-24.11
git switch --create backport-366046-to-release-24.11
git cherry-pick -x b9d800d46814e44db4d0a0104141aeb54e0c3c9a b844cba4e6fcac4c4ceef81e481a5f93a01e4631

FliegendeWurst · 2025-01-02T10:13:06Z

It seems there are some limitations for who we can request reviews from.

gh: Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the NixOS/nixpkgs repository. (HTTP 422)

https://github.com/NixOS/nixpkgs/actions/runs/12580696560/job/35063365466

Mic92 · 2025-01-02T10:58:32Z

@FliegendeWurst #370186

FliegendeWurst · 2025-01-02T11:00:26Z

That only applies for the other issue #355847 (comment)

Mic92 · 2025-01-02T11:31:27Z

Ah right, we need a special github token than.

Mic92 · 2025-01-02T11:31:51Z

We should probably revert this pull request until these two issues are addressed.

FliegendeWurst · 2025-01-02T11:33:23Z

Ah right, we need a special github token than.

I don't think it's possible to request reviews from non-members. So a better token would not help.

JohnRTitor · 2025-01-02T12:37:29Z

I don't think it's possible to request reviews from non-members

Exactly, you either need to be a repo collaborator or an org member.

wolfgangwalther · 2025-01-02T21:05:09Z

We should probably revert this pull request until these two issues are addressed.

I don't think we need to revert it. Yes, we might have failing CI jobs, but ultimately we don't lose anything: The only thing that is not happening with those kind of failures is the request for maintainer reviews. Exactly the same outcome as when reverting the whole feature... except for the many cases where it already works right now.

Exactly, you either need to be a repo collaborator or an org member.

How can it actually happen that a listed maintainer is not an org member? IIUC, maintainers are automatically added to the org, right? That means they probably left the org manually.

But.. I checked the maintainers listed in the comparisons.zip/maintainers.json file in the GA run mentioned above: https://github.com/NixOS/nixpkgs/actions/runs/12580696560#artifacts

All of the 6 maintainers appear to be in the NixOS org to me. I don't understand why GH would throw an error here.

symphorien · 2025-01-02T21:40:53Z

A different failure mode in https://github.com/NixOS/nixpkgs/actions/runs/12588895670/job/35088361244


Run # Get all currently set rebuild labels
gh: Review cannot be requested from pull request author. (HTTP 422)
{"message":"Review cannot be requested from pull request author.","documentation_url":"https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request","status":"422"}

emilazy · 2025-01-02T23:52:32Z

Yes, we might have failing CI jobs, but ultimately we don't lose anything: The only thing that is not happening with those kind of failures is the request for maintainer reviews.

I get an email for every single PR I open telling me the job failed, and every PR has a big red ❌ on it that makes it look like eval failed. I think those are sufficient cause to revert this until it can be fixed.

GGG-KILLER · 2025-01-03T00:51:13Z

I get an email for every single PR I open telling me the job failed, and every PR has a big red ❌ on it that makes it look like eval failed. I think those are sufficient cause to revert this until it can be fixed.

Instead of reversing this change, can't we temporarily add a ||: to the line that adds the reviewers?
That way it'll keep requesting them in the cases it works and not block PRs when it fails.

emilazy · 2025-01-03T00:58:30Z

Sure, that also works if someone PRs it. I just think we shouldn’t be too hesitant to revert PRs that cause treewide issues, since we didn’t have this PR 16 hours ago anyway and it is cheap to merge again with fixes but leaving things merged in a broken state multiplies the problems they cause. Better to revert and bring it back without the issue than to leave the problem present while waiting for a fix.

GGG-KILLER · 2025-01-03T01:02:11Z

Sure, that also works if someone PRs it. I just think we shouldn’t be too hesitant to revert PRs that cause treewide issues, since we didn’t have this PR 16 hours ago anyway and it is cheap to merge again with fixes but leaving things merged in a broken state multiplies the problems they cause. Better to revert and bring it back without the issue than to leave the problem present while waiting for a fix.

I'm not against reversing this, I'd just prefer to have a somewhat working reviewer request automation instead of having to wait 2 business months for ofborg to request them itself, but either work for me.

I can't PR the hack fix right now but I'm willing to do it, if someone wants to revert it meanwhile I'm okay with it as well, better than having a CI check giving a false negative on a PR.

infinisil · 2025-01-03T01:20:01Z

I'm kind of on holidays, but I can make a quick fix, working on it..

Atemu · 2025-01-03T02:23:39Z

We can also just revert until you're back.

infinisil · 2025-01-03T03:10:06Z

Maybe that would've been better, but here's the fix, already tested: #370456 :)

nix-backports · 2025-01-03T20:30:53Z

Successfully created backport PR for release-24.11:

[Backport release-24.11] workflows/eval: Request reviews from changed package maintainers #370709

infinisil added the backport release-24.11 Backport PR automatically label Dec 18, 2024

infinisil requested review from azuwis, Mic92, wolfgangwalther, itepastra, GaetanLepage and JohnRTitor December 18, 2024 02:56

github-actions bot added 6.topic: policy discussion 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions labels Dec 18, 2024

infinisil mentioned this pull request Dec 18, 2024

Replacing ofBorg with GitHub Actions #355847

Open

7 tasks

nix-owners bot requested review from LeSuisse, mweinelt, philiptaron, risicle and zowoq December 18, 2024 02:58

TheGiraffe3 reviewed Dec 18, 2024

View reviewed changes

.github/workflows/eval.yml Show resolved Hide resolved

FliegendeWurst reviewed Dec 18, 2024

View reviewed changes

ci/eval/compare/maintainers.nix Outdated Show resolved Hide resolved

GaetanLepage reviewed Dec 18, 2024

View reviewed changes

ci/eval/compare/utils.nix Outdated Show resolved Hide resolved

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 Dec 18, 2024

infinisil added 2 commits December 18, 2024 22:13

workflows/eval: Use maintainer GitHub IDs for review requests of chan…

b844cba

…ged packages The handles can change over time and there's nothing guaranteeing the ones in the maintainer list are up-to-date. In comparison GitHub IDs never change.

infinisil force-pushed the maintainer-reviews branch from 26ba82d to b844cba Compare December 18, 2024 21:14

JohnRTitor reviewed Dec 18, 2024

View reviewed changes

FliegendeWurst reviewed Dec 20, 2024

View reviewed changes

wolfgangwalther reviewed Dec 21, 2024

View reviewed changes

.github/workflows/eval.yml Show resolved Hide resolved

wolfgangwalther reviewed Dec 21, 2024

View reviewed changes

JohnRTitor approved these changes Jan 2, 2025

View reviewed changes

JohnRTitor merged commit a69bc54 into NixOS:master Jan 2, 2025
36 of 37 checks passed

wolfgangwalther mentioned this pull request Jan 2, 2025

[Backport release-24.11] GitHub Actions PRs #369914

Merged

13 tasks

infinisil mentioned this pull request Jan 3, 2025

workflows/eval: fix maintainer requests #370456

Merged

3 tasks

infinisil deleted the maintainer-reviews branch January 3, 2025 03:09

wolfgangwalther added backport release-24.11 Backport PR automatically and removed backport release-24.11 Backport PR automatically labels Jan 3, 2025

nix-backports bot mentioned this pull request Jan 3, 2025

[Backport release-24.11] workflows/eval: Request reviews from changed package maintainers #370709

Draft

7 tasks

This was referenced Jan 4, 2025

treewide: remove obsolete aligned allocation workarounds #370749

Merged

ci/request-reviews: limit to 10 review requests #371115

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

workflows/eval: Request reviews from changed package maintainers #366046

workflows/eval: Request reviews from changed package maintainers #366046

infinisil commented Dec 18, 2024

infinisil commented Dec 18, 2024 •

edited

Loading

JohnRTitor left a comment

infinisil commented Dec 19, 2024 •

edited

Loading

Mic92 commented Dec 19, 2024 •

edited

Loading

FliegendeWurst Dec 20, 2024

Mic92 Dec 20, 2024

wolfgangwalther Dec 21, 2024

Mic92 Jan 1, 2025 •

edited

Loading

wolfgangwalther Jan 4, 2025

wolfgangwalther Dec 21, 2024

JohnRTitor Dec 25, 2024

Mic92 Jan 1, 2025

JohnRTitor left a comment

nix-backports bot commented Jan 2, 2025

FliegendeWurst commented Jan 2, 2025

Mic92 commented Jan 2, 2025

FliegendeWurst commented Jan 2, 2025

Mic92 commented Jan 2, 2025

Mic92 commented Jan 2, 2025

FliegendeWurst commented Jan 2, 2025

JohnRTitor commented Jan 2, 2025

wolfgangwalther commented Jan 2, 2025

symphorien commented Jan 2, 2025

emilazy commented Jan 2, 2025

GGG-KILLER commented Jan 3, 2025

emilazy commented Jan 3, 2025

GGG-KILLER commented Jan 3, 2025

infinisil commented Jan 3, 2025

Atemu commented Jan 3, 2025

infinisil commented Jan 3, 2025

nix-backports bot commented Jan 3, 2025

workflows/eval: Request reviews from changed package maintainers #366046

workflows/eval: Request reviews from changed package maintainers #366046

Conversation

infinisil commented Dec 18, 2024

Things done

infinisil commented Dec 18, 2024 • edited Loading

JohnRTitor left a comment

Choose a reason for hiding this comment

infinisil commented Dec 19, 2024 • edited Loading

Mic92 commented Dec 19, 2024 • edited Loading

FliegendeWurst Dec 20, 2024

Choose a reason for hiding this comment

Mic92 Dec 20, 2024

Choose a reason for hiding this comment

wolfgangwalther Dec 21, 2024

Choose a reason for hiding this comment

Mic92 Jan 1, 2025 • edited Loading

Choose a reason for hiding this comment

wolfgangwalther Jan 4, 2025

Choose a reason for hiding this comment

wolfgangwalther Dec 21, 2024

Choose a reason for hiding this comment

JohnRTitor Dec 25, 2024

Choose a reason for hiding this comment

Mic92 Jan 1, 2025

Choose a reason for hiding this comment

JohnRTitor left a comment

Choose a reason for hiding this comment

nix-backports bot commented Jan 2, 2025

FliegendeWurst commented Jan 2, 2025

Mic92 commented Jan 2, 2025

FliegendeWurst commented Jan 2, 2025

Mic92 commented Jan 2, 2025

Mic92 commented Jan 2, 2025

FliegendeWurst commented Jan 2, 2025

JohnRTitor commented Jan 2, 2025

wolfgangwalther commented Jan 2, 2025

symphorien commented Jan 2, 2025

emilazy commented Jan 2, 2025

GGG-KILLER commented Jan 3, 2025

emilazy commented Jan 3, 2025

GGG-KILLER commented Jan 3, 2025

infinisil commented Jan 3, 2025

Atemu commented Jan 3, 2025

infinisil commented Jan 3, 2025

nix-backports bot commented Jan 3, 2025

infinisil commented Dec 18, 2024 •

edited

Loading

infinisil commented Dec 19, 2024 •

edited

Loading

Mic92 commented Dec 19, 2024 •

edited

Loading

Mic92 Jan 1, 2025 •

edited

Loading