Merge pull request #9293 from pypa/revert-9264-new-resolver-dont-abort-on-inconsistent-candidate

Author: pradyunsg

news/9264.bugfix.rst

@@ -0,0 +1 @@
+Revert "Skip candidate not providing valid metadata", as that caused pip to be overeager about downloading from the package index.

src/pip/_internal/exceptions.py

@@ -151,21 +151,6 @@ 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 = self._prepare()
+        self._dist = None  # type: Optional[Distribution]
 
     def __str__(self):
         # type: () -> str
@@ -209,6 +209,8 @@ 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)
@@ -217,17 +219,25 @@ def _check_metadata_consistency(self, dist):
             raise MetadataInconsistent(self._ireq, "version", dist.version)
 
     def _prepare(self):
-        # type: () -> Distribution
+        # type: () -> None
+        if self._dist is not None:
+            return
         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)
-        return dist
+        self._dist = dist
+
+    @property
+    def dist(self):
+        # type: () -> Distribution
+        if self._dist is None:
+            self._prepare()
+        return self._dist
 
     def _get_requires_python_dependency(self):
         # type: () -> Optional[Requirement]
@@ -251,6 +261,7 @@ 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,8 +5,6 @@
 from pip._internal.exceptions import (
     DistributionNotFound,
     InstallationError,
-    InstallationSubprocessError,
-    MetadataInconsistent,
     UnsupportedPythonVersion,
     UnsupportedWheel,
 )
@@ -35,7 +33,6 @@
     ExplicitRequirement,
     RequiresPythonRequirement,
     SpecifierRequirement,
-    UnsatisfiableRequirement,
 )
 
 if MYPY_CHECK_RUNNING:
@@ -97,7 +94,6 @@ 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 = {
@@ -140,40 +136,21 @@ def _make_candidate_from_link(
         name,  # type: Optional[str]
         version,  # type: Optional[_BaseVersion]
     ):
-        # type: (...) -> Optional[Candidate]
+        # type: (...) -> 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:
-                    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
+                self._editable_candidate_cache[link] = EditableCandidate(
+                    link, template, factory=self, name=name, version=version,
+                )
             base = self._editable_candidate_cache[link]  # type: BaseCandidate
         else:
             if link not in self._link_candidate_cache:
-                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
+                self._link_candidate_cache[link] = LinkCandidate(
+                    link, template, factory=self, name=name, version=version,
+                )
             base = self._link_candidate_cache[link]
-
         if extras:
             return ExtrasCandidate(base, extras)
         return base
@@ -233,16 +210,13 @@ def iter_index_candidates():
             for ican in reversed(icans):
                 if not all_yanked and ican.link.is_yanked:
                     continue
-                candidate = self._make_candidate_from_link(
+                yield 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,
@@ -306,16 +280,6 @@ 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,44 +158,3 @@ 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 InstallationSubprocessError
+from pip._internal.exceptions import InstallationError
 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,7 +233,11 @@ def call_subprocess(
                     exit_status=proc.returncode,
                 )
                 subprocess_logger.error(msg)
-            raise InstallationSubprocessError(proc.returncode, command_desc)
+            exc_msg = (
+                'Command errored out with exit status {}: {} '
+                'Check the logs for full command output.'
+            ).format(proc.returncode, command_desc)
+            raise InstallationError(exc_msg)
         elif on_returncode == 'warn':
             subprocess_logger.warning(
                 'Command "%s" had error code %s in %s',

tests/functional/test_new_resolver.py

@@ -1218,22 +1218,3 @@ 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 InstallationSubprocessError
+from pip._internal.exceptions import InstallationError
 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(InstallationSubprocessError) as exc:
+        with pytest.raises(InstallationError) 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, (InstallationSubprocessError, 'error', 2)),
+            (3, False, None, INFO, (InstallationError, '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(InstallationSubprocessError):
+        with pytest.raises(InstallationError):
             call_subprocess(
                 [sys.executable, '-c', 'input()'],
                 show_stdout=True,