From af33fe08ff28ed0f07ec64d46e685a5030cfb847 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Wed, 28 May 2025 19:22:31 +0200 Subject: [PATCH 1/6] ci/check-cherry-picks: remove unused condition The command substitution style we recently switched to strips trailing newlines, so we don't need to check for empty lines anymore. (cherry picked from commit 4c345b2fb4857b46b97c6ef89464f1be4d427c2b) (cherry picked from commit 6388b8f4b57b93b822e6a812e40d13cdf44ea861) --- ci/check-cherry-picks.sh | 3 --- 1 file changed, 3 deletions(-) diff --git a/ci/check-cherry-picks.sh b/ci/check-cherry-picks.sh index e7c9e289b22b..cc6c19a0ac43 100755 --- a/ci/check-cherry-picks.sh +++ b/ci/check-cherry-picks.sh @@ -20,9 +20,6 @@ remote="$(git remote -v | grep -i 'NixOS/nixpkgs' | head -n1 | cut -f1 || true)" commits="$(git rev-list --reverse "$1..$2")" while read -r new_commit_sha ; do - if [ -z "$new_commit_sha" ] ; then - continue # skip empty lines - fi if [ "$GITHUB_ACTIONS" = 'true' ] ; then echo "::group::Commit $new_commit_sha" else From 5c98c6b0817848e8ab7ad69c80fc0f54c13b1269 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Wed, 28 May 2025 19:56:33 +0200 Subject: [PATCH 2/6] ci/check-cherry-picks: set -u (cherry picked from commit 30600ac6d13c0533667edbec8f2bf9977cf1039e) (cherry picked from commit fff9467433f23673d9142b2cf506bbd8c3eb3c4c) --- ci/check-cherry-picks.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ci/check-cherry-picks.sh b/ci/check-cherry-picks.sh index cc6c19a0ac43..cd67c43d2822 100755 --- a/ci/check-cherry-picks.sh +++ b/ci/check-cherry-picks.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash # Find alleged cherry-picks -set -eo pipefail +set -euo pipefail if [ $# != "2" ] ; then echo "usage: check-cherry-picks.sh base_rev head_rev" @@ -20,7 +20,7 @@ remote="$(git remote -v | grep -i 'NixOS/nixpkgs' | head -n1 | cut -f1 || true)" commits="$(git rev-list --reverse "$1..$2")" while read -r new_commit_sha ; do - if [ "$GITHUB_ACTIONS" = 'true' ] ; then + if [ -v GITHUB_ACTIONS ] ; then echo "::group::Commit $new_commit_sha" else echo "=================================================" @@ -34,7 +34,7 @@ while read -r new_commit_sha ; do | grep -Eoi -m1 '[0-9a-f]{40}' || true ) if [ -z "$original_commit_sha" ] ; then - if [ "$GITHUB_ACTIONS" = 'true' ] ; then + if [ -v GITHUB_ACTIONS ] ; then echo ::endgroup:: echo -n "::error ::" else @@ -72,7 +72,7 @@ while read -r new_commit_sha ; do ' if $range_diff_common --no-color 2> /dev/null | grep -E '^ {4}[+-]{2}' > /dev/null ; then - if [ "$GITHUB_ACTIONS" = 'true' ] ; then + if [ -v GITHUB_ACTIONS ] ; then echo ::endgroup:: echo -n "::warning ::" else @@ -88,7 +88,7 @@ while read -r new_commit_sha ; do else echo " ✔ $original_commit_sha highly similar to $new_commit_sha" $range_diff_common --color - [ "$GITHUB_ACTIONS" = 'true' ] && echo ::endgroup:: + [ -v GITHUB_ACTIONS ] && echo ::endgroup:: fi # move on to next commit @@ -97,7 +97,7 @@ while read -r new_commit_sha ; do done <<< "$branches" done - if [ "$GITHUB_ACTIONS" = 'true' ] ; then + if [ -v GITHUB_ACTIONS ] ; then echo ::endgroup:: echo -n "::error ::" else From c86445519fe591918da41c2c22a53f084b02ae4a Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Wed, 28 May 2025 20:03:54 +0200 Subject: [PATCH 3/6] ci/check-cherry-picks: refactor output / logging This way it's a bit more centralized and easier to extend. (cherry picked from commit 6054a8f2175e0c0b0bf18f76fc080b08a3db52a7) (cherry picked from commit 290b336eb20ee4a5238e6557259b2062238dbb92) --- ci/check-cherry-picks.sh | 58 +++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 25 deletions(-) diff --git a/ci/check-cherry-picks.sh b/ci/check-cherry-picks.sh index cd67c43d2822..4d91502e3551 100755 --- a/ci/check-cherry-picks.sh +++ b/ci/check-cherry-picks.sh @@ -19,6 +19,29 @@ remote="$(git remote -v | grep -i 'NixOS/nixpkgs' | head -n1 | cut -f1 || true)" commits="$(git rev-list --reverse "$1..$2")" +log() { + type="$1" + shift 1 + + local -A prefix + prefix[success]=" ✔ " + if [ -v GITHUB_ACTIONS ]; then + prefix[warning]="::warning::" + prefix[error]="::error::" + else + prefix[warning]=" ⚠ " + prefix[error]=" ✘ " + fi + + echo "${prefix[$type]}$@" +} + +endgroup() { + if [ -v GITHUB_ACTIONS ] ; then + echo ::endgroup:: + fi +} + while read -r new_commit_sha ; do if [ -v GITHUB_ACTIONS ] ; then echo "::group::Commit $new_commit_sha" @@ -34,13 +57,8 @@ while read -r new_commit_sha ; do | grep -Eoi -m1 '[0-9a-f]{40}' || true ) if [ -z "$original_commit_sha" ] ; then - if [ -v GITHUB_ACTIONS ] ; then - echo ::endgroup:: - echo -n "::error ::" - else - echo -n " ✘ " - fi - echo "Couldn't locate original commit hash in message" + endgroup + log error "Couldn't locate original commit hash in message" echo "Note this should not necessarily be treated as a hard fail, but a reviewer's attention should" \ "be drawn to it and github actions have no way of doing that but to raise a 'failure'" problem=1 @@ -62,8 +80,6 @@ while read -r new_commit_sha ; do while read -r picked_branch ; do if git merge-base --is-ancestor "$original_commit_sha" "$picked_branch" ; then - echo " ✔ $original_commit_sha present in branch $picked_branch" - range_diff_common='git --no-pager range-diff --no-notes --creation-factor=100 @@ -72,13 +88,9 @@ while read -r new_commit_sha ; do ' if $range_diff_common --no-color 2> /dev/null | grep -E '^ {4}[+-]{2}' > /dev/null ; then - if [ -v GITHUB_ACTIONS ] ; then - echo ::endgroup:: - echo -n "::warning ::" - else - echo -n " ⚠ " - fi - echo "Difference between $new_commit_sha and original $original_commit_sha may warrant inspection:" + log success "$original_commit_sha present in branch $picked_branch" + endgroup + log warning "Difference between $new_commit_sha and original $original_commit_sha may warrant inspection:" $range_diff_common --color @@ -86,9 +98,10 @@ while read -r new_commit_sha ; do "be drawn to it and github actions have no way of doing that but to raise a 'failure'" problem=1 else - echo " ✔ $original_commit_sha highly similar to $new_commit_sha" + log success "$original_commit_sha present in branch $picked_branch" + log success "$original_commit_sha highly similar to $new_commit_sha" $range_diff_common --color - [ -v GITHUB_ACTIONS ] && echo ::endgroup:: + endgroup fi # move on to next commit @@ -97,13 +110,8 @@ while read -r new_commit_sha ; do done <<< "$branches" done - if [ -v GITHUB_ACTIONS ] ; then - echo ::endgroup:: - echo -n "::error ::" - else - echo -n " ✘ " - fi - echo "$original_commit_sha not found in any pickable branch" + endgroup + log error "$original_commit_sha not found in any pickable branch" problem=1 done <<< "$commits" From 6b2b5270dab6dd67a2f20e985dc621cbab1b4936 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Wed, 28 May 2025 18:21:33 +0200 Subject: [PATCH 4/6] ci/check-cherry-picks: improve formatting of failure reports (cherry picked from commit 3dff9c34c0e182dc6402ba94c0e4ff0c04dd0500) (cherry picked from commit 5b8b5708ce42b04bfd76f24c9cf2dce6ebbdd9fd) --- ci/check-cherry-picks.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ci/check-cherry-picks.sh b/ci/check-cherry-picks.sh index 4d91502e3551..f00011915dcd 100755 --- a/ci/check-cherry-picks.sh +++ b/ci/check-cherry-picks.sh @@ -92,7 +92,8 @@ while read -r new_commit_sha ; do endgroup log warning "Difference between $new_commit_sha and original $original_commit_sha may warrant inspection:" - $range_diff_common --color + # First line contains commit SHAs, which we already printed. + $range_diff_common --color | tail -n +2 echo "Note this should not necessarily be treated as a hard fail, but a reviewer's attention should" \ "be drawn to it and github actions have no way of doing that but to raise a 'failure'" From 40d0039f15ff0d7f81b924c7886cc73a3d8f4a13 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Wed, 28 May 2025 21:16:01 +0200 Subject: [PATCH 5/6] workflows/check-cherry-picks: post review comments Instead of failing the job, the workflow will now post review comments as "Request Changes". This makes the feedback more readily visible and avoids having to merge despite a failing CI job. It is also a pre-requisite to enable required status checks / required workflows in the future. Committers are asked to confirm the differences by explicitly dismissing the generated review. After dismissal, the related review comment will automatically be marked as "resolved". The comments only report warnings and errors. Reviews are automatically dismissed when they have been addressed by the author and no problems remain. If problems remain, existing, still pending, review comments will be updated. If the same problems had already been dismissed earlier, no new review comment will be created either. (cherry picked from commit 515b174c42ca9f5fa645688d249a00db90c578a0) (cherry picked from commit c971f0dbdc400cd3c91196c5d85e6572b44bb5e2) --- .github/workflows/check-cherry-picks.yml | 102 ++++++++++++++++++++++- .github/workflows/dismissed-review.yml | 30 +++++++ ci/check-cherry-picks.md | 7 ++ ci/check-cherry-picks.sh | 37 ++++++-- 4 files changed, 165 insertions(+), 11 deletions(-) create mode 100644 .github/workflows/dismissed-review.yml create mode 100644 ci/check-cherry-picks.md diff --git a/.github/workflows/check-cherry-picks.yml b/.github/workflows/check-cherry-picks.yml index 3c6ef1d18920..b44027e326d2 100644 --- a/.github/workflows/check-cherry-picks.yml +++ b/.github/workflows/check-cherry-picks.yml @@ -10,7 +10,8 @@ on: - 'staging-**' - '!staging-next' -permissions: {} +permissions: + pull-requests: write jobs: check: @@ -24,8 +25,105 @@ jobs: path: trusted - name: Check cherry-picks + id: check + continue-on-error: true env: BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | - ./trusted/ci/check-cherry-picks.sh "$BASE_SHA" "$HEAD_SHA" + ./trusted/ci/check-cherry-picks.sh "$BASE_SHA" "$HEAD_SHA" checked-cherry-picks.md + + - name: Prepare review + if: steps.check.outcome == 'failure' + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const { readFile, writeFile } = require('node:fs/promises') + + const job_url = (await github.rest.actions.listJobsForWorkflowRun({ + owner: context.repo.owner, + repo: context.repo.repo, + run_id: context.runId + })).data.jobs[0].html_url + '?pr=' + context.payload.pull_request.number + + const header = await readFile('trusted/ci/check-cherry-picks.md') + const body = await readFile('checked-cherry-picks.md') + const footer = + `\n_Hint: The diffs are also available in the [runner logs](${job_url}) with slightly better highlighting._` + + const review = header + body + footer + await writeFile('review.md', review) + core.summary.addRaw(review) + core.summary.write() + + - name: Request changes + if: ${{ github.event_name == 'pull_request_target' && steps.check.outcome == 'failure' }} + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const { readFile } = require('node:fs/promises') + const body = await readFile('review.md', 'utf-8') + + const pendingReview = (await github.paginate(github.rest.pulls.listReviews, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.payload.pull_request.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 (pendingReview) { + await github.rest.pulls.updateReview({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.payload.pull_request.number, + review_id: pendingReview.id, + body + }) + } else { + await github.rest.pulls.createReview({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.payload.pull_request.number, + event: 'REQUEST_CHANGES', + body + }) + } + + - name: Dismiss old reviews + if: ${{ github.event_name == 'pull_request_target' && steps.check.outcome == 'success' }} + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + await Promise.all( + (await github.paginate(github.rest.pulls.listReviews, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.payload.pull_request.number + })).filter(review => + review.user.login == 'github-actions[bot]' && + review.state == 'CHANGES_REQUESTED' + ).map(async (review) => { + await github.rest.pulls.dismissReview({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.payload.pull_request.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 }) + }) + ) diff --git a/.github/workflows/dismissed-review.yml b/.github/workflows/dismissed-review.yml new file mode 100644 index 000000000000..9886dad2b74e --- /dev/null +++ b/.github/workflows/dismissed-review.yml @@ -0,0 +1,30 @@ +name: Dismissed Review + +on: + pull_request_review: + types: [dismissed] + +permissions: + pull-requests: write + +jobs: + # The check-cherry-picks workflow creates review comments, + # that should sometimes be manually dismissed. + # When a CI-generated review is dismissed, this job automatically + # minimizes it, to prevent it from cluttering the PR. + minimize: + name: Minimize as resolved + if: github.event.review.user.login == 'github-actions[bot]' + runs-on: ubuntu-24.04-arm + steps: + - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + await github.graphql(`mutation($node_id:ID!) { + minimizeComment(input: { + classifier: RESOLVED, + subjectId: $node_id + }) + { clientMutationId } + }`, { node_id: context.payload.review.node_id }) + diff --git a/ci/check-cherry-picks.md b/ci/check-cherry-picks.md new file mode 100644 index 000000000000..5af41aadac5e --- /dev/null +++ b/ci/check-cherry-picks.md @@ -0,0 +1,7 @@ +This report is automatically generated by the `check-cherry-picks` CI workflow. + +Some of the commits in this PR have not been cherry-picked exactly and require the author's and reviewer's attention. + +Please make sure to follow the [backporting guidelines](https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#how-to-backport-pull-requests) and cherry-pick with the `-x` flag. This requires changes to go to the unstable branches (`master` / `staging`) first, before backporting them. + +Occasionally, it is not possible to cherry-pick exactly the same patch. This most frequently happens when resolving merge conflicts while cherry-picking or when updating minor versions of packages which have already advanced to the next major on unstable. If you need to merge this PR despite the warnings, please [dismiss](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/dismissing-a-pull-request-review) this review. diff --git a/ci/check-cherry-picks.sh b/ci/check-cherry-picks.sh index f00011915dcd..e0b278207875 100755 --- a/ci/check-cherry-picks.sh +++ b/ci/check-cherry-picks.sh @@ -3,11 +3,14 @@ set -euo pipefail -if [ $# != "2" ] ; then - echo "usage: check-cherry-picks.sh base_rev head_rev" +if [[ $# != "2" && $# != "3" ]] ; then + echo "usage: check-cherry-picks.sh base_rev head_rev [markdown_file]" exit 2 fi +markdown_file="$(realpath ${3:-/dev/null})" +[ -v 3 ] && rm -f "$markdown_file" + # Make sure we are inside the nixpkgs repo, even when called from outside cd "$(dirname "${BASH_SOURCE[0]}")" @@ -34,6 +37,18 @@ log() { fi echo "${prefix[$type]}$@" + + # Only logging errors and warnings, which allows comparing the markdown file + # between pushes to the PR. Even if a new, proper cherry-pick, commit is added + # it won't change the markdown file's content and thus not trigger another comment. + if [ "$type" != "success" ]; then + local -A alert + alert[warning]="WARNING" + alert[error]="CAUTION" + echo >> $markdown_file + echo "> [!${alert[$type]}]" >> $markdown_file + echo "> $@" >> $markdown_file + fi } endgroup() { @@ -58,9 +73,7 @@ while read -r new_commit_sha ; do ) if [ -z "$original_commit_sha" ] ; then endgroup - log error "Couldn't locate original commit hash in message" - echo "Note this should not necessarily be treated as a hard fail, but a reviewer's attention should" \ - "be drawn to it and github actions have no way of doing that but to raise a 'failure'" + log warning "Couldn't locate original commit hash in message of $new_commit_sha." problem=1 continue fi @@ -90,13 +103,19 @@ while read -r new_commit_sha ; do if $range_diff_common --no-color 2> /dev/null | grep -E '^ {4}[+-]{2}' > /dev/null ; then log success "$original_commit_sha present in branch $picked_branch" endgroup - log warning "Difference between $new_commit_sha and original $original_commit_sha may warrant inspection:" + log warning "Difference between $new_commit_sha and original $original_commit_sha may warrant inspection." # First line contains commit SHAs, which we already printed. $range_diff_common --color | tail -n +2 - echo "Note this should not necessarily be treated as a hard fail, but a reviewer's attention should" \ - "be drawn to it and github actions have no way of doing that but to raise a 'failure'" + echo -e ">
Show diff\n>" >> $markdown_file + echo '> ```diff' >> $markdown_file + # The output of `git range-diff` is indented with 4 spaces, which we need to match with the + # code blocks indent to get proper syntax highlighting on GitHub. + $range_diff_common | tail -n +2 | sed -Ee 's/^ {4}/> /g' >> $markdown_file + echo '> ```' >> $markdown_file + echo ">
" >> $markdown_file + problem=1 else log success "$original_commit_sha present in branch $picked_branch" @@ -112,7 +131,7 @@ while read -r new_commit_sha ; do done endgroup - log error "$original_commit_sha not found in any pickable branch" + log error "$original_commit_sha given in $new_commit_sha not found in any pickable branch." problem=1 done <<< "$commits" From 88bef63c74508f75b48ca155e121dc1f725c9d08 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Sat, 31 May 2025 10:56:29 +0200 Subject: [PATCH 6/6] workflows/check-cherry-picks: truncate long diffs after 10k characters GitHub comments have a length limit, so we can't just dump everything. The 10k limit is arbitrary, but the assumption is that reviewing the range-diff is not the sensible thing to do once it becomes a certain size - reviewing the regular diff and treating the commit as "new" is easier to do in that case. Thus, truncating should work out fine, especially when the full range-diff is still available in the runner log. This could still end up in with an error, if a PR has multiple commits, which all hit the limit. Let's get there first, before we try to fix that hypothetical case, too. (cherry picked from commit 856792f93ef48454c154a728000fe01e2b27f5fd) (cherry picked from commit 343e17a0f469c6e48e2f662239b66acc9fa1e061) --- .github/workflows/check-cherry-picks.yml | 5 ++++- ci/check-cherry-picks.sh | 13 ++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/.github/workflows/check-cherry-picks.yml b/.github/workflows/check-cherry-picks.yml index b44027e326d2..84ed5604443d 100644 --- a/.github/workflows/check-cherry-picks.yml +++ b/.github/workflows/check-cherry-picks.yml @@ -49,7 +49,7 @@ jobs: const header = await readFile('trusted/ci/check-cherry-picks.md') const body = await readFile('checked-cherry-picks.md') const footer = - `\n_Hint: The diffs are also available in the [runner logs](${job_url}) with slightly better highlighting._` + `\n_Hint: The full diffs are also available in the [runner logs](${job_url}) with slightly better highlighting._` const review = header + body + footer await writeFile('review.md', review) @@ -79,6 +79,9 @@ jobs: ) ) + // 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 + // this case, consider just splitting up those commits into multiple PRs. if (pendingReview) { await github.rest.pulls.updateReview({ owner: context.repo.owner, diff --git a/ci/check-cherry-picks.sh b/ci/check-cherry-picks.sh index e0b278207875..6036d5a1529a 100755 --- a/ci/check-cherry-picks.sh +++ b/ci/check-cherry-picks.sh @@ -112,7 +112,18 @@ while read -r new_commit_sha ; do echo '> ```diff' >> $markdown_file # The output of `git range-diff` is indented with 4 spaces, which we need to match with the # code blocks indent to get proper syntax highlighting on GitHub. - $range_diff_common | tail -n +2 | sed -Ee 's/^ {4}/> /g' >> $markdown_file + diff="$($range_diff_common | tail -n +2 | sed -Ee 's/^ {4}/> /g')" + # Also limit the output to 10k bytes (and remove the last, potentially incomplete line), because + # GitHub comments are limited in length. The value of 10k is arbitrary with the assumption, that + # after the range-diff becomes a certain size, a reviewer is better off reviewing the regular diff + # in GitHub's UI anyway, thus treating the commit as "new" and not cherry-picked. + # Note: This could still lead to a too lengthy comment with multiple commits touching the limit. We + # consider this too unlikely to happen, to deal with explicitly. + max_length=10000 + if [ "${#diff}" -gt $max_length ]; then + printf -v diff "%s\n\n[...truncated...]" "$(echo "$diff" | head -c $max_length | head -n-1)" + fi + echo "$diff" >> $markdown_file echo '> ```' >> $markdown_file echo "> " >> $markdown_file