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.

Sign up for GitHub

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 env markers when checking resolves. #861

Merged
merged 2 commits into from
Jan 25, 2020
Merged

Respect env markers when checking resolves. #861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9595959
Respect env markers when checking resolves.
jsirois Jan 23, 2020
42b8101
Make subprocess extra requirement more explicit.
jsirois Jan 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions pex/distribution_target.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
41 changes: 29 additions & 12 deletions pex/resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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
)
Expand All @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know any reasons this might happen? It would help if we could add possible remedies to users. For example, "possibly because your platform does not have any pre-built wheels."

Copy link
Member Author

Choose a reason for hiding this comment

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

This happens due to conflicting requirements. Its the red text you get on a pip install surfaced as an error (pip cannot be made to error for these!). There is no easy advice to give in these cases. Sometimes the conflict is artificial - ie: the pip resolver could solve if it did enough backtracking. Sometimes the conflict is real.

Copy link
Contributor

Choose a reason for hiding this comment

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

This happens due to conflicting requirements. Its the red text you get on a pip install surfaced as an error (pip cannot be made to error for these!). There is no easy advice to give in these cases. Sometimes the conflict is artificial - ie: the pip resolver could solve if it did enough backtracking. Sometimes the conflict is real.

Expressing the first and last sentences would be very helpful, imo. Here's a possible rewording:

{dist} requires {requirement} but {resolved_dist} was resolved. This happens when Pip cannot properly resolve the requirements, usually due to conflicting versions.

While this doesn't pinpoint the exact error, it gives some insight to first audit if there are conflicting versions before looking for other things. Honestly, I didn't think to do that upon first reading the original message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrm. That doesn't really help much. I think I can do better, just a sec...

In the meantime, for example, for the same requirements.

  1. pip exit code 0
ERROR: elasticsearch-curator 5.8.1 has requirement pyyaml==3.13, but you'll have pyyaml 5.3 which is incompatible.
ERROR: elasticsearch-curator 5.8.1 has requirement urllib3<1.25,>=1.24.2, but you'll have urllib3 1.25.8 which is incompatible.
ERROR: moto 1.3.15.dev228 has requirement python-dateutil<2.8.1,>=2.1, but you'll have python-dateutil 2.8.1 which is incompatible.
ERROR: pantsbuild-pants 1.25.0.dev1 has requirement docutils==0.14, but you'll have docutils 0.15.2 which is incompatible.
ERROR: pantsbuild-pants 1.25.0.dev1 has requirement packaging==16.8, but you'll have packaging 20.1 which is incompatible.
ERROR: pantsbuild-pants 1.25.0.dev1 has requirement pex==1.6.12, but you'll have pex 2.1.0 which is incompatible.
ERROR: pantsbuild-pants 1.25.0.dev1 has requirement PyYAML==5.1.2, but you'll have pyyaml 5.3 which is incompatible.
ERROR: pantsbuild-pants 1.25.0.dev1 has requirement wheel==0.31.1, but you'll have wheel 0.33.6 which is incompatible.
ERROR: pantsbuild-pants-contrib-mypy 1.25.0.dev1 has requirement pex==1.6.12, but you'll have pex 2.1.0 which is incompatible.
  1. pex exit code 1
Failed to resolve compatible distributions:
1: pantsbuild.pants.contrib.mypy==1.25.0.dev1 requires pex==1.6.12 but pex 2.1.0 was resolved
2: pantsbuild.pants==1.25.0.dev1 requires pex==1.6.12 but pex 2.1.0 was resolved
3: pantsbuild.pants==1.25.0.dev1 requires packaging==16.8 but packaging 20.1 was resolved
4: pantsbuild.pants==1.25.0.dev1 requires wheel==0.31.1 but wheel 0.33.6 was resolved
5: pantsbuild.pants==1.25.0.dev1 requires docutils==0.14 but docutils 0.15.2 was resolved
6: pantsbuild.pants==1.25.0.dev1 requires PyYAML==5.1.2 but PyYAML 5.3 was resolved
7: elasticsearch-curator==5.8.1 requires pyyaml==3.13 but PyYAML 5.3 was resolved
8: elasticsearch-curator==5.8.1 requires urllib3<1.25,>=1.24.2 but urllib3 1.25.8 was resolved
9: moto==1.3.15.dev228 requires python-dateutil<2.8.1,>=2.1 but python-dateutil 2.8.1 was resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. You know what, I take it back. This is actually pretty intuitive! I was having a hard time filling in the blanks of what the message would actually look like.

