ci/github-script/prepare: identify real base branch (#435596)

This commit is contained in:
Wolfgang Walther
2025-08-25 12:05:12 +00:00
committed by GitHub
12 changed files with 296 additions and 256 deletions

View File

@@ -31,24 +31,7 @@ defaults:
shell: bash
jobs:
no-channel-base:
name: no channel base
if: contains(fromJSON(inputs.baseBranch).type, 'channel')
runs-on: ubuntu-24.04-arm
steps:
- run: |
cat <<EOF
The nixos-* and nixpkgs-* branches are pushed to by the channel
release script and should not be merged into directly.
Please target the equivalent release-* branch or master instead.
EOF
exit 1
cherry-pick:
if: |
github.event_name == 'pull_request' ||
(fromJSON(inputs.baseBranch).stable && !contains(fromJSON(inputs.headBranch).type, 'development'))
commits:
permissions:
pull-requests: write
runs-on: ubuntu-24.04-arm
@@ -68,16 +51,20 @@ jobs:
GH_TOKEN: ${{ github.token }}
run: gh api /rate_limit | jq
- name: Check cherry-picks
- name: Check commits
id: check
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1
env:
TARGETS_STABLE: ${{ fromJSON(inputs.baseBranch).stable && !contains(fromJSON(inputs.headBranch).type, 'development') }}
with:
script: |
const targetsStable = JSON.parse(process.env.TARGETS_STABLE)
require('./trusted/ci/github-script/commits.js')({
github,
context,
core,
dry: context.eventName == 'pull_request',
cherryPicks: context.eventName == 'pull_request' || targetsStable,
})
- name: Log current API rate limits

View File

@@ -23,6 +23,9 @@ permissions: {}
jobs:
prepare:
runs-on: ubuntu-24.04-arm
permissions:
# wrong branch review comment
pull-requests: write
outputs:
baseBranch: ${{ steps.prepare.outputs.base }}
headBranch: ${{ steps.prepare.outputs.head }}
@@ -44,6 +47,7 @@ jobs:
github,
context,
core,
dry: context.eventName == 'pull_request',
})
check:

View File

@@ -2,7 +2,6 @@ name: Push
on:
push:
# Keep this synced with ci/request-reviews/dev-branches.txt
branches:
- master
- staging

View File

