Integrate `build_msi` into main CI workflow#121778

webknjaz · 2024-07-14T23:06:55Z

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.

webknjaz · 2024-07-15T07:12:43Z

Since the workflow is failing on main, it's not a blocker for merging this PR, as shown in this run that you @hugovk triggered: https://github.com/python/cpython/actions/runs/9934791196. The most obvious difference between the last failing and this runs is that the successful one uses Python 3.11 and the recent failure is hitting 3.12.

webknjaz · 2024-07-15T19:08:30Z

@hugovk could you set the labels?

webknjaz · 2024-07-16T22:26:42Z

I made a rebase. But FTR, the CI failure is still caused by #121879, not changes here.

zooba · 2024-07-17T22:01:33Z

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 blurb (and I'm now tempted to pin the version, so then a change would trigger these runs).

webknjaz · 2024-07-17T22:10:27Z

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,

@zooba this keeps the conditionals you have already, that doesn't change. It'll get skipped the same way.

webknjaz · 2024-07-17T22:11:06Z

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.

webknjaz · 2024-07-17T22:13:17Z

and the only thing that's external is blurb

I identified two things that are external, though — the second one is the VM+CPython that GH ships in it. It's *-latest and unpinned, as a result. My first impression was that the problem was caused by the Python 3.11->3.12 update. It took some time to notice the blurb bit too.

zooba · 2024-07-18T11:24:57Z

this keeps the conditionals you have already, that doesn't change

Okay, good.

I identified two things that are external, though — the second one is the VM+CPython that GH ships in it.

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.

webknjaz · 2024-07-18T19:47:20Z

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.

.github/workflows/build.yml

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>

hugovk · 2024-07-23T20:53:57Z

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:

In order to keep the commit history intact, please avoid squashing or amending history and then force-pushing to the PR. Reviewers often want to look at individual commits.

https://devguide.python.org/getting-started/pull-request-lifecycle/#quick-guide

.github/workflows/build.yml

AA-Turner · 2024-07-23T23:54:48Z

.github/workflows/reusable-windows-msi.yml

+
+jobs:
+build:
+name: installer for ${{inputs.arch }}


I don't know if the name will be qualified in some other way?
Suggested change
name: installer for ${{inputs.arch }}
name: Windows installer for ${{inputs.arch }}

It's already prefixed with "windows" on the calling side. I intentionally didn't add it here. The UI side is fine.

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>

hugovk · 2024-07-24T09:46:42Z

Thanks!

miss-islington-app · 2024-07-24T09:46:42Z

Thanks @webknjaz for the PR, and @hugovk for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

(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>

miss-islington-app · 2024-07-24T09:46:50Z

Sorry, @webknjaz and @hugovk, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker af4329e7b1a25d58bb92f79480f5059c3683517b 3.12

bedevere-app · 2024-07-24T09:46:51Z

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>

hugovk · 2024-07-24T11:52:04Z

@webknjaz Please could you do the 3.12 backport? Many thanks!

webknjaz · 2024-07-24T12:32:42Z

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>

bedevere-app · 2024-07-24T12:36:21Z

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>

hugovk · 2024-08-02T11:51:11Z

.github/workflows/build.yml


+build_windows_msi:
+name: >- # ${{''} is a hack to nest jobs under the same sidebar category
+ Windows MSI${{''}}


@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?

@hugovk basically, there are two options:
split this into two matrices in the top-level workflow
move the interpolated part of the name into the reusable ones
I personally like the second option. Let me know if you or @ambv want something different.

Though, I think both might break grouping 🤔

Hmm, any other ideas? Someone else has asked about it because it looks weird.

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' }}?

Yeah, I think that might be a bit better?
The recent query was actually about the fromJSON stuff, do you think we can do anything about that?

Duplicating the matrix or moving the conditional inside the reusable workflow would improve this, I suppose.

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.

FWIW I also thought it was a bug (e.g. a missing space before the $ that prevented its expansion) and ended up here. I noticed it in #136959.

Yeah.. We should all go complain to GitHub so the hack would be redundant. Over the past few months, when I was bragging about a few other GHA ideas a couple of people saw this in those examples and immediately were like "I'm now going to be using this everywhere". I never really reported the bug to GitHub but probably should.

webknjaz requested review from ezio-melotti and hugovk as code owners July 14, 2024 23:06

bedevere-appbot added the awaiting review label Jul 14, 2024

webknjaz force-pushed the maintenance/gha-win-msi-reusable-collapse branch 2 times, most recently from 85bb8b4 to a607b3dCompare July 14, 2024 23:09

webknjaz force-pushed the maintenance/gha-win-msi-reusable-collapse branch from a607b3d to 662c1a5Compare July 16, 2024 22:25

webknjaz force-pushed the maintenance/gha-win-msi-reusable-collapse branch from 662c1a5 to df78d34Compare July 18, 2024 00:08

webknjaz force-pushed the maintenance/gha-win-msi-reusable-collapse branch from df78d34 to 6ead508Compare July 18, 2024 19:47

picnixz added skip news skip issue labels Jul 21, 2024

hugovk changed the title ~~🧪💅 Integrate build_msi into main CI workflow~~Integrate build_msi into main CI workflowJul 23, 2024

Eclips4 reviewed Jul 23, 2024
View reviewed changes

.github/workflows/build.yml Outdated Show resolvedHide resolved

hugovk reviewed Jul 23, 2024
View reviewed changes

.github/workflows/build.yml Outdated Show resolvedHide resolved

webknjazand others added 2 commits July 23, 2024 22:18

Make the CI job name less fun
a6bbc82
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>

webknjaz force-pushed the maintenance/gha-win-msi-reusable-collapse branch from ad0ba6b to a6bbc82Compare July 23, 2024 20:18

webknjaz requested a review from hugovk July 23, 2024 20:35

AA-Turner reviewed Jul 23, 2024
View reviewed changes

Fix the MSI change detection step title
a0a5372
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

hugovk added the needs backport to 3.12 only security fixes label Jul 24, 2024

hugovk added the needs backport to 3.13 bugs and security fixes label Jul 24, 2024

hugovk merged commit af4329e into python:mainJul 24, 2024

bedevere-appbot removed the awaiting review label Jul 24, 2024

miss-islington-appbot assigned hugovk Jul 24, 2024

bedevere-appbot removed the needs backport to 3.13 bugs and security fixes label Jul 24, 2024

bedevere-appbot removed the needs backport to 3.12 only security fixes label Jul 24, 2024

hugovk pushed a commit that referenced this pull request Jul 24, 2024
[3.12] Integratebuild_msiinto main CI workflow (GH-121778) (#122231)
f8ddebb
(cherry picked from commit af4329e)

hugovk reviewed Aug 2, 2024
View reviewed changes

nohlson mentioned this pull request Aug 13, 2024
"test" GitHub Action workflow cannot be run manually #122965
Closed

	name: installer for ${{inputs.arch }}
	name: Windows installer for ${{inputs.arch }}

Uh oh!

Integrate build_msi into main CI workflow#121778

Integrate build_msi into main CI workflow #121778

Uh oh!

Conversation

webknjaz commented Jul 14, 2024

Uh oh!

webknjaz commented Jul 15, 2024

Uh oh!

webknjaz commented Jul 15, 2024

Uh oh!

webknjaz commented Jul 16, 2024

Uh oh!

zooba commented Jul 17, 2024

Uh oh!

webknjaz commented Jul 17, 2024

Uh oh!

webknjaz commented Jul 17, 2024

Uh oh!

webknjaz commented Jul 17, 2024

Uh oh!

zooba commented Jul 18, 2024

Uh oh!

webknjaz commented Jul 18, 2024

Uh oh!

Uh oh!

Uh oh!

hugovk commented Jul 23, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjazJul 24, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugovk commented Jul 24, 2024

Uh oh!

miss-islington-appbot commented Jul 24, 2024

Uh oh!

miss-islington-appbot commented Jul 24, 2024

Uh oh!

bedevere-appbot commented Jul 24, 2024

Uh oh!

hugovk commented Jul 24, 2024

Uh oh!

webknjaz commented Jul 24, 2024

Uh oh!

bedevere-appbot commented Jul 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjazAug 2, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Integrate `build_msi` into main CI workflow#121778

Integrate `build_msi` into main CI workflow #121778

hugovk commented Jul 23, 2024•
edited
Loading

webknjazJul 24, 2024•
edited
Loading

webknjazAug 2, 2024•
edited
Loading