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

require frozenlist to update version #107

Merged
merged 2 commits into from
Feb 27, 2024
Merged

require frozenlist to update version #107

merged 2 commits into from
Feb 27, 2024

Conversation

axfelix
Copy link
Contributor

@axfelix axfelix commented Feb 27, 2024

add frozenlist = "^1.4.0" to dev-dependencies since this was trying to install an older version that wasn't building right under Python 3.12 / MSVC 2022

@CLAassistant
Copy link

CLAassistant commented Feb 27, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@cretz
Copy link
Member

cretz commented Feb 27, 2024

Can you give more details? Our CI passes on Windows 3.12, here's the latest: https://github.com/temporalio/samples-python/actions/runs/7977279807/job/21779816371. Is there some test we could write that would make CI fail if this fix wasn't in place?

@axfelix
Copy link
Contributor Author

axfelix commented Feb 27, 2024

Yeah, it was triggering this PEP 517 build error in a relatively stock environment:

https://stackoverflow.com/questions/64038673/could-not-build-wheels-for-which-use-pep-517-and-cannot-be-installed-directly

My knowledge of Python bootstrapping is somewhat out of date -- it's possible our build system is doing something different than my environment to make the build pass -- but updating the dependency to a supported version that has wheels available seemed like a decent idea regardless.

@axfelix
Copy link
Contributor Author

axfelix commented Feb 27, 2024

Also -- this was only occurring when doing poetry install --with encryption as in https://github.com/temporalio/samples-python/tree/main/encryption, which we aren't testing currently.

@cretz
Copy link
Member

cretz commented Feb 27, 2024

it's possible our build system is doing something different than my environment to make the build pass -- but updating the dependency to a supported version that has wheels available seemed like a decent idea regardless.

Sure, but I want to make sure we catch future issues like this too, but if our build environment is not representative in some way I think we should fix.

Also -- this was only occurring when doing poetry install --with encryption as in https://github.com/temporalio/samples-python/tree/main/encryption, which we aren't testing currently.

👍 So I wonder if we should add --with encryption to our poetry install set in .github/workflows/ci.yml that would help catch this

@axfelix
Copy link
Contributor Author

axfelix commented Feb 27, 2024

Done! I strongly suspect that the ci.yml change would not work without the pyproject.toml change -- let me know if you'd like me to verify this further before merging.

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

I strongly suspect that the ci.yml change would not work without the pyproject.toml change -- let me know if you'd like me to verify this further before merging.

Good enough for me! Merge at will.

@axfelix axfelix merged commit 4303a9b into main Feb 27, 2024
9 checks passed
@axfelix axfelix deleted the update-frozenlist branch February 27, 2024 18:30
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.

3 participants