ci/github-script/prepare: identify real base branch by wolfgangwalther · Pull Request #435596 · NixOS/nixpkgs

wolfgangwalther · 2025-08-21T15:38:33Z

When a contributor mistakenly sets the wrong target branch for a Pull Request, this can lead to bad consequences for CI. Most prominent is the mass ping of codeowners, that is already handled in ci/request-reviews/verify-base-branch.sh (cc @infinisil). But there are other things that go wrong:

After eval, a mass ping of maintainers would still be possible, in theory. Practically, this doesn't happen, because we have a limit of 10 reviewer requests at the same time.
This will most often contain a change to ci/pinned.json, thus the full Eval matrix of all Lix/Nix versions will be run, burning a lot of resources.
The PR will be labelled with almost all labels that are available.

We can improve on the current situation with some API calls to determine the "best" merge-base for the current PR. We then consider this as the "real base". If the current target is not the real base, we fail the prepare step, which is early enough to prevent all other CI from running.

As a preparatory step, this also moves the no-channel-branch job into the prepare workflow with the same consequences: When your PR is targeting a channel branch, we're not running any CI anymore, unlike previously, where we ran everything and had one tiny job fail.

Things done

Tested locally.
Tested in my fork.
Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

wolfgangwalther · 2025-08-21T15:48:02Z

ci/github-script/prepare.js

The commands given in this comment should be the same as in verify-base-branch.sh earlier, but I'm hoping we can improve on them. I tried, and currently the --onto commit is not well chosen.

Example:

My branch is branched off of release-25.05.

My PR is targeting master.

I get this:

git rebase --onto $(git merge-base upstream/master HEAD) $(git merge-base upstream/release-25.05 HEAD)

While this "works" in the sense of "the PR will not contain bad commits afterwards", this is likely to lead to conflicts both during the rebase and then in the PR as well (the reverse of those earlier conflicts). That's because the merge-base for upstream/master HEAD is actually branch-off for release-25.05 and both branches (master and release) have advanced in the meantime. Considering backports, I might be rebasing over conflicts where a similar change has been made later in both branches.

In my concrete example, the only commit of my PR above was an update to ci/pinned.json. Now, with this rebase command, I hit a conflict, because this file didn't even exist in the merge-base that I rebased onto. I can't even solve this conflict properly, because when I keep the file away, my entire commit is gone - I only had this change!

What I think we should be telling users instead in this case would be to:

git rebase --onto upstream/master $(git merge-base upstream/release-25.05 HEAD)

This would work better. I can see where the previous approach likely came from, and that's when dealing with staging/master changes. In that case, it might be more helpful to actually target a merge-base between master and staging first, for fewer rebuilds. But even in that case... I'd say the Nr. 1 priority should be to not let the contributor lose changes due to too many conflicts. Thus, I'd say we should always do --onto <target>.

cc @infinisil, I'd like to know your opinion, too, as the one who wrote verify-base-branch.sh.

wolfgangwalther · 2025-08-21T16:00:11Z

Interesting CI failure for nixpkgs-vet:

building '/nix/store/c597vvqnp1c9d00p38pxp0gpax7jr3ax-nixpkgs-vet.drv'...
error: SQLite database '/build/tmp.ZkNPukokVI/db/db.sqlite' is busy
- Nix evaluation failed for some package in `pkgs/by-name`, see error above
This PR introduces the problems listed above. Please fix them before merging, otherwise the base branch would break.

I wonder.. whether that appears in the same case that the "symlink creation errors" have appeared in before...

MattSturgeon

SGTM

ci/github-script/prepare.js

.github/workflows/push.yml

ci/github-script/prepare.js

MattSturgeon

Thanks for improving #435596 (comment)

I have a couple wording suggestions for the new comment.

It'd also be great to get @infinisil's ack. Especially w.r.t. #435596 (comment).

Otherwise LGTM 🚀

ci/supportedBranches.js

wolfgangwalther · 2025-08-22T16:18:38Z

It'd also be great to get @infinisil's ack. Especially w.r.t. #435596 (comment).

Well, specifically for the comment, I only proposed the change, so far, but didn't implement it, yet. So that shouldn't block merging this. But I agree, it would be good if you, @infinisil, could have a quick look at the general approach for "is this the right branch?" verification here, having spent time on that topic more than enough yourself.

philiptaron

Approved with some thoughts.

.github/workflows/check.yml

.github/workflows/pr.yml

.github/workflows/check.yml

ci/github-script/commits.js

ci/github-script/prepare.js

wolfgangwalther · 2025-08-22T16:29:34Z

Improved wording based on the resolved comments.

