From bf3607aa872cc42996ae4553c00088a64c438c91 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Wed, 13 Aug 2025 14:53:00 +0200 Subject: [PATCH 1/3] ci/github-script/commits: allow reason for not cherry-picking This change allows giving a reason via footer of the commit message for why this commit is not cherry-picked. This avoids having to "explain" the automated review comment afterwards - instead, this explanation can be given immediately when writing that commit. For example, for an update of `xen` on the stable branch, this could be: ``` xen: 4.19.3-unstable-2025-07-09 -> 4.19.3 [... commit message ...] Not-cherry-picked-because: unstable is on a different minor version ``` This would then be shown as part of the automated review. The severity of this will be downgraded from "warning" to "important". We still treat the review as "changes requested", because it would be very complicated and noisy to handle two different categories of reviews, some with requested changes and some with comments only. An alternative would be to not show this review at all. However, given that the reviewers expectation on backports should already be "if it's not a clean backport, the automated review will tell me what to look at", it seems better to show these and have the committer confirm by dismissing the review. Otherwise we risk merging actually unreviewed commits. --- ci/github-script/commits.js | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/ci/github-script/commits.js b/ci/github-script/commits.js index f22034ea788d..f155898a5c7e 100644 --- a/ci/github-script/commits.js +++ b/ci/github-script/commits.js @@ -23,13 +23,25 @@ module.exports = async function ({ github, context, core, dry }) { pull_number async function extract({ sha, commit }) { + const noCherryPick = Array.from( + commit.message.matchAll(/^Not-cherry-picked-because: (.*)$/g) + ).at(0) + + if (noCherryPick) + return { + sha, + commit, + severity: 'important', + message: `${sha} is not a cherry-pick, because: ${noCherryPick[1]}. Please review this commit manually.`, + } + // Using the last line with "cherry" + hash, because a chained backport // can result in multiple of those lines. Only the last one counts. - const match = Array.from( + const cherry = Array.from( commit.message.matchAll(/cherry.*([0-9a-f]{40})/g), ).at(-1) - if (!match) + if (!cherry) return { sha, commit, @@ -37,7 +49,7 @@ module.exports = async function ({ github, context, core, dry }) { message: `Couldn't locate original commit hash in message of ${sha}.`, } - const original_sha = match[1] + const original_sha = cherry[1] let branches try { @@ -222,7 +234,7 @@ module.exports = async function ({ github, context, core, dry }) { // Whether this is intended or just an implementation detail is unclear. core.summary.addRaw('
') core.summary.addRaw( - `\n\n[!${severity == 'warning' ? 'WARNING' : 'CAUTION'}]`, + `\n\n[!${({ important: 'IMPORTANT', warning: 'WARNING', error: 'CAUTION' })[severity]}]`, true, ) core.summary.addRaw(`${message}`, true) From 2f8ed18c97af8c4c829bb0cdbdec99ee057a758f Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Wed, 13 Aug 2025 20:46:43 +0200 Subject: [PATCH 2/3] ci/github-script/commits: clarify comments Splits the "occasionally" case into two, depending on whether the commit has a diff or was not cherry-picked at all. Prepares the next commit, where these are conditionally shown only. --- ci/github-script/check-cherry-picks.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/ci/github-script/check-cherry-picks.md b/ci/github-script/check-cherry-picks.md index 1b214b28eaa7..cda9936d6e30 100644 --- a/ci/github-script/check-cherry-picks.md +++ b/ci/github-script/check-cherry-picks.md @@ -5,6 +5,11 @@ Some of the commits in this PR require the author's and reviewer's attention. Please 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 the unstable `master` and `staging` branches first, before backporting them. -Occasionally, it is not possible to cherry-pick exactly the same patch. -This most frequently happens when resolving merge conflicts or when updating minor versions of packages which have already advanced to the next major on unstable. +Occasionally, commits are not cherry-picked at all, for example when updating minor versions of packages which have already advanced to the next major on unstable. +These commits can optionally be marked with a `Not-cherry-picked-because: ` footer. + +Sometimes it is not possible to cherry-pick exactly the same patch. +This most frequently happens when resolving merge conflicts. +The range-diff will help to review the resolution of conflicts. + 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 shortly before merging. From 91fd9b10aca8eabf0778da1daea9d7ee62fd615e Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Wed, 13 Aug 2025 21:00:42 +0200 Subject: [PATCH 3/3] ci/github-script/commits: conditionally show comments This only shows *some* of the additional hints, depending on what the checks resulted in. Should hopefully reduce confusion a bit. --- ci/github-script/check-cherry-picks.md | 15 ------------- ci/github-script/commits.js | 30 ++++++++++++++++++++++---- 2 files changed, 26 insertions(+), 19 deletions(-) delete mode 100644 ci/github-script/check-cherry-picks.md diff --git a/ci/github-script/check-cherry-picks.md b/ci/github-script/check-cherry-picks.md deleted file mode 100644 index cda9936d6e30..000000000000 --- a/ci/github-script/check-cherry-picks.md +++ /dev/null @@ -1,15 +0,0 @@ -This report is automatically generated by the `PR / Check / cherry-pick` CI workflow. - -Some of the commits in this PR require the author's and reviewer's attention. - -Please 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 the unstable `master` and `staging` branches first, before backporting them. - -Occasionally, commits are not cherry-picked at all, for example when updating minor versions of packages which have already advanced to the next major on unstable. -These commits can optionally be marked with a `Not-cherry-picked-because: ` footer. - -Sometimes it is not possible to cherry-pick exactly the same patch. -This most frequently happens when resolving merge conflicts. -The range-diff will help to review the resolution of conflicts. - -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 shortly before merging. diff --git a/ci/github-script/commits.js b/ci/github-script/commits.js index f155898a5c7e..82fedd608b06 100644 --- a/ci/github-script/commits.js +++ b/ci/github-script/commits.js @@ -33,6 +33,7 @@ module.exports = async function ({ github, context, core, dry }) { commit, severity: 'important', message: `${sha} is not a cherry-pick, because: ${noCherryPick[1]}. Please review this commit manually.`, + type: 'no-cherry-pick', } // Using the last line with "cherry" + hash, because a chained backport @@ -47,6 +48,7 @@ module.exports = async function ({ github, context, core, dry }) { commit, severity: 'warning', message: `Couldn't locate original commit hash in message of ${sha}.`, + type: 'no-commit-hash', } const original_sha = cherry[1] @@ -133,6 +135,7 @@ module.exports = async function ({ github, context, core, dry }) { colored_diff, severity: 'warning', message: `Difference between ${sha} and original ${original_sha} may warrant inspection.`, + type: 'diff', } } @@ -214,10 +217,29 @@ module.exports = async function ({ github, context, core, dry }) { if (results.some(({ severity }) => severity == 'error')) process.exitCode = 1 - core.summary.addRaw( - await readFile(join(__dirname, 'check-cherry-picks.md'), 'utf-8'), - true, - ) + core.summary.addRaw('This report is automatically generated by the `PR / Check / cherry-pick` CI workflow.', true) + core.summary.addEOL() + core.summary.addRaw("Some of the commits in this PR require the author's and reviewer's attention.", true) + core.summary.addEOL() + + if (results.some(({ type }) => type === 'no-commit-hash')) { + core.summary.addRaw('Please 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.', true) + core.summary.addRaw('This requires changes to the unstable `master` and `staging` branches first, before backporting them.', true) + core.summary.addEOL() + core.summary.addRaw('Occasionally, commits are not cherry-picked at all, for example when updating minor versions of packages which have already advanced to the next major on unstable.', true) + core.summary.addRaw('These commits can optionally be marked with a `Not-cherry-picked-because: ` footer.', true) + core.summary.addEOL() + } + + if (results.some(({ type }) => type === 'diff')) { + core.summary.addRaw('Sometimes it is not possible to cherry-pick exactly the same patch.', true) + core.summary.addRaw('This most frequently happens when resolving merge conflicts.', true) + core.summary.addRaw('The range-diff will help to review the resolution of conflicts.', true) + core.summary.addEOL() + } + + core.summary.addRaw('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 shortly before merging.', true) + results.forEach(({ severity, message, diff }) => { if (severity == 'info') return