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

Respect Requires-Python in PEXEnvironment. #923

Merged
merged 1 commit into from
Mar 19, 2020

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Mar 18, 2020

Previously Pex did not properly handle activation of wheels with
Requires-Python metadata restricting the wheel use to certain Python
interpreter versions.

Fixes #898

@jsirois
Copy link
Member Author

jsirois commented Mar 18, 2020

A test was added that failed previously as outlined in #898.

Beyond that, concretely, for:

$ python -mpex --python=python3.6 --python=python2.7 'zipp>=1,<=3.1.0' -o zipp.pex

OLD

We used to see the following Pex metadata:

$ unzip -qc zipp.pex PEX-INFO | jq -r .requirements[]
contextlib2==0.6.0.post1; python_version < "3.4"
zipp==1.2.0
zipp==3.1.0

And the following suprious errors:

  1. Straight up failure to activate the PEX when it has all needed requirements:
$ python2.7 zipp.pex
Failed to execute PEX file. Needed manylinux2014_x86_64-cp-27-cp27mu compatible dependencies for:
 1: zipp==3.1.0
    But this pex only contains:
      zipp-1.2.0-py2.py3-none-any.whl
      zipp-3.1.0-py3-none-any.whl
  1. Successful activation, but picking the wrong version of the embedded zipp dist (should be 1.2.0 since 3.1.0 is marked as Python >=3.6 only):
$ PEX_VERBOSE=2 python3.5 zipp.pex
...
pex: Activating PEX virtual environment from zipp.pex: 10.7ms                                   
pex:   Searching dependency cache: /home/jsirois/dev/pantsbuild/pex/zipp.pex/.deps: 2.2ms
pex:     Adding zipp 1.2.0: 0.1ms
pex:     Adding contextlib2 0.6.0.post1: 0.1ms
pex:     Adding zipp 3.1.0: 0.1ms
pex:   Resolving zipp==3.1.0: 5.1ms
pex:   Activating zipp 3.1.0: 0.3ms
pex:     Adding sitedir: 0.1ms
...
pex: PYTHONPATH contains:
pex:     /home/jsirois/dev/pantsbuild/pex/zipp.pex
pex:   * /usr/lib/python35.zip
pex:     /usr/lib/python3.5
pex:     /usr/lib/python3.5/plat-linux
pex:     /usr/lib/python3.5/lib-dynload
pex:     /home/jsirois/.pex/installed_wheels/72b7f46adbc9d0780393bac5dd98d910a507f452/zipp-3.1.0-py3-none-any.whl
pex:   * /home/jsirois/dev/pantsbuild/pex/zipp.pex/.bootstrap
pex:   * - paths that do not exist or will be imported via zipimport

NEW

We now see the following Pex metadata:

$ unzip -qc zipp.pex PEX-INFO | jq -r .requirements[]
contextlib2==0.6.0.post1; python_version != "3.0.*" and python_version != "3.1.*" and python_version != "3.2.*" and python_version != "3.3.*" and python_version >= "2.7" and python_version < "3.4"
zipp==1.2.0; python_version >= "2.7"
zipp==3.1.0; python_version >= "3.6"

The Python 2.7 case above now works and the python 3.5 case above does as well as shown here:

$ PEX_VERBOSE=2 python3.5 zipp.pex
...
pex: Skipping activation of `contextlib2==0.6.0.post1; python_version != "3.0.*" and python_version != "3.1.*" and python_version != "3.2.*" and python_version != "3.3.*" and python_version >= "2.7" and python_version < "3.4"` due to environment marker de-selection
pex: Skipping activation of `zipp==3.1.0; python_version >= "3.6"` due to environment marker de-selection
pex: Activating PEX virtual environment from zipp.pex: 20.3ms                                                                           
pex:   Searching dependency cache: /home/jsirois/dev/pantsbuild/jsirois-pex/zipp.pex/.deps: 4.9ms
pex:     Adding contextlib2 0.6.0.post1: 0.6ms
pex:     Adding zipp 1.2.0: 0.3ms
pex:   Resolving zipp from [Requirement.parse('zipp==1.2.0; python_version >= "2.7"')]: 8.9ms
pex:   Activating zipp 1.2.0: 0.3ms
pex:     Adding sitedir: 0.2ms
...
pex: PYTHONPATH contains:
pex:     /home/jsirois/dev/pantsbuild/jsirois-pex/zipp.pex
pex:   * /usr/lib/python35.zip
pex:     /usr/lib/python3.5
pex:     /usr/lib/python3.5/plat-linux
pex:     /usr/lib/python3.5/lib-dynload
pex:     /home/jsirois/.pex/installed_wheels/ba927d0181dc282776bb19497dad82f2389764e7/zipp-1.2.0-py2.py3-none-any.whl
pex:   * /home/jsirois/dev/pantsbuild/jsirois-pex/zipp.pex/.bootstrap
pex:   * - paths that do not exist or will be imported via zipimport

Copy link
Collaborator

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

Looks great, I just had a question that occurred to me while reviewing.

# used to build this pex already did so. There may be multiple distributions satisfying any
# particular key (e.g.: a Python 2 specific version and a Python 3 specific version for a
# multi-python PEX) and we want the working set to pick the most appropriate one.
req = Requirement.parse(key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Requirement.parse() chokes on requirements that aren't of the form foo==x.y.z, e.g., URLs. So this cannot support URLs. And I know that is not a consequence of this change, it was true before, since the method that calls this one also calls Requirement.parse(), and WorkingSet.resolve() takes parsed Requirement objects.

So this comment is actually a question for my own education - do URL requirements work today? If so, how? Is this a build time/run time distinction? I have a feeling I have used them successfully since the switch to using pip.

Copy link
Member Author

@jsirois jsirois Mar 19, 2020

Choose a reason for hiding this comment

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

Yes, this is a build time / run time distinction.

These are requirements stored in a built PEX's metadata, and all of those requirements are of the form [key]==[version](; [markers]). They are this way since they're buit from the results of a resolve. That resolve does accept the myriad forms of requirement strings, but it plops out concrete dists. Those concrete dists are used to create the requirements in the PEX metadata used here to boot up at PEX runtime:
https://github.com/pantsbuild/pex/blob/a07954a1bce1cf0c023c67f36e4e37f078eb23ba/pex/resolver.py#L555-L579

Which uses:
https://github.com/pantsbuild/pex/blob/a07954a1bce1cf0c023c67f36e4e37f078eb23ba/pex/resolver.py#L96-L97

Which ues:
https://github.com/pantsbuild/pex/blob/a07954a1bce1cf0c023c67f36e4e37f078eb23ba/pex/vendor/_vendored/setuptools/pkg_resources/__init__.py#L2870-L2877

@jsirois
Copy link
Member Author

jsirois commented Mar 19, 2020

CI is getting hit by tox-dev/tox#1538 - I'll need to pin our tox version - I think - to fix.

Previously Pex did not properly handle activation of wheels with
`Requires-Python` metadata restricting the wheel use to certain Python
interpreter versions.

Fixes pex-tool#898
@jsirois
Copy link
Member Author

jsirois commented Mar 19, 2020

Re-based against tox/venv fix in #924.

@jsirois jsirois merged commit 73ce2f4 into pex-tool:master Mar 19, 2020
@jsirois jsirois deleted the issues/898 branch March 19, 2020 18:48
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.

Top level requirements in PEX-INFO are missing needed environment markers.
2 participants