wolfgangwalther · 2025-08-23T10:24:56Z

Something doesn't work well, yet: That's PRs for staging/haskell-updates.

Example: #429810

Doing this locally: ./run prepare NixOS nixpkgs 429810, I get:

This PR is for NixOS unstable.
The base branches for this PR are:
github: staging
candidates: haskell-updates
best candidate: haskell-updates
no pending review found
The PR's base branch is set to `staging`, but 136 commits from the `haskell-updates` branch are included. Make sure you know the [right base branch for your changes](https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#branch-conventions), then:
- If the changes should go to the `haskell-updates` branch, [change the base branch](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request).
- If the changes should go to the `staging` branch, rebase your PR onto the correct merge-base:
  ```bash
  # git rebase --onto $(git merge-base upstream/staging HEAD) $(git merge-base upstream/haskell-updates HEAD)
  git rebase --onto ce24ba99aa6bd01007c0eb16a2f8140b2fa69b3a 699736ec2896277f6532e1e3e961adb968f613a0
  git push --force-with-lease
  ```

Error: The PR contains commits from a different base.

Surprise... the haskell-updates branch includes commits from ... haskell-updates, so haskell-updates itself would be a better target :D

wolfgangwalther · 2025-08-23T10:36:28Z

I added an if (headClassification.type.includes('wip')) - only if that's the case, we will do all the "best merge-base" checks. If the head ref is a development branch, then we're merging between these branches and can't sensibly make such a check. Also, it's not required, because these can't be force pushed to anyway, so there is no way to change anything about it.

MattSturgeon · 2025-08-23T22:30:45Z

Surprise... the haskell-updates branch includes commits from ... haskell-updates, so haskell-updates itself would be a better target :D

Could we exclude the head_ref if the PR's repo owner is the same as the workflow's repo owner (i.e. not a PR from a fork)?

That way you would never attempt to match a branch against itself.

This is the very first step to extending the commits job to do more than just cherry-picks in the future: It could check reverts or merge commits, but also the commit message format and more. Of course, cherry-picks are still just checked on the stable branches as before. For now, this allows us to run the part that dismisses automated reviews automatically. This helps us when we do branch related checks in the prepare step, which would also create such a review. To avoid cluttering multiple reviews across a PR, we'll want all of these reviews to be handled by the same code, thus this change.

This allows re-using postReview in the next commit.

wolfgangwalther · 2025-08-24T10:17:27Z

Could we exclude the head_ref if the PR's repo owner is the same as the workflow's repo owner (i.e. not a PR from a fork)?

No, that won't be enough. I tried this at first, but this would still break on staging-next, which includes commits from staging, but merges into master. For that case, the script would still suggest staging as the best fit.

I found no other way than to say "if merging between development branches, don't check", which is what I implemented as "only check if the head branch is WIP". Note that every branch that is not a development or channel branch is a WIP branch. That includes not only the wip- prefixed branches, but also all other branches in the nixpkgs repo (revert-, backport-, all those branches without special meaning, e.g python-updates now), but also all branches from forks (no matter the name).

…nches This moves the no-channel-base check into the prepare script to exit early and prevent all of CI to run against those branches. We also provide better output by posting a "Changes Requested" review, using the existing infrastructure from the old cherry-picks check. The review will be dismissed automatically once the branch has been corrected, because the commits check will run and do it.

wolfgangwalther · 2025-08-24T16:08:55Z

I tested this against a wide variety of different PRs now, targeting different branches etc. - one more thing that doesn't work, yet: A PR targeting r-updates, currently results in:

The PR's base branch is set to `r-updates`, but -22 commits from the `master` branch are included. Make sure you know the [right base branch for your changes](https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#branch-conventions), then:
- If the changes should go to the `master` branch, [change the base branch](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request).
- If the changes should go to the `r-updates` branch, rebase your PR onto the correct merge-base:
  ```bash
  # git rebase --onto $(git merge-base upstream/r-updates HEAD) $(git merge-base upstream/master HEAD)
  git rebase --onto 06f1f61845fbba5f37167c392fba79a65614c53e 626516d1eb4a0acea3eae889f40c7ae6a8445a6b
  git push --force-with-lease
  ```

That's minus 22 commits, interesting :D

The solution to this is to always include the current target branch in the set of branches to check as well. Once I do that, the script will return r-updates as the best match as expected.

Tested again against all these different branches - this seems to work well now.

