-
Notifications
You must be signed in to change notification settings - Fork 239
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
Add support for Py_LIMITED_API ? #78
Comments
Hmmm, I hadn't come across this before. So how do you reckon would this then be supported by cibuildwheel? What's the difference to the build process? And, trying to assess the idea further: how relevant is |
I think the only change to the build process is passing something like As for for "does it make sense to use this with cibuildwheel" I can think of a couple of benefits:
As stated initially, I mainly wanted to get a discussion started about whether this makes sense or not, I don't really have the answer at this stage. |
Interesting. I didn't know about this! It would be a great improvement if we could support this. A few notes off the top of my head:
|
If we don't want to introduce an extra option, and want to continue using http://wheel.readthedocs.io/en/stable/#defining-the-python-version I'm not sure what happens if you're building with an older Python though. For the testing phase: I definitely agree, we want to test all Python versions. |
Here's a little experiment, I set Here are the results: https://travis-ci.org/jlaine/pylibsrtp/builds/398909104 The build actually succeeds on Linux! On OS X I get a "file already exists" error and Windows just bombs. |
It seems tooling for Py_LIMITED_API on Windows is not quite there yet: |
@jlaine Yeah, ok, those are good arguments indeed :-) But I think it's important to maybe explicitly list them and keep them in the back of our minds. @joerick |
@YannickJadoul looks like the @jlaine great catch on the
I'm leaning toward the latter, but open to other opinions too. |
The packaging issue was solve a while ago. pnumpy is using it via the setup.cfg directive. But there is still a problem when running the matrix since, I think, an identical wheel is built for each version of python. This is fine for linux and mac, but windows complains with
There should be some kind of a check that an |
We could have |
Also, shouldn't you just build for one Python version with CIBW_BUILD? |
I keep meaning to come back to this issue! cibuildwheel should definitely support this. Design-wise, we should read it from If there's a limited-api set, we'd build the limited-api wheel once (probably on the oldest version selected by BUILD/SKIP and the limited abi). Subsequent runs with newer Pythons should skip the build step, because we already built a compatible wheel, but should still use it to run tests. On Pythons not compatible with the limited-api, we'd just do a normal platform wheel build. Ideally, the user should be able to just add I haven't full thought it through, but I think this might be simple-ish to add into the codebase, by making the build parts of the main build loop conditional on whether a abi-compatible wheel is already in the output_dir. |
One currently available workaround is to just select one Python version, ofc. In general, how would any |
Yes, this even lets you select the version you want to build on. Maybe just detecting that a wheel already exists and has a limited ABI name should cause a warning or an error, on all platforms? It's wasteful to rebuild the same wheels, even if it allowed on Unix, and means that the final wheel depends on what order we build in (which could change).
I assume it would change the set to ones that started |
Judging from @mattip's comment (#78 (comment)) I thought it already was an error? Not the clearest, though |
Just a line or 2 in the docs' FAQ might also just do the trick, though? |
It's not an error on Linux or macOS; I rather think it should be, you are waisting resources by just rebuilding the same wheel and replacing it over and over. And on Windows, the error makes it seem like it's something cibuildwheel should support, while if the error said "Wheel would be overwritten; it looks like you are making limited ABI wheels; please limit your Python selection to one version when building limited ABI wheels with CIBW_BUILD", then users would do that instead of coming here. :) PS: Not very familiar with Limited ABI; pybind11 doesn't support it, and Cython/SWIG either don't or just partially do, so don't know of anything using it); basing the above mostly on the comments above. |
And agree about adding a line or two in the FAQ, too. |
Ah, that's the part I overlooked! Thanks! |
While building for |
Yes, longer term, it would be nice to have multiple Pythons selected cause later ones to be tested with the wheel built on the earlier one (though a user can set this up afterwards with nox or something, as well). It's distinctly different than the current situation, where on Unix, the wheel is rebuilt on each Python version and replaced, which doesn't test its ability to be loaded on multiple Pythons at all! (which is why #569 is a strict improvement). This would be a very good reason to keep the builds going from earliest to latest Python version, though. I do think #536 will be enough to really help out on #468 so that reordering the builds is not needed to improve on that. |
This is a more a "should we do this" question rather than an actual issue.
Background : Python 3.2 and up offer a subset of the C API which is guaranteed to be stable at the ABI level. For packages that only make use of this limited API, it is possible to build wheels which can be used across multiple Python versions.
https://docs.python.org/3/c-api/stable.html
For instance
cryptography
switched to Py_LIMITED_API a few months ago:pyca/cryptography#3307
Adding support for this would for instance have avoid the need to rebuild wheels for Python 3.7.
The text was updated successfully, but these errors were encountered: