-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
Integrate build_msi
into main CI workflow
#121778
Integrate build_msi
into main CI workflow
#121778
Conversation
85bb8b4
to
a607b3d
Compare
Since the workflow is failing on |
@hugovk could you set the labels? |
a607b3d
to
662c1a5
Compare
I made a rebase. But FTR, the CI failure is still caused by #121879, not changes here. |
I'm not really a fan of just adding more and more checks to every PR. There's really no need to run this one without changes unless something external has changed, and the only thing that's external is |
@zooba this keeps the conditionals you have already, that doesn't change. It'll get skipped the same way. |
One of the reasons, I'm making this series of updates is that the workflows are a bit disorganized and I want to improve that. |
I identified two things that are external, though — the second one is the VM+CPython that GH ships in it. It's |
662c1a5
to
df78d34
Compare
Okay, good.
We shouldn't have any direct reliance on the underlying OS though, and the build is designed to abstract away installed tools. Having that occasionally change is a better way to flush out real issues (and we don't try to run the installer, which would be annoyingly impacted by the way GitHub installs Python... currently the release tests only succeed because GitHub can't have installed a copy of Python that we haven't released yet, but if you run the tests on an earlier version it often fails! Haven't found a good way around this yet, short of running our own (clean) VM.) Hopefully Blurb changes so little in future that we don't regress its CLI/API again, and then there's no issue with that either. It also seems we may not actually need to run it at all (based on some discussion with @ambv), but I'll need more time (in a few weeks) to confirm and fix that. |
Aha, I see. Well, I was thinking of a way to get this run on the VM changes earlier — with the current patch, it's becoming possible to add more clever change detection with cache-based checks that would trigger these job runs. I can explore that in a separate PR if you think it's something that makes sense. Let me know. I'll keep this PR scope tight, though. |
df78d34
to
6ead508
Compare
build_msi
into main CI workflowbuild_msi
into main CI workflow
Previously, this workflow would run on related file changes and not contribute the the overall outcome of the CI run. This patch turns it into a reusable workflow, integrating it closer with the rest of the setup. It remains non-voting and skips or failures will not block the CI, just as before.
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
ad0ba6b
to
a6bbc82
Compare
Please remember not to force push in this repo, it makes it harder to review what changed since last review. Everything is squash-merged at the end anyway. Thanks! From the devguide:
https://devguide.python.org/getting-started/pull-request-lifecycle/#quick-guide |
|
||
jobs: | ||
build: | ||
name: installer for ${{ inputs.arch }} |
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.
I don't know if the name will be qualified in some other way?
name: installer for ${{ inputs.arch }} | |
name: Windows installer for ${{ inputs.arch }} |
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.
It's already prefixed with "windows" on the calling side. I intentionally didn't add it here. The UI side is fine.
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 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.
On the screenshots above, it's visible that it's also consistent with other reusable workflows.
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Thanks! |
(cherry picked from commit af4329e) Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua> Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Sorry, @webknjaz and @hugovk, I could not cleanly backport this to
|
GH-122226 is a backport of this pull request to the 3.13 branch. |
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua> Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@webknjaz Please could you do the 3.12 backport? Many thanks! |
Sure, it's on my list, I was AFK for a bit and it got postponed ;) |
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>. (cherry picked from commit af4329e) Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
GH-122231 is a backport of this pull request to the 3.12 branch. |
(cherry picked from commit af4329e)
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@@ -218,6 +233,21 @@ jobs: | |||
arch: ${{ matrix.arch }} | |||
free-threading: ${{ matrix.free-threading }} | |||
|
|||
build_windows_msi: | |||
name: >- # ${{ '' } is a hack to nest jobs under the same sidebar category | |||
Windows MSI${{ '' }} |
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.
@webknjaz Someone else has said "Tests / Windows MSI${{ '' }} (pull_request)" looks like a bug.
I think in some other thread you said we can adjust it somehow, is that possible?
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 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.
Though, I think both might break grouping 🤔
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.
Hmm, any other ideas? Someone else has asked about it because it looks weird.
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.
I think it would be most impactful if GitHub fixed this in its own UI rather than having people resort to hacks. I'd be happy to drop the ${{ '' }}
hack from other places.
Would it be less confusing is we switched Windows MSI${{ '' }}
to ${{ 'Windows MSI' }}
?
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.
How would that look for the different builds?
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.
The same, just bits of syntax would be around the string, not after.
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 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.
Duplicating the matrix or moving the conditional inside the reusable workflow would improve this, I suppose.
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.
I've done some thinking and realized that duplicating the matrices is the only way to make it look different while preserving the matrix grouping in the UI.
Previously, this workflow would run on related file changes and not contribute the the overall outcome of the CI run. This patch turns it into a reusable workflow, integrating it closer with the rest of the setup. It remains non-voting and skips or failures will not block the CI, just as before.