Merge pull request #9264 from uranusjr/new-resolver-dont-abort-on-inconsistent-candidate

Skip candidate not providing valid metadata

Author: pradyunsg

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.

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"""
 

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,17 @@ 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:
+            # 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
-
-        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 +251,6 @@ def iter_dependencies(self, with_requires):
 
     def get_install_requirement(self):
         # type: () -> Optional[InstallRequirement]
-        self._prepare()
         return self._ireq
 
 

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:
@@ -94,6 +97,7 @@ def __init__(
         self._force_reinstall = force_reinstall
         self._ignore_requires_python = ignore_requires_python
 
+        self._build_failures = {}  # type: Cache[InstallationError]
         self._link_candidate_cache = {}  # type: Cache[LinkCandidate]
         self._editable_candidate_cache = {}  # type: Cache[EditableCandidate]
         self._installed_candidate_cache = {
@@ -136,21 +140,40 @@ 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:
+            # 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:
-                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)
         return base
@@ -210,13 +233,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,
@@ -280,6 +306,16 @@ 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:
+            # 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))
         return self.make_requirement_from_candidate(cand)
 
     def make_requirement_from_candidate(self, candidate):

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

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',

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")

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,