When a contributor mistakenly sets the wrong target branch for a Pull Request, this can lead to bad consequences for CI. Most prominent is the mass ping of codeowners, that is already handled in `ci/request-reviews/verify-base-branch.sh`. But there are other things that go wrong: - After eval, a mass ping of maintainers would still be possible, in theory. Practically, this doesn't happen, because we have a limit of 10 reviewer requests at the same time. - This will most often contain a change to `ci/pinned.json`, thus the full Eval matrix of all Lix/Nix versions will be run, burning a lot of resources. - The PR will be labelled with almost all labels that are available. We can improve on the current situation with some API calls to determine the "best" merge-base for the current PR. We then consider this as the "real base". If the current target is not the real base, we fail the prepare step, which is early enough to prevent all other CI from running.

MattSturgeon

Tested again against all these different branches - this seems to work well now.

SGTM, thanks for your due diligence!

nixpkgs-ci · 2025-08-25T12:05:53Z

Successfully created backport PR for release-25.05:

[Backport release-25.05] ci/github-script/prepare: identify real base branch #436758

philiptaron

LGTM also.

nix-owners bot requested review from MattSturgeon, Mic92, philiptaron and zowoq August 21, 2025 15:45

wolfgangwalther commented Aug 21, 2025

View reviewed changes

wolfgangwalther force-pushed the ci-prepare-real-base branch from c1f3b00 to 70942db Compare August 21, 2025 15:51

MattSturgeon reviewed Aug 21, 2025

View reviewed changes

wolfgangwalther mentioned this pull request Aug 22, 2025

workflows/test: init #435547

Merged

2 tasks

wolfgangwalther force-pushed the ci-prepare-real-base branch from 70942db to 262c3c3 Compare August 22, 2025 15:47

nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 22, 2025

wolfgangwalther force-pushed the ci-prepare-real-base branch from 262c3c3 to 4b8b775 Compare August 22, 2025 15:51

nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 22, 2025

MattSturgeon approved these changes Aug 22, 2025

View reviewed changes

ci/supportedBranches.js Outdated Show resolved Hide resolved

philiptaron approved these changes Aug 22, 2025

View reviewed changes

nixpkgs-ci bot added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Aug 22, 2025

wolfgangwalther force-pushed the ci-prepare-real-base branch from 4b8b775 to 60c8d5e Compare August 22, 2025 16:29

wolfgangwalther force-pushed the ci-prepare-real-base branch from 60c8d5e to 0f1d397 Compare August 23, 2025 10:35

wolfgangwalther force-pushed the ci-prepare-real-base branch from 0f1d397 to 40e4426 Compare August 23, 2025 10:41

ci/github-script/commits: split review function into separate file

c96b0e6

This allows re-using postReview in the next commit.

wolfgangwalther force-pushed the ci-prepare-real-base branch from 40e4426 to 74a0d67 Compare August 24, 2025 10:18

wolfgangwalther mentioned this pull request Aug 24, 2025

python313Packages.habluetooth: 4.0.2 -> 5.1.0, python313Packages.bleak-retry-connector: 4.3.0 -> 4.4.0 #436397

Closed

13 tasks

wolfgangwalther force-pushed the ci-prepare-real-base branch from 74a0d67 to 87d9b08 Compare August 24, 2025 16:09

MattSturgeon approved these changes Aug 24, 2025

View reviewed changes

wolfgangwalther merged commit 40d8532 into NixOS:master Aug 25, 2025
56 of 58 checks passed

wolfgangwalther deleted the ci-prepare-real-base branch August 25, 2025 12:05

nixpkgs-ci bot mentioned this pull request Aug 25, 2025

[Backport release-25.05] ci/github-script/prepare: identify real base branch #436758

Merged

1 task

github-actions bot added the 8.has: port to stable This PR already has a backport to the stable release. label Aug 25, 2025

philiptaron reviewed Aug 25, 2025

View reviewed changes

Uh oh!

Conversation

wolfgangwalther commented Aug 21, 2025

Things done

Uh oh!

wolfgangwalther Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

wolfgangwalther commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MattSturgeon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MattSturgeon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wolfgangwalther commented Aug 22, 2025

Uh oh!

philiptaron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfgangwalther commented Aug 22, 2025

Uh oh!

wolfgangwalther commented Aug 23, 2025

Uh oh!

wolfgangwalther commented Aug 23, 2025

Uh oh!

MattSturgeon commented Aug 23, 2025

Uh oh!

wolfgangwalther commented Aug 24, 2025

Uh oh!

wolfgangwalther commented Aug 24, 2025

Uh oh!

MattSturgeon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nixpkgs-ci bot commented Aug 25, 2025

Uh oh!

philiptaron left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wolfgangwalther commented Aug 21, 2025 •

edited

Loading

MattSturgeon left a comment •

edited

Loading

MattSturgeon left a comment •

edited

Loading