@@ -1,7 +1,8 @@
module.exports = async ({ github, context, core, dry }) => {
module.exports = async ({ github, context, core, dry, cherryPicks }) => {
const { execFileSync } = require('node:child_process')
const { classify } = require('../supportedBranches.js')
const withRateLimit = require('./withRateLimit.js')
const { dismissReviews, postReview } = require('./reviews.js')
await withRateLimit({ github, core }, async (stats) => {
stats.prs = 1
@@ -16,7 +17,7 @@ module.exports = async ({ github, context, core, dry }) => {
run_id: context.runId,
per_page: 100,
})
).find(({ name }) => name.endsWith('Check / cherry-pick')).html_url +
).find(({ name }) => name.endsWith('Check / commits')).html_url +
'?pr=' +
pull_number
@@ -137,7 +138,11 @@ module.exports = async ({ github, context, core, dry }) => {
}
}
const commits = await github.paginate(github.rest.pulls.listCommits, {
// For now we short-circuit the list of commits when cherryPicks should not be checked.
// This will not run any checks, but still trigger the "dismiss reviews" part below.
const commits = !cherryPicks
? []
: await github.paginate(github.rest.pulls.listCommits, {
...context.repo,
pull_number,
})
@@ -185,38 +190,10 @@ module.exports = async ({ github, context, core, dry }) => {
// Only create step summary below in case of warnings or errors.
// Also clean up older reviews, when all checks are good now.
// An empty results array will always trigger this condition, which is helpful
// to clean up reviews created by the prepare step when on the wrong branch.
if (results.every(({ severity }) => severity === 'info')) {
if (!dry) {
await Promise.all(
(
await github.paginate(github.rest.pulls.listReviews, {
...context.repo,
pull_number,
})
)
.filter((review) => review.user.login === 'github-actions[bot]')
.map(async (review) => {
if (review.state === 'CHANGES_REQUESTED') {
await github.rest.pulls.dismissReview({
...context.repo,
pull_number,
review_id: review.id,
message: 'All cherry-picks are good now, thank you!',
})
}
await github.graphql(
`mutation($node_id:ID!) {
minimizeComment(input: {
classifier: RESOLVED,
subjectId: $node_id
})
{ clientMutationId }
}`,
{ node_id: review.node_id },
)
}),
)
}
await dismissReviews({ github, context, dry })
return
}
@@ -336,45 +313,9 @@ module.exports = async ({ github, context, core, dry }) => {
const body = core.summary.stringify()
core.summary.write()
const pendingReview = (
await github.paginate(github.rest.pulls.listReviews, {
...context.repo,
pull_number,
})
).find(
(review) =>
review.user.login === 'github-actions[bot]' &&
// If a review is still pending, we can just update this instead
// of posting a new one.
(review.state === 'CHANGES_REQUESTED' ||
// No need to post a new review, if an older one with the exact
// same content had already been dismissed.
review.body === body),
)
if (dry) {
if (pendingReview)
core.info(`pending review found: ${pendingReview.html_url}`)
else core.info('no pending review found')
} else {
// Either of those two requests could fail for very long comments. This can only happen
// with multiple commits all hitting the truncation limit for the diff. If you ever hit
// Posting a review could fail for very long comments. This can only happen with
// multiple commits all hitting the truncation limit for the diff. If you ever hit
// this case, consider just splitting up those commits into multiple PRs.
if (pendingReview) {
await github.rest.pulls.updateReview({
...context.repo,
pull_number,
review_id: pendingReview.id,
body,
})
} else {
await github.rest.pulls.createReview({
...context.repo,
pull_number,
event: 'REQUEST_CHANGES',
body,
})
}
}
await postReview({ github, context, core, dry, body })
})
}

View File

@@ -1,6 +1,7 @@
const { classify } = require('../supportedBranches.js')
const { postReview } = require('./reviews.js')
module.exports = async ({ github, context, core }) => {
module.exports = async ({ github, context, core, dry }) => {
const pull_number = context.payload.pull_request.number
for (const retryInterval of [5, 10, 20, 40, 80]) {
@@ -24,6 +25,160 @@ module.exports = async ({ github, context, core }) => {
const { base, head } = prInfo
const baseClassification = classify(base.ref)
core.setOutput('base', baseClassification)
console.log('base classification:', baseClassification)
const headClassification =
base.repo.full_name === head.repo.full_name
? classify(head.ref)
: // PRs from forks are always considered WIP.
{ type: ['wip'] }
core.setOutput('head', headClassification)
console.log('head classification:', headClassification)
if (baseClassification.type.includes('channel')) {
const { stable, version } = baseClassification
const correctBranch = stable ? `release-${version}` : 'master'
const body = [
'The `nixos-*` and `nixpkgs-*` branches are pushed to by the channel release script and should not be merged into directly.',
'',
`Please target \`${correctBranch}\` instead.`,
].join('\n')
await postReview({ github, context, core, dry, body })
throw new Error('The PR targets a channel branch.')
}
if (headClassification.type.includes('wip')) {
// In the following, we look at the git history to determine the base branch that
// this Pull Request branched off of. This is *supposed* to be the branch that it
// merges into, but humans make mistakes. Once that happens we want to error out as
// early as possible.
// To determine the "real base", we are looking at the merge-base of primary development
// branches and the head of the PR. The merge-base which results in the least number of
// commits between that base and head is the real base. We can query for this via GitHub's
// REST API. There can be multiple candidates for the real base with the same number of
// commits. In this case we pick the "best" candidate by a fixed ordering of branches,
// as defined in ci/supportedBranches.js.
//
// These requests take a while, when comparing against the wrong release - they need
// to look at way more than 10k commits in that case. Thus, we try to minimize the
// number of requests across releases:
// - First, we look at the primary development branches only: master and release-xx.yy.
// The branch with the fewest commits gives us the release this PR belongs to.
// - We then compare this number against the relevant staging branches for this release
// to find the exact branch that this belongs to.
// All potential development branches
const branches = (
await github.paginate(github.rest.repos.listBranches, {
...context.repo,
per_page: 100,
})
).map(({ name }) => classify(name))
// All stable primary development branches from latest to oldest.
const releases = branches
.filter(({ stable, type }) => type.includes('primary') && stable)
.sort((a, b) => b.version.localeCompare(a.version))
async function mergeBase({ branch, order, version }) {
const { data } = await github.rest.repos.compareCommitsWithBasehead({
...context.repo,
basehead: `${branch}...${head.sha}`,
// Pagination for this endpoint is about the commits listed, which we don't care about.
per_page: 1,
// Taking the second page skips the list of files of this changeset.
page: 2,
})
return {
branch,
order,
version,
commits: data.total_commits,
sha: data.merge_base_commit.sha,
}
}
// Multiple branches can be OK at the same time, if the PR was created of a merge-base,
// thus storing as array.
let candidates = [await mergeBase(classify('master'))]
for (const release of releases) {
const nextCandidate = await mergeBase(release)
if (candidates[0].commits === nextCandidate.commits)
candidates.push(nextCandidate)
if (candidates[0].commits > nextCandidate.commits)
candidates = [nextCandidate]
// The number 10000 is principally arbitrary, but the GitHub API returns this value
// when the number of commits exceeds it in reality. The difference between two stable releases
// is certainly more than 10k commits, thus this works for us as well: If we're targeting
// a wrong release, the number *will* be 10000.
if (candidates[0].commits < 10000) break
}
core.info(`This PR is for NixOS ${candidates[0].version}.`)
// Secondary development branches for the selected version only.
const secondary = branches.filter(
({ branch, type, version }) =>
type.includes('secondary') && version === candidates[0].version,
)
// Make sure that we always check the current target as well, even if its a WIP branch.
// If it's not a WIP branch, it was already included in either releases or secondary.
if (classify(base.ref).type.includes('wip')) {
secondary.push(classify(base.ref))
}
for (const branch of secondary) {
const nextCandidate = await mergeBase(branch)
if (candidates[0].commits === nextCandidate.commits)
candidates.push(nextCandidate)
if (candidates[0].commits > nextCandidate.commits)
candidates = [nextCandidate]
}
// If the current branch is among the candidates, this is always better than any other,
// thus sorting at -1.
candidates = candidates
.map((candidate) =>
candidate.branch === base.ref
? { ...candidate, order: -1 }
: candidate,
)
.sort((a, b) => a.order - b.order)
const best = candidates.at(0)
core.info('The base branches for this PR are:')
core.info(`github: ${base.ref}`)
core.info(
`candidates: ${candidates.map(({ branch }) => branch).join(',')}`,
)
core.info(`best candidate: ${best.branch}`)
if (best.branch !== base.ref) {
const current = await mergeBase(classify(base.ref))
const body = [
`The PR's base branch is set to \`${current.branch}\`, but ${current.commits === 10000 ? 'at least 10000' : current.commits - best.commits} commits from the \`${best.branch}\` 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 \`${best.branch}\` 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 \`${current.branch}\` branch, rebase your PR onto the correct merge-base:`,
' ```bash',
` # git rebase --onto $(git merge-base upstream/${current.branch} HEAD) $(git merge-base upstream/${best.branch} HEAD)`,
` git rebase --onto ${current.sha} ${best.sha}`,
` git push --force-with-lease`,
' ```',
].join('\n')
await postReview({ github, context, core, dry, body })
throw new Error(`The PR contains commits from a different base.`)
}
}
let mergedSha, targetSha
if (prInfo.mergeable) {
@@ -39,7 +194,7 @@ module.exports = async ({ github, context, core }) => {
} else {
core.warning('The PR has a merge conflict.')
mergedSha = prInfo.head.sha
mergedSha = head.sha
targetSha = (
await github.rest.repos.compareCommitsWithBasehead({
...context.repo,
@@ -56,18 +211,6 @@ module.exports = async ({ github, context, core }) => {
core.setOutput('systems', require('../supportedSystems.json'))
const baseClassification = classify(base.ref)
core.setOutput('base', baseClassification)
console.log('base classification:', baseClassification)
const headClassification =
base.repo.full_name === head.repo.full_name
? classify(head.ref)
: // PRs from forks are always considered WIP.
{ type: ['wip'] }
core.setOutput('head', headClassification)
console.log('head classification:', headClassification)
const files = (
await github.paginate(github.rest.pulls.listFiles, {
...context.repo,

View File

@@ -0,0 +1,85 @@
async function dismissReviews({ github, context, dry }) {
const pull_number = context.payload.pull_request.number
if (dry) {
return
}
await Promise.all(
(
await github.paginate(github.rest.pulls.listReviews, {
...context.repo,
pull_number,
})
)
.filter((review) => review.user.login === 'github-actions[bot]')
.map(async (review) => {
if (review.state === 'CHANGES_REQUESTED') {
await github.rest.pulls.dismissReview({
...context.repo,
pull_number,
review_id: review.id,
message: 'All good now, thank you!',
})
}
await github.graphql(
`mutation($node_id:ID!) {
minimizeComment(input: {
classifier: RESOLVED,
subjectId: $node_id
})
{ clientMutationId }
}`,
{ node_id: review.node_id },
)
}),
)
}
async function postReview({ github, context, core, dry, body }) {
const pull_number = context.payload.pull_request.number
const pendingReview = (
await github.paginate(github.rest.pulls.listReviews, {
...context.repo,
pull_number,
})
).find(
(review) =>
review.user.login === 'github-actions[bot]' &&
// If a review is still pending, we can just update this instead
// of posting a new one.
(review.state === 'CHANGES_REQUESTED' ||
// No need to post a new review, if an older one with the exact
// same content had already been dismissed.
review.body === body),
)
if (dry) {
if (pendingReview)
core.info(`pending review found: ${pendingReview.html_url}`)
else core.info('no pending review found')
core.info(body)
} else {
if (pendingReview) {
await github.rest.pulls.updateReview({
...context.repo,
pull_number,
review_id: pendingReview.id,
body,
})
} else {
await github.rest.pulls.createReview({
...context.repo,
pull_number,
event: 'REQUEST_CHANGES',
body,
})
}
}
}
module.exports = {
dismissReviews,
postReview,
}

View File

@@ -7,7 +7,7 @@ import { program } from 'commander'
import * as core from '@actions/core'
import { getOctokit } from '@actions/github'
async function run(action, owner, repo, pull_number, dry = true) {
async function run(action, owner, repo, pull_number, options = {}) {
const token = execSync('gh auth token', { encoding: 'utf-8' }).trim()
const github = getOctokit(token)
@@ -35,7 +35,8 @@ async function run(action, owner, repo, pull_number, dry = true) {
},
},
core,
dry,
dry: true,
...options,
})
}
@@ -45,9 +46,10 @@ program
.argument('<owner>', 'Owner of the GitHub repository to check (Example: NixOS)')
.argument('<repo>', 'Name of the GitHub repository to check (Example: nixpkgs)')
.argument('<pr>', 'Number of the Pull Request to check')
.action(async (owner, repo, pr) => {
.option('--no-dry', 'Make actual modifications')
.action(async (owner, repo, pr, options) => {
const prepare = (await import('./prepare.js')).default
run(prepare, owner, repo, pr)
run(prepare, owner, repo, pr, options)
})
program
@@ -56,9 +58,10 @@ program
.argument('<owner>', 'Owner of the GitHub repository to check (Example: NixOS)')
.argument('<repo>', 'Name of the GitHub repository to check (Example: nixpkgs)')
.argument('<pr>', 'Number of the Pull Request to check')
.action(async (owner, repo, pr) => {
.option('--no-cherry-picks', 'Do not expect cherry-picks.')
.action(async (owner, repo, pr, options) => {
const commits = (await import('./commits.js')).default
run(commits, owner, repo, pr)
run(commits, owner, repo, pr, options)
})
program
@@ -74,7 +77,7 @@ program
try {
process.env.GITHUB_WORKSPACE = tmp
process.chdir(tmp)
run(labels, owner, repo, pr, options.dry)
run(labels, owner, repo, pr, options)
} finally {
rmSync(tmp, { recursive: true })
}

View File

@@ -17,15 +17,12 @@ stdenvNoCC.mkDerivation {
./get-code-owners.sh
./request-reviewers.sh
./request-code-owner-reviews.sh
./verify-base-branch.sh
./dev-branches.txt
];
};
nativeBuildInputs = [ makeWrapper ];
dontBuild = true;
installPhase = ''
mkdir -p $out/bin
mv dev-branches.txt $out/bin
for bin in *.sh; do
mv "$bin" "$out/bin"
wrapProgram "$out/bin/$bin" \

View File

@@ -1,8 +0,0 @@
# Trusted development branches:
# These generally require PRs to update and are built by Hydra.
# Keep this synced with the branches in .github/workflows/eval.yml
master
staging
release-*
staging-*
haskell-updates

View File

@@ -1,6 +1,6 @@
#!/usr/bin/env bash
# Requests reviews for a PR after verifying that the base branch is correct
# Requests reviews for a PR
set -euo pipefail
tmp=$(mktemp -d)
@@ -11,14 +11,6 @@ log() {
echo "$@" >&2
}
effect() {
if [[ -n "${DRY_MODE:-}" ]]; then
log "Skipping in dry mode:" "${@@Q}"
else
"$@"
fi
}
if (( $# < 3 )); then
log "Usage: $0 GITHUB_REPO PR_NUMBER OWNERS_FILE"
exit 1
@@ -63,20 +55,6 @@ git -C "$tmp/nixpkgs.git" config remote.fork.promisor true
git -C "$tmp/nixpkgs.git" fetch --no-tags fork "$prBranch"
headRef=$(git -C "$tmp/nixpkgs.git" rev-parse refs/remotes/fork/"$prBranch")
log "Checking correctness of the base branch"
if ! "$SCRIPT_DIR"/verify-base-branch.sh "$tmp/nixpkgs.git" "$headRef" "$baseRepo" "$baseBranch" "$prRepo" "$prBranch" | tee "$tmp/invalid-base-error" >&2; then
log "Posting error as comment"
if ! response=$(effect gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
-H "X-GitHub-Api-Version: 2022-11-28" \
"/repos/$baseRepo/issues/$prNumber/comments" \
-F "body=@$tmp/invalid-base-error"); then
log "Failed to post the comment: $response"
fi
exit 1
fi
log "Requesting reviews from code owners"
"$SCRIPT_DIR"/get-code-owners.sh "$tmp/nixpkgs.git" "$ownersFile" "$baseBranch" "$headRef" | \
"$SCRIPT_DIR"/request-reviewers.sh "$baseRepo" "$prNumber" "$prAuthor"

View File

@@ -1,104 +0,0 @@
#!/usr/bin/env bash
# Check that a PR doesn't include commits from other development branches.
# Fails with next steps if it does
set -euo pipefail
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' exit
SCRIPT_DIR=$(dirname "$0")
log() {
echo "$@" >&2
}
# Small helper to check whether an element is in a list
# Usage: `elementIn foo "${list[@]}"`
elementIn() {
local e match=$1
shift
for e; do
if [[ "$e" == "$match" ]]; then
return 0
fi
done
return 1
}
if (( $# < 6 )); then
log "Usage: $0 LOCAL_REPO HEAD_REF BASE_REPO BASE_BRANCH PR_REPO PR_BRANCH"
exit 1
fi
localRepo=$1
headRef=$2
baseRepo=$3
baseBranch=$4
prRepo=$5
prBranch=$6
# All development branches
devBranchPatterns=()
while read -r pattern; do
if [[ "$pattern" != '#'* ]]; then
devBranchPatterns+=("$pattern")
fi
done < "$SCRIPT_DIR/dev-branches.txt"
git -C "$localRepo" branch --list --format "%(refname:short)" "${devBranchPatterns[@]}" > "$tmp/dev-branches"
readarray -t devBranches < "$tmp/dev-branches"
if [[ "$baseRepo" == "$prRepo" ]] && elementIn "$prBranch" "${devBranches[@]}"; then
log "This PR merges $prBranch into $baseBranch, no commit check necessary"
exit 0
fi
# The current merge base of the PR
prMergeBase=$(git -C "$localRepo" merge-base "$baseBranch" "$headRef")
log "The PR's merge base with the base branch $baseBranch is $prMergeBase"
# This is purely for debugging
git -C "$localRepo" rev-list --reverse "$baseBranch".."$headRef" > "$tmp/pr-commits"
log "The PR includes these $(wc -l < "$tmp/pr-commits") commits:"
cat <"$tmp/pr-commits" >&2
for testBranch in "${devBranches[@]}"; do
if [[ -z "$(git -C "$localRepo" rev-list -1 --since="1 month ago" "$testBranch")" ]]; then
log "Not checking $testBranch, was inactive for the last month"
continue
fi
log "Checking if commits from $testBranch are included in the PR"
# We need to check for any commits that are in the PR which are also in the test branch.
# We could check each commit from the PR individually, but that's unnecessarily slow.
#
# This does _almost_ what we want: `git rev-list --count headRef testBranch ^baseBranch`,
# except that it includes commits that are reachable from _either_ headRef or testBranch,
# instead of restricting it to ones reachable by both
# Easily fixable though, because we can use `git merge-base testBranch headRef`
# to get the least common ancestor (aka merge base) commit reachable by both.
# If the branch being tested is indeed the right base branch,
# this is then also the commit from that branch that the PR is based on top of.
testMergeBase=$(git -C "$localRepo" merge-base "$testBranch" "$headRef")
# And then use the `git rev-list --count`, but replacing the non-working
# `headRef testBranch` with the merge base of the two.
extraCommits=$(git -C "$localRepo" rev-list --count "$testMergeBase" ^"$baseBranch")
if (( extraCommits != 0 )); then
log -e "\e[33m"
echo "The PR's base branch is set to $baseBranch, but $extraCommits commits from the $testBranch 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:"
echo "- If the changes should go to the $testBranch 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) to $testBranch"
echo "- If the changes should go to the $baseBranch branch, rebase your PR onto the merge base with the $baseBranch branch:"
echo " \`\`\`bash"
echo " # git rebase --onto \$(git merge-base upstream/$baseBranch HEAD) \$(git merge-base upstream/$testBranch HEAD)"
echo " git rebase --onto $prMergeBase $testMergeBase"
echo " git push --force-with-lease"
echo " \`\`\`"
log -e "\e[m"
exit 1
fi
done
log "Base branch is correct, no commits from development branches are included"

View File

@@ -13,6 +13,16 @@ const typeConfig = {
nixpkgs: ['channel'],
}
// "order" ranks the development branches by how likely they are the intended base branch
// when they are an otherwise equally good fit according to ci/github-script/prepare.js.
const orderConfig = {
master: 0,
release: 1,
staging: 2,
'haskell-updates': 3,
'staging-next': 4,
}
function split(branch) {
return {
...branch.match(
@@ -24,8 +34,11 @@ function split(branch) {
function classify(branch) {
const { prefix, version } = split(branch)
return {
branch,
order: orderConfig[prefix] ?? Infinity,
stable: (version ?? 'unstable') !== 'unstable',
type: typeConfig[prefix] ?? ['wip'],
version: version ?? 'unstable',
}
}
@@ -39,6 +52,7 @@ if (!module.parent) {
}
testSplit('master')
testSplit('release-25.05')
testSplit('staging')
testSplit('staging-next')
testSplit('staging-25.05')
testSplit('staging-next-25.05')
@@ -55,6 +69,7 @@ if (!module.parent) {
}
testClassify('master')
testClassify('release-25.05')
testClassify('staging')
testClassify('staging-next')
testClassify('staging-25.05')
testClassify('staging-next-25.05')