-
Notifications
You must be signed in to change notification settings - Fork 49
Build Wheels with ABI3 #627
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
base: main
Are you sure you want to change the base?
Conversation
|
I don't know how tox works to fix the failures here, but a |
|
I cannot figure out what is wrong with the CIs. In a different env, where the wheels are built and tested, everything passes. I'm guessing it is the lockfiles, maybe the cython version there is different? |
| env: | ||
| CIBW_SKIP: "cp39-* pp* *-musllinux*" | ||
| CIBW_SKIP: "*-musllinux*" | ||
| CIBW_BUILD: "cp311-* cp314-*" |
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.
This syntax does not specify a range, as far as I can tell (ref). Should 312 and 313 be explicitly referenced here?
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.
Nope. In fact we only need cp311, and that one will be installable in any Python >= 3.11. The cp314 is just for testing purposes so we know nothing will break with future changes in the stable ABI.
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.
So if I can help this PR to pass its CI, then we'll remove the 314 reference?
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.
BTW, you can download the wheels generated here in https://github.com/ocefpaf/cf-units/actions/runs/18846733110 and test them in any env with py>=3.11.
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.
So if I can help this PR to pass its CI, then we'll remove the
314reference?
It is nice to have for testing purposes IMO, but it can be removed if you don't care about future proofing.
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 you've probably noticed: there is not enough resource available for maintaining cf-units. So this is really appealing:
Note that, with ABI3, we don't need to rebuild the extension every new Python release.
Specifying 314 at the moment is good future proofing. But at some point 3.14 will be officially supported1 and this setting will be confusing to developers who were not part of this conversation. So yes please lets remove it.
Footnotes
-
apologies if I've misunderstood, I'm having to teach myself cibuildwheel so that I can review this. ↩
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.
No need to apologize! I'm learning ABI3 as well. Note that, having the 314 there is not a big deal. We do not ship any wheels with it, we only test if the current config works against the latest Python. In theory it should always work b/c the ABI3's promise is stability. However, they may introduce breaking changes every-now-and-then and, by testing against latest Python, we will catch it many years before we are forced to upgrade the min Python build.
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.
OK I think I'm catching up. You said earlier that we could support 3.10, and 3.11 is just for consistency with SPEC-0, which I'm happy with. So we're talking about this line being a shifting 'window'. Later on it will say:
CIBW_BUILD: "cp312-* cp315-*"
If I've got this right, then it would be great to make some appropriate changes to test_python_versions():
cf-units/cf_units/tests/test_coding_standards.py
Lines 157 to 159 in 9e34057
| ci_wheels_text = ci_wheels_file.read_text() | |
| (cibw_line,) = (line for line in ci_wheels_text.splitlines() if "CIBW_SKIP" in line) | |
| assert all(p not in cibw_line for p in supported_strip) |
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.
Not really a shifting Window. We will keep cp311 until it breaks, while updating the latest one as soon as a new Python is released.
I'm not familiar with what test_python_versions does, but I would still encourage testing things for all supported Python's in the CIs. Only the built wheels will be simplified.
|
Regarding Tox: we've been worried about it for a while, since tox-conda is unsupported and out-of-date. Unfortunately it has now stopped working. We've been meaning to adopt Pixi anyway so I've spent this afternoon writing a PR for that: Hopefully I can get that merged soon and then we can update your PR. |
This PR builds a single wheel for each platform using Python 3.11 ABI3. That means we drop support for 3.10 and 3.9, I can try to re-add 3.10 later but I don't think 3.9 is worth it. I would stick with SPEC 0 though, that way we can remove 3.10 now and make our lives a bit easier.
Note that, with ABI3, we don't need to rebuild the extension every new Python release. I just tested this wheel against 3.14 and it works as expected.
Closes #626