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.

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

Fix windows 3.13t wheels #50

Merged
merged 19 commits into from
Jan 3, 2025
Merged

Fix windows 3.13t wheels #50

merged 19 commits into from
Jan 3, 2025

Conversation

ddelange
Copy link
Collaborator

@ddelange ddelange commented Dec 24, 2024

it seems we have CI rot coming from external/upstream changes somewhere in the past two weeks: just tried releasing 1.0.1 fc75159...4f172e2 and we're no longer green


- name: Add python lib to PATH
if: matrix.os == 'windows-2025'
run: Add-Content $env:GITHUB_PATH "${{ env.pythonLocation }}\lib"
Copy link
Collaborator Author

@ddelange ddelange Dec 24, 2024

Choose a reason for hiding this comment

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

something broke upstream in the past two weeks.

this is now necessary to get python313.lib discovered by maturin.

without this, we again hit PyO3/maturin-action#292:

fatal error LNK1181: cannot open input file 'python313.lib'

we encountered and fixed in #47 using generate-import-lib

now it manages to pass the 3.13 build again, but 3.13t is still red with LNK1181: ce73691 in this PR

both 3.13 and 3.13t were still working 2 weeks ago with all the same versions (maturin version, maturin-action version, pyo3 version, python 3.13.1 patch version): fc75159...4f172e2

tried both windows-2022 (no setup-python cache hit) and windows-2025 (python 3.13.1 cache hit), no difference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

alright, we're green with 3.13t 🎉

turns out we don't need this Add python lib to PATH step. we need an explicit 3.13t distribution installed instead.

.github/workflows/dists.yml Outdated Show resolved Hide resolved
@ddelange ddelange changed the title Pin runner images Fix windows 3.13 wheels again Dec 24, 2024
@oconnor663
Copy link
Owner

Haven't looked carefully yet but Merry Christmas all :)

@ddelange
Copy link
Collaborator Author

ddelange commented Dec 25, 2024

ok, windows 3.13t is green again 🎉 and now without generate-import-lib pyo3 extension enabled!

we're now simply setup-python'ing (near instant if py version is cached in the runner image) all python versions, including 3.13t, that we want to link against.

I wonder if 3.13t was really working 2 weeks ago...

now we'be become robust to external changes that github is making to the runner images.

@ddelange ddelange marked this pull request as ready for review December 25, 2024 11:18
@ddelange
Copy link
Collaborator Author

ddelange commented Dec 25, 2024

@oconnor663 lets make a squash&merge to avoid all these debug commits on master?

or even enable Require linear history in the branch protection of master?

I deleted the broken 1.0.1 Github Release and the git tag by running git push --delete origin 1.0.1, so we can squash and release a fresh 1.0.1

@ddelange ddelange changed the title Fix windows 3.13 wheels again Fix windows 3.13t wheels Dec 25, 2024
@ddelange ddelange requested a review from oconnor663 December 29, 2024 20:33
@@ -3,7 +3,7 @@ name: dists
on:
pull_request:
push:
branches: '**' # not tags
branches: [master]
Copy link
Owner

Choose a reason for hiding this comment

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

I do like being able to run CI on my branches without creating a PR. Does the '**' setting cause any problems?

Copy link
Collaborator Author

@ddelange ddelange Jan 2, 2025

Choose a reason for hiding this comment

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

the setting causes duplicate jobs for every commit pushed to a PR. i think what you're looking for is on: workflow_dispatch. we already added it in a previous PR: it adds a Run Workflow button on the right hand side here, where you can run CI manually on any branch without creating a PR

Copy link
Owner

Choose a reason for hiding this comment

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

Got it.

@oconnor663 oconnor663 merged commit 5b10c3c into master Jan 3, 2025
111 checks passed
@oconnor663
Copy link
Owner

Thanks! Merged.

with:
target: ${{ matrix.target }}
manylinux: ${{ matrix.manylinux || 'auto' }}
# Keep in sync with tests.yml
manylinux: ${{ matrix.manylinux }}
Copy link
Owner

Choose a reason for hiding this comment

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

Just so I follow, what's the reason for this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah I was experimenting with furthet breaking up the matrix, one Python version at a time. To keep that simple, I wrote out the full matrix, including the auto string. Ended up reverting, but decided to keep this one.

@ddelange ddelange deleted the runner-images branch January 3, 2025 07:57
@oconnor663
Copy link
Owner

Btw, releasing 1.0.1 twice cause this issue downstream: https://aur.archlinux.org/packages/python-blake3#comment-1005277. If we need to do something similar again, maybe better to just increment the patch version number?

@ddelange
Copy link
Collaborator Author

ddelange commented Jan 4, 2025

ah great that you mention, sorry about that. makes total sense!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants