From 959595994c24a427b3fce606f228062f3da27b7e Mon Sep 17 00:00:00 2001 From: John Sirois Date: Wed, 22 Jan 2020 20:09:28 -0800 Subject: [PATCH 1/2] Respect env markers when checking resolves. Previously we failed to do this and would erroneously fail resolves as a result. Fixes #851 --- pex/distribution_target.py | 15 ++++++++++++++ pex/resolver.py | 41 +++++++++++++++++++++++++++----------- tests/test_resolver.py | 26 ++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 12 deletions(-) diff --git a/pex/distribution_target.py b/pex/distribution_target.py index d1ec090d0..0ec8878a5 100644 --- a/pex/distribution_target.py +++ b/pex/distribution_target.py @@ -38,6 +38,21 @@ def get_interpreter(self): def get_platform(self): return self._platform or Platform.current() + def requirement_applies(self, requirement): + """Determines if the given requirement applies to this distribution target. + + :param requirement: The requirement to evaluate. + :type requirement: :class:`pex.third_party.pkg_resources.Requirement` + :rtype: bool + """ + if not requirement.marker: + return True + + if self._interpreter is None: + return True + + return requirement.marker.evaluate(self._interpreter.identity.env_markers) + @property def id(self): """A unique id for a resolve target suitable as a path name component. diff --git a/pex/resolver.py b/pex/resolver.py index 79a09ee24..261aa9cd3 100644 --- a/pex/resolver.py +++ b/pex/resolver.py @@ -33,13 +33,15 @@ class Unsatisfiable(Exception): pass -class ResolvedDistribution(namedtuple('ResolvedDistribution', ['requirement', 'distribution'])): - """A requirement and the resolved distribution that satisfies it.""" +class ResolvedDistribution(namedtuple('ResolvedDistribution', + ['target', 'requirement', 'distribution'])): + """A distribution target, requirement and the resolved distribution that satisfies them both.""" - def __new__(cls, requirement, distribution): + def __new__(cls, target, requirement, distribution): + assert isinstance(target, DistributionTarget) assert isinstance(requirement, Requirement) assert isinstance(distribution, Distribution) - return super(ResolvedDistribution, cls).__new__(cls, requirement, distribution) + return super(ResolvedDistribution, cls).__new__(cls, target, requirement, distribution) class DistributionRequirements(object): @@ -565,6 +567,7 @@ def add_requirements_requests(install_result): for distribution in requirements_request.distributions: resolved_distributions.add( ResolvedDistribution( + target=requirements_request.target, requirement=distribution_requirements.to_requirement(distribution), distribution=distribution ) @@ -575,23 +578,37 @@ def add_requirements_requests(install_result): return resolved_distributions def _check_resolve(self, resolved_distributions): - dist_by_key = OrderedDict( - (resolved_distribution.requirement.key, resolved_distribution.distribution) + resolved_distribution_by_key = OrderedDict( + (resolved_distribution.requirement.key, resolved_distribution) for resolved_distribution in resolved_distributions ) unsatisfied = [] - for dist in dist_by_key.values(): + for resolved_distribution in resolved_distribution_by_key.values(): + dist = resolved_distribution.distribution + target = resolved_distribution.target for requirement in dist.requires(): - resolved_dist = dist_by_key.get(requirement.key) - if not resolved_dist or resolved_dist not in requirement: + if not target.requirement_applies(requirement): + continue + + resolved_requirement_dist = resolved_distribution_by_key.get(requirement.key) + if not resolved_requirement_dist: unsatisfied.append( - '{dist} requires {requirement} but {resolved_dist} was resolved'.format( + '{dist} requires {requirement} but no version was resolved'.format( dist=dist.as_requirement(), - requirement=requirement, - resolved_dist=resolved_dist.as_requirement() if resolved_dist else None + requirement=requirement ) ) + else: + resolved_dist = resolved_requirement_dist.distribution + if resolved_dist not in requirement: + unsatisfied.append( + '{dist} requires {requirement} but {resolved_dist} was resolved'.format( + dist=dist.as_requirement(), + requirement=requirement, + resolved_dist=resolved_dist + ) + ) if unsatisfied: raise Unsatisfiable( diff --git a/tests/test_resolver.py b/tests/test_resolver.py index 5a4aaddba..ae8791432 100644 --- a/tests/test_resolver.py +++ b/tests/test_resolver.py @@ -13,6 +13,7 @@ from pex.testing import ( IS_LINUX, IS_PYPY, + PY27, PY35, PY36, PY_VER, @@ -282,3 +283,28 @@ def test_resolve_foreign_abi3(): 'cryptography-2.8-cp34-abi3-manylinux1_x86_64.whl', 'cryptography-2.8-cp34-abi3-macosx_10_6_intel.whl' } == set(wheel_names) + + +def test_issues_851(): + # Previously, the PY36 resolve would fail post-resolution checks for configparser, pathlib2 and + # contextlib2 which are only required for python_version<3. + + def resolve_pytest(python_version, pytest_version): + interpreter = PythonInterpreter.from_binary(ensure_python_interpreter(python_version)) + resolved_dists = resolve_multi(interpreters=[interpreter], + requirements=['pytest=={}'.format(pytest_version)]) + project_to_version = {rd.requirement.key: rd.distribution.version for rd in resolved_dists} + assert project_to_version['pytest'] == pytest_version + return project_to_version + + resolved_project_to_version = resolve_pytest(python_version=PY36, pytest_version='5.3.4') + assert 'importlib-metadata' in resolved_project_to_version + assert 'configparser' not in resolved_project_to_version + assert 'pathlib2' not in resolved_project_to_version + assert 'contextlib2' not in resolved_project_to_version + + resolved_project_to_version = resolve_pytest(python_version=PY27, pytest_version='4.6.9') + assert 'importlib-metadata' in resolved_project_to_version + assert 'configparser' in resolved_project_to_version + assert 'pathlib2' in resolved_project_to_version + assert 'contextlib2' in resolved_project_to_version From 42b8101f67880b6541ff6f767d8fb09ba8d19fa4 Mon Sep 17 00:00:00 2001 From: John Sirois Date: Fri, 24 Jan 2020 18:00:56 -0800 Subject: [PATCH 2/2] Make subprocess extra requirement more explicit. Even though things were fine as-is, adding an environment marker that re-iterates the subprocess marker only applies for python_version<3 makes it both clearer at the point of use and clearer this can be dropped when / if we eventually frop python2 support. --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 002fb5bfe..42aef4304 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -27,7 +27,7 @@ requires-python = ">=2.7,<3.9,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*,!=3.4.*" [tool.flit.metadata.requires-extra] # For improved subprocess robustness under python2.7. -subprocess = ["subprocess32>=3.2.7"] +subprocess = ["subprocess32>=3.2.7; python_version<'3'"] [tool.flit.scripts] pex = "pex.bin.pex:main"