CI: Merge open blocker PRs in all CI workflows + other improvements #36338
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| #!/bin/sh | ||
| # Merge open PRs from sagemath/sage labeled "blocker". | ||
| REPO="sagemath/sage" | ||
| GH="gh -R $REPO" | ||
| PRs="$($GH pr list --label "p: blocker / 1" --json number --jq '.[].number')" | ||
|
There was a problem hiding this comment. You probably want to make sure that they also have the "postive review" label. There was a problem hiding this comment. Definitely not. Sometimes it takes a while to find a competent reviewer. There was a problem hiding this comment. See #36338 (comment) for @mkoeppe's opinion on this. Without requiring "positive review" label, we can check the PR before we set it positive review. There was a problem hiding this comment. It will influence every other PR, so I think there should be some safety measure.
What do you mean? Doesn't the CI of that PR already checks the PR? Why do you need other PRs to check it? There was a problem hiding this comment.
We already have that, namely the Triage group + common sense. There was a problem hiding this comment.
Well, it's not fire, but it's a matter of "availability of the CI services", which, if poor, leads to developer frustration (@fchapoton @tornaria) and can dramatically reduce development velocity because developers may disengage or wait until the CI is fixed again. Let's consider the period from the release of a beta version to the release of the next beta version, currently about a week. For, say, 95% of availability of the CI, we want a CI fix to take effect within a few hours of a broken release.
This may be true for trivial fixes such as the linter failures, which can be fixed and reviewed by many people. But for fixes to CI and build system, it hinges on the availability of a very small number of people, and time-to-positive-review can be several days -- which can easily make the availability of the CI services less than 50%. There was a problem hiding this comment. @tobiasdiez wrote:
I know that @tobiasdiez knows this already, but I'll point out again: Our project does not hold majority votes In PR discussions.
This is false. This PR does resolve the issue of the failing CIs. Merging this PR is enough to fix all CI problems in the current cycle: it auto-applies the blocker PRs #36327 and #36276.
This is false, and moreover an inappropriate attack on author and reviewers. There was a problem hiding this comment. OK. Here is my suggestion. As we can observe by looking at the labelling war below, there are conflicting opinions on which way this PR should proceed. If the issue is large and of broad interest in our community, then this situation would be resolved by a voting in sage-devel. But the current issue is a small one, and it would be ridiculous and shameful to spend a long time for endless discussions or a large-scale voting. Hence let me suggest a small voting here, as defined below: The voting is about (a) "only blocker label" vs (b) "blocker label + positive review". The voting occurs only in this thread. The voting starts by the votes of @tobiasdiez and @mkoeppe, by which they are supposed to have vowed on accepting the voting result. The voting ends automatically when there was no more vote for the last 24 hours. When the voting ends, the author of the PR proceeds in the way of winning candidate. Please only vote with an optional simple comment in the rest of this thread. There was a problem hiding this comment. -1 on holding majority votes in PRs. There was a problem hiding this comment.
Sadly not false... |
||
| if [ -z "$PRs" ]; then | ||
| echo 'Nothing to do: Found no open PRs with "blocker" status.' | ||
| else | ||
| echo "Found PRs: " $PRs | ||
| export GIT_AUTHOR_NAME="ci-sage workflow" | ||
| export GIT_AUTHOR_EMAIL="ci-sage@example.com" | ||
| export GIT_COMMITTER_NAME="$GIT_AUTHOR_NAME" | ||
| export GIT_COMMITTER_EMAIL="$GIT_AUTHOR_EMAIL" | ||
| git tag -f test_base | ||
| git commit -q -m "Uncommitted changes" --no-allow-empty -a | ||
| for a in $PRs; do | ||
| echo "::group::Merging PR https://github.com/$REPO/pull/$a" | ||
| git tag -f test_head | ||
| $GH pr checkout -b pr-$a $a | ||
| git fetch --unshallow --all | ||
| git checkout -q test_head | ||
| if git merge --no-edit --squash -q pr-$a; then | ||
| echo "::endgroup::" | ||
| git commit -q -m "Merge https://github.com/$REPO/pull/$a" -a --allow-empty | ||
| echo "Merged #$a" | ||
| else | ||
| echo "::endgroup::" | ||
| echo "Failure merging #$a, resetting" | ||
| git reset --hard | ||
| fi | ||
| done | ||
| git log test_base..HEAD | ||
| fi | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,9 +24,30 @@ concurrency: | |
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| get_ci_fixes: | ||
|
There was a problem hiding this comment. This adds now another row in the checks under each PR, which is already a pretty crowded. Can this not be merged directly in the build workflow as you do say with conda? There was a problem hiding this comment. No, because the container does not have a suitable version of the gh script available, and installing it is too annoying. There was a problem hiding this comment. @mkoeppe, No response to this? There was a problem hiding this comment. You may need to switch a different view; I responded above There was a problem hiding this comment. https://github.com/marketplace/actions/install-gh-cli or something similar? There was a problem hiding this comment. that one's not portable to i686, and overall I don't think this is a worthwhile change There was a problem hiding this comment. I don't like the duplicate lines too. But that's a trivial issue, and there's no simple fix. Let's ignore it. There was a problem hiding this comment. We are not using any i686 architecture...that's a very potential if and when argument... There was a problem hiding this comment.
Yes, we are. The build.yml workflow accepts the container image on workflow_dispatch, and we do test 32-bit platforms. |
||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| id: checkout | ||
| uses: actions/checkout@v4 | ||
| - name: Merge CI fixes from sagemath/sage | ||
| run: | | ||
| .ci/merge-fixes.sh | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| - name: Store CI fixes in upstream artifact | ||
| run: | | ||
| mkdir -p upstream | ||
| git format-patch --stdout test_base > upstream/ci_fixes.patch | ||
| - uses: actions/upload-artifact@v2 | ||
| with: | ||
| path: upstream | ||
| name: upstream | ||
|
|
||
| build: | ||
| runs-on: ubuntu-latest | ||
| container: ghcr.io/sagemath/sage/sage-${{ github.event.inputs.platform || 'ubuntu-focal-standard' }}-with-targets:${{ github.event.inputs.docker_tag || 'dev'}} | ||
| needs: [get_ci_fixes] | ||
| steps: | ||
| - name: Checkout | ||
| id: checkout | ||
|
|
@@ -68,6 +89,21 @@ jobs: | |
| if [ ! -f worktree-image/.gitignore ]; then cp .gitignore worktree-image/; fi | ||
| (cd worktree-image && git add -A && git commit --quiet --allow-empty -m "old" -a && git tag -f old && git reset --hard new && git reset --quiet old && git add -N . && git status) | ||
|
|
||
| - name: Download upstream artifact | ||
| uses: actions/download-artifact@v3 | ||
| with: | ||
| path: upstream | ||
| name: upstream | ||
|
|
||
| - name: Apply CI fixes from sagemath/sage | ||
| # After applying the fixes, make sure all changes are marked as uncommitted changes. | ||
| run: | | ||
| if [ -r upstream/ci_fixes.patch ]; then | ||
| (cd worktree-image && git commit --quiet -m "current changes" --allow-empty -a && git am) < upstream/ci_fixes.patch | ||
| git reset --quiet old | ||
| git add --quiet -N . | ||
| fi | ||
|
|
||
| - name: Incremental build, test changed files (sage -t --new) | ||
| id: incremental | ||
| run: | | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting approach. There is a couple of thoughts I have about this:
Would there still be a sanity check that just tests this PR without merging in other things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't, and I don't think it's needed. Since the transition to GitHub, using priority labels has become pretty rare, and no one has used the blocker tag without a good reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but for these the merge would be a no-op, and I think I'm handling this correctly in the scripts.
EDIT: See the run for the current workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the release manager scripts ignore all dependencies and all priorities anyway...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds plausible.