-
-
Notifications
You must be signed in to change notification settings - Fork 259
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 python 2.x abi tag support #214
Conversation
@@ -99,6 +99,14 @@ def _iter_supported_tags(cls, impl, version, platform): | |||
""" | |||
# Predict soabi for reasonable interpreters. This is technically wrong but essentially right. | |||
abis = [] | |||
if impl == 'cp' and version.startswith('2'): |
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.
how about for now, just checking for either '2' or '3' and reusing the abis.extend call + appending the 'abi3' bit to eliminate duplication? we can split this back out if/when necessary.
something like:
if impl == 'cp' and (version.startswith('2') or version.startswith('3')):
abis.extend(...)
if version.startswith('3'):
abis.append('abi3')
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 did that initially, but wasn't sure if all the abi tags from cp3 also applied to cp2; the docs on seems to be lacking on this topic. The 2 separate conditionals allows for keeping the 2 lists independent.
I can extract out the common ones if that's preferred. Let me know.
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 think it's safe to assume based on pip's implementation that this initial set will apply to both cp2 and cp3. if other python3 specific items beyond 'abi3' are needed, presumably they'd be additive and could be handled in the if version.startswith('3')
branch.
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.
err, I spoke too soon - looks like the current implementation is:
https://github.com/pypa/pip/blob/develop/pip/pep425tags.py#L85
which seems to have special case for 'u' requiring python < 3.3
one comment, otherwise lgtm. btw, here's the relevant bit from pip for py2.x and pypy if you wanted to go all the way and match that behavior (i.e. https://github.com/pypa/pip/blob/develop/pip/pep425tags.py#L85 |
Fixed the PR and squashed the commits. I think this one should be good to go for now. Any idea if we could get this included in the next weekly pants release? I will look into the pip implementation, but that implementation will require rewriting the test cases to simulate the various interpreter flags. That said, the pip implementation only returns a single tag for the current interpreter, whereas the pex implementation is to return the entire list of compatible tags, so they aren't doing quite the same thing. |
sounds good - feel free to also just log an issue if you don't feel like dealing with it and then when a consumer comes along that needs it they can implement it themselves. anyway, thanks for the PR! I'll try to get a pex release out tomorrow - but it's probably a good idea to let a version bump bake in pants master for at least a couple days, so tomorrows pants release may be a little early. would next friday's pants release work? |
Yea, next week's pants release should be fine. Also, just wondering if the pex version used by pants could be overridden the same way the bootstrapped jar tools could be overridden via the BUILD.tools file as a python_requirement_library? |
@ewang depending on how you build/run pants, you could probably just cherrypick the requirements.txt version bump before building to use this (or just build from master after the version bump lands). we actually build from master vs release tags and use cherry picks quite often in our pants deployments. |
Not quite sure how to test this short of running it across a whole bunch of libraries. I updated the test case accordingly.
Left the existing code pertaining to the cp3 abi tags as is in case we need to change the supported tags for cp2 and cp3 independently.
But it has been tested with the cryptography==1.2.3 issue mentioned in #213.