Copy link
Member Author

@jsirois jsirois Jan 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

How about this mod some formatting / coalescing in a follow-up?:

Failed to resolve compatible distributions:
1: pantsbuild.pants.contrib.mypy==1.25.0.dev1 requires pex==1.6.12 but pex 2.1.0 was resolved.
Other requirements for pex were:
	pantsbuild.pants 1.25.0.dev1 requires pex==1.6.12
2: pantsbuild.pants==1.25.0.dev1 requires packaging==16.8 but packaging 20.1 was resolved.
Other requirements for packaging were:
	pytest 5.3.4 requires packaging
3: pantsbuild.pants==1.25.0.dev1 requires PyYAML==5.1.2 but PyYAML 5.3 was resolved.
Other requirements for pyyaml were:
	cfn-lint 0.27.2 requires pyyaml; python_version != "3.4"
	moto 1.3.15.dev228 requires PyYAML>=5.1
	kubernetes 10.0.1 requires pyyaml>=3.12
	yq 2.8.1 requires PyYAML>=3.11
	elasticsearch-curator 5.8.1 requires pyyaml==3.13
4: pantsbuild.pants==1.25.0.dev1 requires pex==1.6.12 but pex 2.1.0 was resolved.
Other requirements for pex were:
	pantsbuild.pants.contrib.mypy 1.25.0.dev1 requires pex==1.6.12
5: pantsbuild.pants==1.25.0.dev1 requires wheel==0.31.1 but wheel 0.33.6 was resolved.
Other requirements for wheel were:
	
6: pantsbuild.pants==1.25.0.dev1 requires docutils==0.14 but docutils 0.15.2 was resolved.
Other requirements for docutils were:
	botocore 1.14.9 requires docutils<0.16,>=0.10
7: moto==1.3.15.dev228 requires python-dateutil<2.8.1,>=2.1 but python-dateutil 2.8.1 was resolved.
Other requirements for python-dateutil were:
	kubernetes 10.0.1 requires python-dateutil>=2.5.3
	freezegun 0.3.14 requires python-dateutil!=2.0,>=1.0
	elasticsearch-dsl 7.1.0 requires python-dateutil
	botocore 1.14.9 requires python-dateutil<3.0.0,>=2.1
8: elasticsearch-curator==5.8.1 requires urllib3<1.25,>=1.24.2 but urllib3 1.25.8 was resolved.
Other requirements for urllib3 were:
	sentry-sdk 0.14.0 requires urllib3>=1.10.0
	requests 2.22.0 requires urllib3!=1.25.0,!=1.25.1,<1.26,>=1.21.1
	kubernetes 10.0.1 requires urllib3>=1.24.2
	elasticsearch 7.5.1 requires urllib3>=1.21.1
	botocore 1.14.9 requires urllib3<1.26,>=1.20
9: elasticsearch-curator==5.8.1 requires pyyaml==3.13 but PyYAML 5.3 was resolved.
Other requirements for pyyaml were:
	cfn-lint 0.27.2 requires pyyaml; python_version != "3.4"
	pantsbuild.pants 1.25.0.dev1 requires PyYAML==5.1.2
	moto 1.3.15.dev228 requires PyYAML>=5.1
	kubernetes 10.0.1 requires pyyaml>=3.12
	yq 2.8.1 requires PyYAML>=3.11

Copy link
Contributor

Choose a reason for hiding this comment

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

That is extremely helpful! I wonder if Pip would accept that as a contribution.

dist=dist.as_requirement(),
requirement=requirement,
resolved_dist=resolved_dist
)
)

if unsatisfied:
raise Unsatisfiable(
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
26 changes: 26 additions & 0 deletions tests/test_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from pex.testing import (
IS_LINUX,
IS_PYPY,
PY27,
PY35,
PY36,
PY_VER,
Expand Down Expand Up @@ -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