Skip to content
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.

Sign up for GitHub

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

Jump to bottom

.ci/merge-fixes.sh: Obtain patches via URL, make customizable by repository variable #36686

Merged
merged 7 commits into from
Dec 10, 2023
Merged

.ci/merge-fixes.sh: Obtain patches via URL, make customizable by repository variable #36686

merged 7 commits into from
Dec 10, 2023

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Nov 9, 2023 •
edited

Instead of using gh pr checkout, we obtain the CI fixes via their patch URLs.

When a repository variable SAGE_CI_FIXES_FROM_REPOSITORIES is set in a fork, it is used instead of the hardcoded sagemath/sage as the source(s) of the CI fixes; this gives better control in decentralized development. When set to "none", this also makes it possible to see the "ground truth", addressing a concern raised in #36349 (comment). See comments at the top of .ci/merge-fixes.sh for details.

Also improving the display of details of the merged PRs, as requested by @tornaria in #36696 (comment)

Example runs:

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@mkoeppe mkoeppe self-assigned this Nov 9, 2023
@mkoeppe mkoeppe force-pushed the ci_merge_fixes_via_patch_url branch 5 times, most recently from d259cac to b2925cb Compare November 9, 2023 18:12
@mkoeppe mkoeppe marked this pull request as draft November 9, 2023 18:12
@mkoeppe mkoeppe mentioned this pull request Nov 11, 2023
Merged
2 tasks
@mkoeppe mkoeppe marked this pull request as ready for review November 12, 2023 03:19
tornaria
tornaria reviewed Nov 13, 2023
View reviewed changes
Copy link
Contributor

@tornaria tornaria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general.

.ci/merge-fixes.sh Outdated
@@ -2,7 +2,9 @@
# 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')"
mkdir -p upstream
PRs="$($GH pr list --label "p: blocker / 1" --json number --jq '.[].number' | tee upstream/ci-fixes.txt)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not add --label "s: positive review"? Multiple --label arguments work just fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do that later when the label-warring phase has been outgrown

.ci/merge-fixes.sh Outdated Show resolved Hide resolved
@tornaria
Copy link
Contributor

LGTM, but how is it tested?

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 13, 2023

So far, only on its own CI run...

@mkoeppe mkoeppe force-pushed the ci_merge_fixes_via_patch_url branch 2 times, most recently from 320f0a9 to 23114e6 Compare November 13, 2023 19:43
@mkoeppe mkoeppe changed the title .ci/merge-fixes.sh: Obtain patches via URL .ci/merge-fixes.sh: Obtain patches via URL, make customizable by repository secret Nov 13, 2023
@mkoeppe mkoeppe changed the title .ci/merge-fixes.sh: Obtain patches via URL, make customizable by repository secret .ci/merge-fixes.sh: Obtain patches via URL, make customizable by repository variable Nov 13, 2023
@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 13, 2023

I've reworked and generalized this a bit. Ready for review.

@github-actions GitHub Actions
Copy link

Documentation preview for this PR (built with commit 49b7f74; changes) is ready! 🎉

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 16, 2023 •
edited

LGTM, but how is it tested?

See it in action here:

https://github.com/kwankyu/sage/actions/runs/6887591924/job/18735031877

It seems working well. You can even watch it recovering from applying-patch failure.

Comparing with the original merge-fixes.sh as seen in https://github.com/sagemath/sage/actions/runs/6885524199/job/18729793450?pr=3672

(1) It is just slightly faster.

(2) It fails in applying the blocker #36617 but the original merge-fixes.sh succeeds.

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 16, 2023

Thanks for testing!

Comparing with the original merge-fixes.sh [...]

(1) It is just slightly faster.

Note it is much faster in runs where patches apply directly

(2) It fails in applying the blocker #36617 but the original merge-fixes.sh succeeds.

I think this failure would go away if we rebased #36617.

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 16, 2023

Comparing with the original merge-fixes.sh [...]
(1) It is just slightly faster.

Note it is much faster in runs where patches apply directly

Right. It was slow this time because of #36617 that failed to be applied directly.

(2) It fails in applying the blocker #36617 but the original merge-fixes.sh succeeds.

I think this failure would go away if we rebased #36617.

Failing in applying non-rebased blockers may be a good thing, by the way.

kwankyu
kwankyu approved these changes Nov 17, 2023
View reviewed changes
Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too.

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 17, 2023

Failing in applying non-rebased blockers may be a good thing, by the way.

I think so, too. It sort of enforces some restraint on what to do in these CI fixes.

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 17, 2023

Thanks both for the review!

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 7, 2023
sagemathgh-36686: .ci/merge-fixes.sh: Obtain patches via URL, make …
a706971 
…customizable by repository variable

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
Instead of using `gh pr checkout`, we obtain the CI fixes via their
patch URLs.

- This is faster because typically we do not have to unshallow the repo
to apply the patches (seconds instead of ~2 minutes)
- Fewer surprises when applied to a PR based on an older release
- Conjecturally uses fewer API queries, helping avoid sagemath#36685

When a repository variable `SAGE_CI_FIXES_FROM_REPOSITORIES` is set in a
fork, it is used instead of the hardcoded sagemath/sage as the source(s)
of the CI fixes; this gives better control in decentralized development.
When set to "none", this also makes it possible to see the "ground
truth", addressing a concern raised in
sagemath#36349 (comment). See
comments at the top of `.ci/merge-fixes.sh` for details.

Also improving the display of details of the merged PRs, as requested by
@tornaria in
sagemath#36696 (comment)

Example runs:
- https://github.com/sagemath/sage/actions/runs/6854931832/job/186389018
59?pr=36686 (here it recovers gracefully from failed merges due to the
10.2.rc1/10.2.rc2 tagging confusions)
- https://github.com/mkoeppe/sage/actions/runs/6855410957/job/1864038039
8#step:3:8 (I have set `SAGE_CI_FIXES_FROM_REPOSITORIES=none` in my fork
-- for testing this PR)

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36686
Reported by: Matthias Köppe
Reviewer(s): Gonzalo Tornaría, Kwankyu Lee, Matthias Köppe
@vbraun vbraun merged commit 1e50484 into sagemath:develop Dec 10, 2023
20 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 10, 2023
@mkoeppe mkoeppe deleted the ci_merge_fixes_via_patch_url branch December 10, 2023 17:16
@tobiasdiez
Copy link
Contributor

tobiasdiez commented Dec 11, 2023 •
edited

It fails to apply #36372 now (and sometimes the jmol PR as well), although this PR is up-to-date with the newest dev branch. It's seems like an unnecessary constraint that blocker PR's need to be rebased (instead of merging the latest develop). Would be nice if this could be fixed as soon as possible.

@mkoeppe mkoeppe mentioned this pull request Dec 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tornaria tornaria tornaria left review comments

@kwankyu kwankyu kwankyu approved these changes

Assignees

@mkoeppe mkoeppe

Labels
c: scripts
Projects
None yet
Milestone
sage-10.3
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mkoeppe @tornaria @kwankyu @tobiasdiez @vbraun