-
-
Notifications
You must be signed in to change notification settings - Fork 454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CI: Merge open blocker PRs in all CI workflows + other improvements #36338
Changes from all commits
52508f6
0a27b65
4ba2b6a
9cff8ff
a9d84a8
e4025c1
3949cc7
8d1b661
21fc2b3
b92d9f4
b14ef26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
#!/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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You probably want to make sure that they also have the "postive review" label. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely not. Sometimes it takes a while to find a competent reviewer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We already have that, namely the Triage group + common sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. -1 on holding majority votes in PRs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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::" | ||
if git commit -q -m "Merge https://github.com/$REPO/pull/$a" -a --no-allow-empty; then | ||
echo "Merged #$a" | ||
else | ||
echo "Empty, skipped" | ||
fi | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mkoeppe, No response to this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may need to switch a different view; I responded above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/marketplace/actions/install-gh-cli or something similar? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are not using any i686 architecture...that's a very potential if and when argument... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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@v3 | ||
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,19 @@ 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 -q -m "current changes" --allow-empty -a && git am; git reset --quiet old; git add -N .) < upstream/ci_fixes.patch | ||
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.