Error on creation of wheel with a name that already exists, and add Py_LIMITED_API to the docs' FAQ #569
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
YannickJadoul
force-pushed
the
limited-abi
branch
from
73f2b36
to
79d6844
Compare
henryiii
reviewed
Jan 30, 2021
YannickJadoul
force-pushed
the
limited-abi
branch
from
1d8c1ec
to
19db6d4
Compare
YannickJadoul
commented
Jan 30, 2021
| 'please limit your Python selection to a single version when building limited API\n' | ||
| 'wheels, with CIBW_BUILD.') | ||
| log.error(message) | ||
| sys.exit(1) |
There was a problem hiding this comment.
Do we want sys.exit, or an exception? Or downgrade this to just a warning (though Windows will error, anyway?).
And if sys.exit: just 1, or a new/specific error code?
There was a problem hiding this comment.
How hard would it be to just skip the build here and go directly to tests when a abi3 wheel is already built? It should likely also be an error if tests are empty, as you should not "build" with multiple versions but not test? That would provide the "full support" solution.
There was a problem hiding this comment.
We've already done the build at this point. To do the 'full support' thing, we'd need to predict the produced wheel name without actually doing the build.
There was a problem hiding this comment.
The first time, you just build like normal. Then you could register that you produced a abi3 wheel, and future builds would skip building. For python-requires, you have to read the config, since you don't know what you are allowed to produce. For ABI3, you could just use the technique above and react to the built, rather than trying to compute what will happen first.
There was a problem hiding this comment.
True, that should be possible! The main question is how close this is to cibuildwheel's other behavior. Most of the time, you have to explicitly opt-in to things and cibuildwheel won't say "ah, you probably meant ...".
On a more practical note: how would you decide which Python version to build on? I don't fully understand Py_LIMITED_API, but it seems to also include a "minimum version"?
There was a problem hiding this comment.
how would you decide which Python version to build on?
Via Requires-Python and/or build selectors. cibuildwheel builds the oldest Python selected, and works it's way forward. Say it builds 2.7, normal wheel. 3.5, normal wheel. Then 3.6 is a stable ABI build. From then on, it just tests the existing wheel: 3.7, 3.8, and 3.9 are just tests of the ABI wheel.
There was a problem hiding this comment.
This would even work if someone had special logic in setup.py for the limited_abi only working after some version of minimum required Python, while any other non-reactive method would not. You don't need to worry about the other direction (yet?), since the limited ABI works on all future Pythons (at least until Python 4).
There was a problem hiding this comment.
Right, OK, that could work, indeed :-)
There was a problem hiding this comment.
I like this idea, yeah! Avoiding the metadata read would be great.
YannickJadoul
commented
Jan 30, 2021
cibuildwheel/linux.py
Outdated
| @@ -217,6 +217,18 @@ def build(options: BuildOptions) -> None: | |||
| # clean up test environment | |||
| docker.call(['rm', '-rf', venv_dir]) | |||
|
|
|||
| existing_output_files = docker.call(['ls'], cwd=container_output_dir).split('\n') | |||
There was a problem hiding this comment.
It still feels like there ought to be a better way.
YannickJadoul
force-pushed
the
limited-abi
branch
from
d6e85b3
to
cd5d185
Compare
|
(@henryiii, if you want to play around, feel free to push things here, btw! I'll be careful with force-pushes, then :-) ) |
|
Force pushes don't bother me, I can always reset. :) I think you got it, though. |
|
Cool. This is a good, pragmatic improvement on the situation in #78. I wouldn't consider this the ultimate support (maybe it doesn't close #78 exactly), ideally, I think that a project that sets But, those are theoretical 'some-day' desires, I'd still be +1 on merging this - more docs, better error messages, and the py-limited-api test coverage are great improvements! |
|
@joerick, yes, I had overlooked the issue with testing on different platforms, as mentioned by @mayeut (#78 (comment)). That might be a reason to come up with something, as I don't see a way to do this in At the same time, to me, setting just a single Python version in |
YannickJadoul
commented
Jan 31, 2021
YannickJadoul
force-pushed
the
limited-abi
branch
from
77eb721
to
f48240c
Compare
|
Does anyone have any idea what CircleCI is up to now? |
|
For some reason it seems to think there's already a wheel there? Maybe if you had a printout of the contents of the directory on failure, you could see what it was complaining about. ;) |
|
Actually, it's the reverse problem (not seeing a wheel that was supposed to be there), but the statement still stands. :) |
No, it's the one where I try to get a failure, that's not working. If only pytest would decently print out the mismatch. |
|
I often add a print statement since pytest's mismatch / contains printing is terrible. |
|
Let's try with |
|
In general, how do we want to continue with this? Do I write some docs and we get this in, and do the other thing (checking |
YannickJadoul
force-pushed
the
limited-abi
branch
from
3d46777
to
b52d27d
Compare
…a helpful warning about building for Python's limited API
…based on that output
YannickJadoul
force-pushed
the
limited-abi
branch
from
7b13a0f
to
cc0991a
Compare
Good call! So, output was written to stdout instead of standard error!? Why were the other platforms OK?? Also, all had had needed to do would have been rebasing, apparently :-( Because dbb6d80 |
|
Nvm AppVeyor failing: Cfr. pypy/manylinux#15 |
Entirely up to you - if you're interested in implementing the full 'checking But if that seems like a lot of work right now, I'm also happy to merge the interim solution and punt full support down the road. An aside, that occurred to me - it feels like the natural way to implement the 'full solution' above is to have a function like that would return true for something like Interesting to think that this might change our policy on non-platform wheels - after all, the result of is |
|
I kind of like that idea, yes! But at the same time, I'm very much not going to implement this |
Oh, probably this: https://packaging.pypa.io/en/latest/tags.html |
|
Yeah, hopefully that would work? Ideally we could get the list of supported tags in the host process, without downloading the Python distribution first |
|
|
Looking at the source, we probably would need to run it in the target Python. So perhaps it would be more like |
Hmmm, good point. And it needs to be run inside Docker. What about adding a short script that takes a directory, gets copied into Docker on Linux, and gets run on the guest Python? |
|
Together with @mattip's fragment (thanks, @mattip!), |
|
Make sure you bump the deps to 20.9 if you use parse_wheel_filename. :) |
Or even passing a little script with produces: This is similar to how the Docker container gets the environment inside the Docker container. |
|
The changes this would require (run only on one Python) are exactly opposite the long-term solution (run on all Pythons supported for testing), so I think it would be better to solve correctly once. I'm guessing this will miss the next release, which is a shame (I have at least one abi3 project identified that I might try to suggest cibuildwheel), but not worth delaying a release. |
|
Sorry, this has been dragging on as I couldn't find time and motivation at the same time, recently. One thing to note/discuss, though: after #607 failing because the wrong version of Python got installed and created the same wheel, are we still convinced that detecting use of the limited API though duplicate filenames is a good plan? |
|
No worries, I have also been struggling for one or the other of those lately!
"though duplicate filenames" - I'm not sure I fully understand the problem you foresee... if we had limited-api support, the issue in #607 would still have been caught because the test would have expected a different wheel, right? |
Right, that's true! We just never got to the tests, because something else failed earlier. Thanks for setting that right! :-) |
|
Closing in favour of full support in #1091 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #78, by providing clear error messages and documentation on how to use
cibuildwheelto create wheels with Python's limited API/stable ABI.TODO: