Skip to content
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

Use all compatible versions when calculating tags. #692

Merged
merged 5 commits into from
Mar 30, 2019

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Mar 28, 2019

Previously, we'd pass a single version to get_supported when we knew
one which would lead to a single tag being generated for the abi3 case
instead of one per previous minor version.

As part of this change, we need a more modern version of the
pep425tags.py we had copied from pip that allows passing multiple
versions to get_supported. Ideally, we'd use the version embedded in
our vendored setuptools code and this is tracked by #696. Extra code
had been added to pep425tags to implement get_supported_for_any_abi
and handle manylinux. This is now inlined in platforms.py, which was
the only user, the door to using undisturbed copies in #696.

Fixes #539

Previously, we'd pass a single version to `get_supported` when we knew
one which would lead to a single tag being generated for the abi3 case
instead of one per previous minor version.

Fixes pex-tool#539
@Eric-Arellano
Copy link
Contributor

Starting review now.

If you're able to, it would be helpful to break out a pre-cursor PR for the tweaks you make to the copied code, i.e. the indentation and the NB comments about what you changed from the copied code. Don't include the ABI3 changes. That way, this diff can be smaller and only include the changes you needed to make for ABI3 to work. Won't block on this though.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Especially good testing.

Regarding my earlier request to break out pep425 and glibc PR, ignore the part about breaking out the NB comments change. I think a PR that only changes the indention to be the original would be quite helpful to have a smaller PR.

pex/pep425tags.py Show resolved Hide resolved
pex/pep425tags.py Show resolved Hide resolved
pex/platforms.py Outdated Show resolved Hide resolved
pex/platforms.py Show resolved Hide resolved
pex/platforms.py Show resolved Hide resolved
Co-Authored-By: jsirois <john.sirois@gmail.com>
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for updating that comment and explaining everything.

@jsirois
Copy link
Member Author

jsirois commented Mar 30, 2019

I filed #696 to track getting rid of these very confusing doubly indirect, doubly modified copies of pip's glibc.py and pep425tags.py files.

@jsirois
Copy link
Member Author

jsirois commented Mar 30, 2019

Thanks for the reviews and please accept my apolgies for lack of reviewer notes. I'm adding them below for posterity.

1st, the actual changes to the pep425tags.py and glibc.py files copied from out vendored setuptools:

$ diff pex/vendor/_vendored/setuptools/setuptools/pep425tags.py pex/pep425tags.py
0a1,6
> # NB: Copied from our vendored setuptools' version at:
> #   pex/vendor/_vendored/setuptools/setuptools/pep425tags.py
> # Modifications are marked with `# NB: Modified from ...`
> # TODO(John Sirois): Remove this file as part of https://github.com/pantsbuild/pex/issues/696
> # -------------------------------------------------------------------------------------------
> 
3c9,16
< """Generate and work with PEP 425 Compatibility Tags."""
---
> """Generate and work with PEP 425 Compatibility Tags.
> 
> NB: Several functions here have their code grabbed by `pex.interpreter._generate_identity_source`
> to use in runtime identification of python interpreter platform details. These functions are marked
> here and should maintain reliance on only stdlib support. Even then, any new imports added should
> be checked against those setup in `pex.interpreter.ID_PY_TMPL`.
> """
> 
12d24
< import warnings
15,17c27
< from .extern import six
< 
< from . import glibc
---
> from pex import glibc  # NB: Modified from `from . import glibc`
21a32,33
> # NB: Used in `pex.interpreter.ID_PY_TMPL` and should only rely on stdlib. If imports change,
> # please consult `pex.interpreter.ID_PY_TMPL` and adjust stdlib imports there as needed.
26c38
<         warnings.warn("{}".format(e), RuntimeWarning)
---
>         log.warn(str(e))  # NB: Modified from `warnings.warn("{}".format(e), RuntimeWarning)`
29a42,43
> # NB: Used in `pex.interpreter.ID_PY_TMPL` and should only rely on stdlib. If imports change,
> # please consult `pex.interpreter.ID_PY_TMPL` and adjust stdlib imports there as needed.
42a57,58
> # NB: Used in `pex.interpreter.ID_PY_TMPL` and should only rely on stdlib. If imports change,
> # please consult `pex.interpreter.ID_PY_TMPL` and adjust stdlib imports there as needed.
50a67,68
> # NB: Used in `pex.interpreter.ID_PY_TMPL` and should only rely on stdlib. If imports change,
> # please consult `pex.interpreter.ID_PY_TMPL` and adjust stdlib imports there as needed.
62,68c80,81
< def get_impl_tag():
<     """
<     Returns the Tag for this specific implementation.
<     """
<     return "{}{}".format(get_abbr_impl(), get_impl_ver())
< 
< 
---
> # NB: Used in `pex.interpreter.ID_PY_TMPL` and should only rely on stdlib. If imports change,
> # please consult `pex.interpreter.ID_PY_TMPL` and adjust stdlib imports there as needed.
80a94,95
> # NB: Used in `pex.interpreter.ID_PY_TMPL` and should only rely on stdlib. If imports change,
> # please consult `pex.interpreter.ID_PY_TMPL` and adjust stdlib imports there as needed.
98,103c113,119
<         if get_flag('Py_UNICODE_SIZE',
<                     lambda: sys.maxunicode == 0x10ffff,
<                     expected=4,
<                     warn=(impl == 'cp' and
<                           six.PY2)) \
<                 and six.PY2:
---
> 
>         # NB: Modified from ~ `from .extern import six; six.PY2`
>         PY2 = sys.version_info[0] == 2
>         if (get_flag('Py_UNICODE_SIZE',
>                      lambda: sys.maxunicode == 0x10ffff,
>                      expected=4,
>                      warn=(impl == 'cp' and PY2)) and PY2):
104a121
> 
317,319d333
< 
< 
< implementation_tag = get_impl_tag()
$ diff pex/vendor/_vendored/setuptools/setuptools/glibc.py pex/glibc.py
0a1,6
> # NB: Copied from our vendored setuptools' version at:
> #   pex/vendor/_vendored/setuptools/setuptools/glibc.py
> # Modifications are marked with `# NB: Modified from ...`
> # TODO(John Sirois): Remove this file as part of https://github.com/pantsbuild/pex/issues/696
> # -------------------------------------------------------------------------------------------
> 
7c13
< import warnings
---
> from pex import pex_warnings  # NB: Modified from `import warnings`
45,46c51,53
<         warnings.warn("Expected glibc version with 2 components major.minor,"
<                       " got: %s" % version_str, RuntimeWarning)
---
>         # NB: Modified from `warnings.warn(..., RuntimeError)`
>         pex_warnings.warn("Expected glibc version with 2 components major.minor,"
>                           " got: %s" % version_str)

2nd, note that the code added to the top of platforms.py:

  1. Moves the get_supported_for_any_abi from pep425tags.py to _get_supported_for_any_abi unaltered.
  2. Moves the manylinux code previously embedded in the pep425tags.py get_supported code into _get_supported - changing to a post-filter technique for 0 disruption to the original pep425tags.py code.

@jsirois jsirois merged commit 45140bf into pex-tool:master Mar 30, 2019
@jsirois jsirois deleted the issues/539 branch March 30, 2019 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants