From d45541c8f3b3d87ae55a08d7021e8e879293285c Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Sat, 12 Dec 2020 01:02:59 +0800 Subject: [PATCH 1/2] Skip candidate not providing valid metadata This is done by catching InstallationError from the underlying distribution preparation logic. There are three cases to catch: 1. Candidates from indexes. These are simply ignored since we can potentially satisfy the requirement with other candidates. 2. Candidates from URLs with a dist name (PEP 508 or #egg=). A new UnsatisfiableRequirement class is introduced to represent this; it is like an ExplicitRequirement without an underlying candidate. As the name suggests, an instance of this can never be satisfied, and will cause eventual backtracking. 3. Candidates from URLs without a dist name. This is only possible for top-level user requirements, and no recourse is possible for them. So we error out eagerly. The InstallationError raised during distribution preparation is cached in the factory, like successfully prepared candidates, since we don't want to repeatedly try to build a candidate if we already know it'd fail. Plus pip's preparation logic also does not allow packages to be built multiple times anyway. --- news/9246.bugfix.rst | 2 + src/pip/_internal/exceptions.py | 15 +++++++ .../resolution/resolvelib/candidates.py | 20 ++------- .../resolution/resolvelib/factory.py | 41 +++++++++++++++---- .../resolution/resolvelib/requirements.py | 41 +++++++++++++++++++ src/pip/_internal/utils/subprocess.py | 8 +--- tests/functional/test_new_resolver.py | 19 +++++++++ tests/unit/test_utils_subprocess.py | 8 ++-- 8 files changed, 119 insertions(+), 35 deletions(-) create mode 100644 news/9246.bugfix.rst diff --git a/news/9246.bugfix.rst b/news/9246.bugfix.rst new file mode 100644 index 00000000000..e7ebd398f3e --- /dev/null +++ b/news/9246.bugfix.rst @@ -0,0 +1,2 @@ +New resolver: Discard a candidate if it fails to provide metadata from source, +or if the provided metadata is inconsistent, instead of quitting outright. diff --git a/src/pip/_internal/exceptions.py b/src/pip/_internal/exceptions.py index 56482caf77b..3f2659e8792 100644 --- a/src/pip/_internal/exceptions.py +++ b/src/pip/_internal/exceptions.py @@ -151,6 +151,21 @@ def __str__(self): ) +class InstallationSubprocessError(InstallationError): + """A subprocess call failed during installation.""" + def __init__(self, returncode, description): + # type: (int, str) -> None + self.returncode = returncode + self.description = description + + def __str__(self): + # type: () -> str + return ( + "Command errored out with exit status {}: {} " + "Check the logs for full command output." + ).format(self.returncode, self.description) + + class HashErrors(InstallationError): """Multiple HashError instances rolled into one for reporting""" diff --git a/src/pip/_internal/resolution/resolvelib/candidates.py b/src/pip/_internal/resolution/resolvelib/candidates.py index cd1f188706f..5d838d1d405 100644 --- a/src/pip/_internal/resolution/resolvelib/candidates.py +++ b/src/pip/_internal/resolution/resolvelib/candidates.py @@ -141,7 +141,7 @@ def __init__( self._ireq = ireq self._name = name self._version = version - self._dist = None # type: Optional[Distribution] + self.dist = self._prepare() def __str__(self): # type: () -> str @@ -209,8 +209,6 @@ def _prepare_distribution(self): def _check_metadata_consistency(self, dist): # type: (Distribution) -> None """Check for consistency of project name and version of dist.""" - # TODO: (Longer term) Rather than abort, reject this candidate - # and backtrack. This would need resolvelib support. name = canonicalize_name(dist.project_name) if self._name is not None and self._name != name: raise MetadataInconsistent(self._ireq, "name", dist.project_name) @@ -219,25 +217,14 @@ def _check_metadata_consistency(self, dist): raise MetadataInconsistent(self._ireq, "version", dist.version) def _prepare(self): - # type: () -> None - if self._dist is not None: - return + # type: () -> Distribution try: dist = self._prepare_distribution() except HashError as e: e.req = self._ireq raise - - assert dist is not None, "Distribution already installed" self._check_metadata_consistency(dist) - self._dist = dist - - @property - def dist(self): - # type: () -> Distribution - if self._dist is None: - self._prepare() - return self._dist + return dist def _get_requires_python_dependency(self): # type: () -> Optional[Requirement] @@ -261,7 +248,6 @@ def iter_dependencies(self, with_requires): def get_install_requirement(self): # type: () -> Optional[InstallRequirement] - self._prepare() return self._ireq diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index c723d343bf9..5cd333bad14 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -5,6 +5,8 @@ from pip._internal.exceptions import ( DistributionNotFound, InstallationError, + InstallationSubprocessError, + MetadataInconsistent, UnsupportedPythonVersion, UnsupportedWheel, ) @@ -33,6 +35,7 @@ ExplicitRequirement, RequiresPythonRequirement, SpecifierRequirement, + UnsatisfiableRequirement, ) if MYPY_CHECK_RUNNING: @@ -96,6 +99,7 @@ def __init__( self._link_candidate_cache = {} # type: Cache[LinkCandidate] self._editable_candidate_cache = {} # type: Cache[EditableCandidate] + self._build_failures = {} # type: Cache[InstallationError] if not ignore_installed: self._installed_dists = { @@ -130,20 +134,34 @@ def _make_candidate_from_link( name, # type: Optional[str] version, # type: Optional[_BaseVersion] ): - # type: (...) -> Candidate + # type: (...) -> Optional[Candidate] # TODO: Check already installed candidate, and use it if the link and # editable flag match. + if link in self._build_failures: + return None if template.editable: if link not in self._editable_candidate_cache: - self._editable_candidate_cache[link] = EditableCandidate( - link, template, factory=self, name=name, version=version, - ) + try: + self._editable_candidate_cache[link] = EditableCandidate( + link, template, factory=self, + name=name, version=version, + ) + except (InstallationSubprocessError, MetadataInconsistent) as e: + logger.warning("Discarding %s. %s", link, e) + self._build_failures[link] = e + return None base = self._editable_candidate_cache[link] # type: BaseCandidate else: if link not in self._link_candidate_cache: - self._link_candidate_cache[link] = LinkCandidate( - link, template, factory=self, name=name, version=version, - ) + try: + self._link_candidate_cache[link] = LinkCandidate( + link, template, factory=self, + name=name, version=version, + ) + except (InstallationSubprocessError, MetadataInconsistent) as e: + logger.warning("Discarding %s. %s", link, e) + self._build_failures[link] = e + return None base = self._link_candidate_cache[link] if extras: return ExtrasCandidate(base, extras) @@ -204,13 +222,16 @@ def iter_index_candidates(): for ican in reversed(icans): if not all_yanked and ican.link.is_yanked: continue - yield self._make_candidate_from_link( + candidate = self._make_candidate_from_link( link=ican.link, extras=extras, template=template, name=name, version=ican.version, ) + if candidate is None: + continue + yield candidate return FoundCandidates( iter_index_candidates, @@ -274,6 +295,10 @@ def make_requirement_from_install_req(self, ireq, requested_extras): name=canonicalize_name(ireq.name) if ireq.name else None, version=None, ) + if cand is None: + if not ireq.name: + raise self._build_failures[ireq.link] + return UnsatisfiableRequirement(canonicalize_name(ireq.name)) return self.make_requirement_from_candidate(cand) def make_requirement_from_candidate(self, candidate): diff --git a/src/pip/_internal/resolution/resolvelib/requirements.py b/src/pip/_internal/resolution/resolvelib/requirements.py index d926d0a0656..1229f353750 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -158,3 +158,44 @@ def is_satisfied_by(self, candidate): # already implements the prerelease logic, and would have filtered out # prerelease candidates if the user does not expect them. return self.specifier.contains(candidate.version, prereleases=True) + + +class UnsatisfiableRequirement(Requirement): + """A requirement that cannot be satisfied. + """ + def __init__(self, name): + # type: (str) -> None + self._name = name + + def __str__(self): + # type: () -> str + return "{} (unavailable)".format(self._name) + + def __repr__(self): + # type: () -> str + return "{class_name}({name!r})".format( + class_name=self.__class__.__name__, + name=str(self._name), + ) + + @property + def project_name(self): + # type: () -> str + return self._name + + @property + def name(self): + # type: () -> str + return self._name + + def format_for_error(self): + # type: () -> str + return str(self) + + def get_candidate_lookup(self): + # type: () -> CandidateLookup + return None, None + + def is_satisfied_by(self, candidate): + # type: (Candidate) -> bool + return False diff --git a/src/pip/_internal/utils/subprocess.py b/src/pip/_internal/utils/subprocess.py index 605e711e603..325897c8739 100644 --- a/src/pip/_internal/utils/subprocess.py +++ b/src/pip/_internal/utils/subprocess.py @@ -7,7 +7,7 @@ from pip._vendor.six.moves import shlex_quote from pip._internal.cli.spinners import SpinnerInterface, open_spinner -from pip._internal.exceptions import InstallationError +from pip._internal.exceptions import InstallationSubprocessError from pip._internal.utils.compat import console_to_str, str_to_display from pip._internal.utils.logging import subprocess_logger from pip._internal.utils.misc import HiddenText, path_to_display @@ -233,11 +233,7 @@ def call_subprocess( exit_status=proc.returncode, ) subprocess_logger.error(msg) - exc_msg = ( - 'Command errored out with exit status {}: {} ' - 'Check the logs for full command output.' - ).format(proc.returncode, command_desc) - raise InstallationError(exc_msg) + raise InstallationSubprocessError(proc.returncode, command_desc) elif on_returncode == 'warn': subprocess_logger.warning( 'Command "%s" had error code %s in %s', diff --git a/tests/functional/test_new_resolver.py b/tests/functional/test_new_resolver.py index b730b3cbdf9..00d82fb95ee 100644 --- a/tests/functional/test_new_resolver.py +++ b/tests/functional/test_new_resolver.py @@ -1218,3 +1218,22 @@ def test_new_resolver_does_not_reinstall_when_from_a_local_index(script): assert "Installing collected packages: simple" not in result.stdout, str(result) assert "Requirement already satisfied: simple" in result.stdout, str(result) assert_installed(script, simple="0.1.0") + + +def test_new_resolver_skip_inconsistent_metadata(script): + create_basic_wheel_for_package(script, "A", "1") + + a_2 = create_basic_wheel_for_package(script, "A", "2") + a_2.rename(a_2.parent.joinpath("a-3-py2.py3-none-any.whl")) + + result = script.pip( + "install", + "--no-cache-dir", "--no-index", + "--find-links", script.scratch_path, + "--verbose", + "A", + allow_stderr_warning=True, + ) + + assert " different version in metadata: '2'" in result.stderr, str(result) + assert_installed(script, a="1") diff --git a/tests/unit/test_utils_subprocess.py b/tests/unit/test_utils_subprocess.py index b0de2bf578d..1a031065114 100644 --- a/tests/unit/test_utils_subprocess.py +++ b/tests/unit/test_utils_subprocess.py @@ -7,7 +7,7 @@ import pytest from pip._internal.cli.spinners import SpinnerInterface -from pip._internal.exceptions import InstallationError +from pip._internal.exceptions import InstallationSubprocessError from pip._internal.utils.misc import hide_value from pip._internal.utils.subprocess import ( call_subprocess, @@ -276,7 +276,7 @@ def test_info_logging__subprocess_error(self, capfd, caplog): command = 'print("Hello"); print("world"); exit("fail")' args, spinner = self.prepare_call(caplog, log_level, command=command) - with pytest.raises(InstallationError) as exc: + with pytest.raises(InstallationSubprocessError) as exc: call_subprocess(args, spinner=spinner) result = None exc_message = str(exc.value) @@ -360,7 +360,7 @@ def test_info_logging_with_show_stdout_true(self, capfd, caplog): # log level is only WARNING. (0, True, None, WARNING, (None, 'done', 2)), # Test a non-zero exit status. - (3, False, None, INFO, (InstallationError, 'error', 2)), + (3, False, None, INFO, (InstallationSubprocessError, 'error', 2)), # Test a non-zero exit status also in extra_ok_returncodes. (3, False, (3, ), INFO, (None, 'done', 2)), ]) @@ -396,7 +396,7 @@ def test_spinner_finish( assert spinner.spin_count == expected_spin_count def test_closes_stdin(self): - with pytest.raises(InstallationError): + with pytest.raises(InstallationSubprocessError): call_subprocess( [sys.executable, '-c', 'input()'], show_stdout=True, From b2c04877fa145894579af423f3088b300f74877c Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Sat, 12 Dec 2020 18:47:20 +0800 Subject: [PATCH 2/2] Add comments explaining InstallationError handling --- src/pip/_internal/resolution/resolvelib/candidates.py | 3 +++ src/pip/_internal/resolution/resolvelib/factory.py | 11 +++++++++++ 2 files changed, 14 insertions(+) diff --git a/src/pip/_internal/resolution/resolvelib/candidates.py b/src/pip/_internal/resolution/resolvelib/candidates.py index 5d838d1d405..83b6c98ab6a 100644 --- a/src/pip/_internal/resolution/resolvelib/candidates.py +++ b/src/pip/_internal/resolution/resolvelib/candidates.py @@ -221,6 +221,9 @@ def _prepare(self): try: dist = self._prepare_distribution() except HashError as e: + # Provide HashError the underlying ireq that caused it. This + # provides context for the resulting error message to show the + # offending line to the user. e.req = self._ireq raise self._check_metadata_consistency(dist) diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 5cd333bad14..c7fb4f3f03e 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -137,8 +137,12 @@ def _make_candidate_from_link( # type: (...) -> Optional[Candidate] # TODO: Check already installed candidate, and use it if the link and # editable flag match. + if link in self._build_failures: + # We already tried this candidate before, and it does not build. + # Don't bother trying again. return None + if template.editable: if link not in self._editable_candidate_cache: try: @@ -163,6 +167,7 @@ def _make_candidate_from_link( self._build_failures[link] = e return None base = self._link_candidate_cache[link] + if extras: return ExtrasCandidate(base, extras) return base @@ -296,6 +301,12 @@ def make_requirement_from_install_req(self, ireq, requested_extras): version=None, ) if cand is None: + # There's no way we can satisfy a URL requirement if the underlying + # candidate fails to build. An unnamed URL must be user-supplied, so + # we fail eagerly. If the URL is named, an unsatisfiable requirement + # can make the resolver do the right thing, either backtrack (and + # maybe find some other requirement that's buildable) or raise a + # ResolutionImpossible eventually. if not ireq.name: raise self._build_failures[ireq.link] return UnsatisfiableRequirement(canonicalize_name(ireq.name))