Skip to content

Commit

Permalink
Merge pull request pypa#9293 from pypa/revert-9264-new-resolver-dont-…
Browse files Browse the repository at this point in the history
…abort-on-inconsistent-candidate
  • Loading branch information
pradyunsg authored Dec 15, 2020
2 parents 03d5f56 + 95c3ae3 commit b4fb710
Show file tree
Hide file tree
Showing 8 changed files with 36 additions and 131 deletions.
1 change: 1 addition & 0 deletions news/9264.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Revert "Skip candidate not providing valid metadata", as that caused pip to be overeager about downloading from the package index.
15 changes: 0 additions & 15 deletions src/pip/_internal/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"""

Expand Down
23 changes: 17 additions & 6 deletions src/pip/_internal/resolution/resolvelib/candidates.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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]
Expand All @@ -251,6 +261,7 @@ def iter_dependencies(self, with_requires):

def get_install_requirement(self):
# type: () -> Optional[InstallRequirement]
self._prepare()
return self._ireq


Expand Down
52 changes: 8 additions & 44 deletions src/pip/_internal/resolution/resolvelib/factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
from pip._internal.exceptions import (
DistributionNotFound,
InstallationError,
InstallationSubprocessError,
MetadataInconsistent,
UnsupportedPythonVersion,
UnsupportedWheel,
)
Expand Down Expand Up @@ -35,7 +33,6 @@
ExplicitRequirement,
RequiresPythonRequirement,
SpecifierRequirement,
UnsatisfiableRequirement,
)

if MYPY_CHECK_RUNNING:
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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):
Expand Down
41 changes: 0 additions & 41 deletions src/pip/_internal/resolution/resolvelib/requirements.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
8 changes: 6 additions & 2 deletions src/pip/_internal/utils/subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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',
Expand Down
19 changes: 0 additions & 19 deletions tests/functional/test_new_resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
8 changes: 4 additions & 4 deletions tests/unit/test_utils_subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)),
])
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit b4fb710

Please sign in to comment.