From cfac6aebdd10d1b992b689a4d17ee784b6ebc1ed Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Fri, 10 Apr 2020 19:53:18 +0800 Subject: [PATCH 1/3] Always return an install candidate last if matches This rewrites how a SpecifierRequirement generates candidates, so it * Always return an AlreadyInstalledCandidate (as long as the version satisfies the specifier), even if PackageFinder does not return a candidate for the same version. * Always put the AlreadyInstalledCandidate last, so it's preferred over LinkCandidate, preventing version changes if possible. --- .../resolution/resolvelib/factory.py | 53 +++++++++-------- .../resolution/resolvelib/requirements.py | 15 +---- tests/functional/test_new_resolver.py | 57 +++++++++++++++++++ 3 files changed, 89 insertions(+), 36 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 30993d99553..9367bed4ded 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -17,14 +17,13 @@ ) if MYPY_CHECK_RUNNING: - from typing import Dict, Optional, Set, Tuple, TypeVar + from typing import Dict, Iterator, Optional, Set, Tuple, TypeVar from pip._vendor.packaging.specifiers import SpecifierSet from pip._vendor.packaging.version import _BaseVersion from pip._vendor.pkg_resources import Distribution from pip._internal.index.package_finder import PackageFinder - from pip._internal.models.candidate import InstallationCandidate from pip._internal.models.link import Link from pip._internal.operations.prepare import RequirementPreparer from pip._internal.req.req_install import InstallRequirement @@ -88,6 +87,8 @@ def _make_candidate_from_link( version=None, # type: Optional[_BaseVersion] ): # type: (...) -> Candidate + # TODO: Check already installed candidate, and use it if the link and + # editable flag match. if parent.editable: if link not in self._editable_candidate_cache: self._editable_candidate_cache[link] = EditableCandidate( @@ -104,32 +105,38 @@ def _make_candidate_from_link( return ExtrasCandidate(base, extras) return base - def make_candidate_from_ican( - self, - ican, # type: InstallationCandidate - extras, # type: Set[str] - parent, # type: InstallRequirement - ): - # type: (...) -> Candidate - dist = self._installed_dists.get(ican.name) - should_use_installed_dist = ( - not self._force_reinstall and - dist is not None and - dist.parsed_version == ican.version + def iter_found_candidates(self, ireq, extras): + # type: (InstallRequirement, Set[str]) -> Iterator[Candidate] + name = canonicalize_name(ireq.req.name) + if not self._force_reinstall: + dist = self._installed_dists.get(name) + else: + dist = None + + found = self.finder.find_best_candidate( + project_name=ireq.req.name, + specifier=ireq.req.specifier, + hashes=ireq.hashes(trust_internet=False), ) - if not should_use_installed_dist: - return self._make_candidate_from_link( + for ican in found.iter_applicable(): + if dist is not None and dist.parsed_version == ican.version: + continue + yield self._make_candidate_from_link( link=ican.link, extras=extras, - parent=parent, - name=canonicalize_name(ican.name), + parent=ireq, + name=name, version=ican.version, ) - return self._make_candidate_from_dist( - dist=dist, - extras=extras, - parent=parent, - ) + + # Return installed distribution if it matches the specifier. This is + # done last so the resolver will prefer it over downloading links. + if dist is not None and dist.parsed_version in ireq.req.specifier: + yield self._make_candidate_from_dist( + dist=dist, + extras=extras, + parent=ireq, + ) def make_requirement_from_install_req(self, ireq): # type: (InstallRequirement) -> Requirement diff --git a/src/pip/_internal/resolution/resolvelib/requirements.py b/src/pip/_internal/resolution/resolvelib/requirements.py index 027e36aa61e..cd413efac1d 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -94,19 +94,8 @@ def name(self): def find_matches(self): # type: () -> Sequence[Candidate] - found = self._factory.finder.find_best_candidate( - project_name=self._ireq.req.name, - specifier=self._ireq.req.specifier, - hashes=self._ireq.hashes(trust_internet=False), - ) - return [ - self._factory.make_candidate_from_ican( - ican=ican, - extras=self.extras, - parent=self._ireq, - ) - for ican in found.iter_applicable() - ] + it = self._factory.iter_found_candidates(self._ireq, self.extras) + return list(it) def is_satisfied_by(self, candidate): # type: (Candidate) -> bool diff --git a/tests/functional/test_new_resolver.py b/tests/functional/test_new_resolver.py index 25c726f2574..5ee60cd14ab 100644 --- a/tests/functional/test_new_resolver.py +++ b/tests/functional/test_new_resolver.py @@ -88,6 +88,63 @@ def test_new_resolver_picks_latest_version(script): assert_installed(script, simple="0.2.0") +def test_new_resolver_picks_installed_version(script): + create_basic_wheel_for_package( + script, + "simple", + "0.1.0", + ) + create_basic_wheel_for_package( + script, + "simple", + "0.2.0", + ) + script.pip( + "install", "--unstable-feature=resolver", + "--no-cache-dir", "--no-index", + "--find-links", script.scratch_path, + "simple==0.1.0" + ) + assert_installed(script, simple="0.1.0") + + result = script.pip( + "install", "--unstable-feature=resolver", + "--no-cache-dir", "--no-index", + "--find-links", script.scratch_path, + "simple" + ) + assert "Collecting" not in result.stdout, "Should not fetch new version" + assert_installed(script, simple="0.1.0") + + +def test_new_resolver_picks_installed_version_if_no_match_found(script): + create_basic_wheel_for_package( + script, + "simple", + "0.1.0", + ) + create_basic_wheel_for_package( + script, + "simple", + "0.2.0", + ) + script.pip( + "install", "--unstable-feature=resolver", + "--no-cache-dir", "--no-index", + "--find-links", script.scratch_path, + "simple==0.1.0" + ) + assert_installed(script, simple="0.1.0") + + result = script.pip( + "install", "--unstable-feature=resolver", + "--no-cache-dir", "--no-index", + "simple" + ) + assert "Collecting" not in result.stdout, "Should not fetch new version" + assert_installed(script, simple="0.1.0") + + def test_new_resolver_installs_dependencies(script): create_basic_wheel_for_package( script, From 84b99c20b852d6dcdb0fdc48e38bb03b639b576d Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Mon, 13 Apr 2020 18:05:24 +0800 Subject: [PATCH 2/3] Canonicalize installed distribution keys --- src/pip/_internal/resolution/resolvelib/factory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 9367bed4ded..fb5d26abe23 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -60,7 +60,7 @@ def __init__( if not ignore_installed: self._installed_dists = { - dist.project_name: dist + canonicalize_name(dist.project_name): dist for dist in get_installed_distributions() } else: From 9c97b285b9a3df360034417ddc4f8d2d7f29a4b6 Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Thu, 16 Apr 2020 13:44:13 +0800 Subject: [PATCH 3/3] Rename variable for clarity --- src/pip/_internal/resolution/resolvelib/factory.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index fb5d26abe23..d6236d77984 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -109,9 +109,9 @@ def iter_found_candidates(self, ireq, extras): # type: (InstallRequirement, Set[str]) -> Iterator[Candidate] name = canonicalize_name(ireq.req.name) if not self._force_reinstall: - dist = self._installed_dists.get(name) + installed_dist = self._installed_dists.get(name) else: - dist = None + installed_dist = None found = self.finder.find_best_candidate( project_name=ireq.req.name, @@ -119,7 +119,8 @@ def iter_found_candidates(self, ireq, extras): hashes=ireq.hashes(trust_internet=False), ) for ican in found.iter_applicable(): - if dist is not None and dist.parsed_version == ican.version: + if (installed_dist is not None and + installed_dist.parsed_version == ican.version): continue yield self._make_candidate_from_link( link=ican.link, @@ -131,9 +132,10 @@ def iter_found_candidates(self, ireq, extras): # Return installed distribution if it matches the specifier. This is # done last so the resolver will prefer it over downloading links. - if dist is not None and dist.parsed_version in ireq.req.specifier: + if (installed_dist is not None and + installed_dist.parsed_version in ireq.req.specifier): yield self._make_candidate_from_dist( - dist=dist, + dist=installed_dist, extras=extras, parent=ireq, )