-
Notifications
You must be signed in to change notification settings - Fork 338
[infra] clean up cibuildwheel build #1599
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
Conversation
.github/workflows/python-release.yml
Outdated
# Ignore tests for pypy since not all dependencies are compiled for it | ||
# and would require a local rust build chain | ||
CIBW_TEST_SKIP: "pp* *macosx*" |
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.
Nice! I also noticed this when reviewing #1391. Should we remove that one on main
apart from this PR?? I think that would be something that we want to include in 0.9.0 anyway
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.
As pointed out in the comment, I'm not sure if we only want to use uv for the building of the artifacts. Since we don't do releases every so often, I don't think the few minutes are that important. I'd much rather avoid breaking something by using different tooling.
.github/workflows/python-release.yml
Outdated
@@ -147,23 +147,23 @@ jobs: | |||
run: python3 -m poetry build --format=sdist | |||
if: startsWith(matrix.os, 'ubuntu') | |||
|
|||
- uses: astral-sh/setup-uv@v4 # required to use `uv` in cibuildwheel | |||
|
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'm a bit reluctant to only use uv
just for building the wheels. I'd be in favor of replacing Poetry with uv first and see if that's possible. I'm hearing a lot of good stuff about uv, bit some blogs also suggest that it is still lacking some functionality.
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.
backed out of the uv changes, i created #1602 so we can take a look in the future
d111cb6
to
b4b9ba7
Compare
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.
Looking good, thanks @kevinjqliu for cleaning this up and running the test-run 👍
@@ -75,13 +75,10 @@ jobs: | |||
CIBW_ARCHS: "auto64" | |||
CIBW_PROJECT_REQUIRES_PYTHON: ">=3.9,<3.13" | |||
CIBW_TEST_REQUIRES: "pytest==7.4.2 moto==5.0.1" | |||
CIBW_TEST_EXTRAS: "s3fs,glue" |
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.
Nice, wasn't aware that we could drop those. Should speed it up also quite a bit 👍
Removes `CIBW_TEST_EXTRAS: "s3fs,glue" since we dont need the extra dependencies for tests
Removes skipping MacOS build in
CIBW_TEST_SKIP
Test run on fork of release candidate on https://github.com/kevinjqliu/iceberg-python/actions/runs/13117565428