[Backport release-24.11] workflows/check-cherry-picks: post review comments (#412857)

2026-07-22 00:20:58 +00:00 · 2025-06-01 10:03:46 +00:00
parent deea005fd7 88bef63c74
commit e02e68aa3f
4 changed files with 213 additions and 39 deletions
--- 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,108 @@ 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 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)
+            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
+              )
+            )
+
+            // 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,
+                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 })
+              })
+            )
--- a/.github/workflows/dismissed-review.yml
+++ 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 })
+
--- a/ci/check-cherry-picks.md
+++ 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.
--- a/ci/check-cherry-picks.sh
+++ b/ci/check-cherry-picks.sh
@@ -1,13 +1,16 @@
 #!/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"
+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]}")"

@@ -19,11 +22,43 @@ 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
+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
-  if [ "$GITHUB_ACTIONS" = 'true' ] ; then
+
+  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() {
+  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"
  else
    echo "================================================="
@@ -37,15 +72,8 @@ 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
-      echo ::endgroup::
-      echo -n "::error ::"
-    else
-      echo -n "  ✘ "
-    fi
-    echo "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'"
+    endgroup
+    log warning "Couldn't locate original commit hash in message of $new_commit_sha."
    problem=1
    continue
  fi
@@ -65,8 +93,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
@@ -75,23 +101,38 @@ 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
-            echo ::endgroup::
-            echo -n "::warning ::"
-          else
-            echo -n "  ⚠ "
+          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."
+
+          # First line contains commit SHAs, which we already printed.
+          $range_diff_common --color | tail -n +2
+
+          echo -e "> <details><summary>Show diff</summary>\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.
+          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 "Difference between $new_commit_sha and original $original_commit_sha may warrant inspection:"
+          echo "$diff" >> $markdown_file
+          echo '> ```' >> $markdown_file
+          echo "> </details>" >> $markdown_file

-          $range_diff_common --color
-
-          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
        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
-          [ "$GITHUB_ACTIONS" = 'true' ] && echo ::endgroup::
+          endgroup
        fi

        # move on to next commit
@@ -100,13 +141,8 @@ while read -r new_commit_sha ; do
    done <<< "$branches"
  done

-  if [ "$GITHUB_ACTIONS" = 'true' ] ; 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 given in $new_commit_sha not found in any pickable branch."

  problem=1
 done <<< "$commits"