From c4d9f563dd827906732da466131425b397f68897 Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Sat, 23 Oct 2021 23:49:24 +0800 Subject: [PATCH 1/2] Add failing test for extra-already-installed case --- tests/functional/test_new_resolver.py | 33 +++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/functional/test_new_resolver.py b/tests/functional/test_new_resolver.py index a1e4c9b28d3..abff7d4f056 100644 --- a/tests/functional/test_new_resolver.py +++ b/tests/functional/test_new_resolver.py @@ -2180,3 +2180,36 @@ def test_new_resolver_dont_backtrack_on_extra_if_base_constrained(script): ) assert "pkg-2.0" not in result.stdout, "Should not try 2.0 due to constraint" script.assert_installed(pkg="1.0", dep="1.0") + + +def test_new_resolver_respect_user_requested_if_extra_is_installed(script): + create_basic_wheel_for_package(script, "pkg1", "1.0") + create_basic_wheel_for_package(script, "pkg2", "1.0", extras={"ext": ["pkg1"]}) + create_basic_wheel_for_package(script, "pkg2", "2.0", extras={"ext": ["pkg1"]}) + create_basic_wheel_for_package(script, "pkg3", "1.0", depends=["pkg2[ext]"]) + + # Install pkg3 with an older pkg2. + script.pip( + "install", + "--no-cache-dir", + "--no-index", + "--find-links", + script.scratch_path, + "pkg3", + "pkg2==1.0", + ) + script.assert_installed(pkg3="1.0", pkg2="1.0", pkg1="1.0") + + # Now upgrade both pkg3 and pkg2. pkg2 should be upgraded although pkg2[ext] + # is not requested by the user. + script.pip( + "install", + "--no-cache-dir", + "--no-index", + "--find-links", + script.scratch_path, + "--upgrade", + "pkg3", + "pkg2", + ) + script.assert_installed(pkg3="1.0", pkg2="2.0", pkg1="1.0") From 26eaddcfc972347a89731c1db38a4eb5c66a0baf Mon Sep 17 00:00:00 2001 From: Tzu-ping Chung Date: Sun, 24 Oct 2021 00:10:58 +0800 Subject: [PATCH 2/2] Extract identifier cleanup for user_requested --- news/10613.bugfix.rst | 3 + .../resolution/resolvelib/provider.py | 71 ++++++++++++++----- 2 files changed, 55 insertions(+), 19 deletions(-) create mode 100644 news/10613.bugfix.rst diff --git a/news/10613.bugfix.rst b/news/10613.bugfix.rst new file mode 100644 index 00000000000..b849c4344a6 --- /dev/null +++ b/news/10613.bugfix.rst @@ -0,0 +1,3 @@ +When a package is requested by the user for upgrade, correctly identify that +the extra-ed variant of that same package depended by another user-requested +package is requesting the same package, and upgrade it accordingly. diff --git a/src/pip/_internal/resolution/resolvelib/provider.py b/src/pip/_internal/resolution/resolvelib/provider.py index 579958dddc9..e6ec9594f62 100644 --- a/src/pip/_internal/resolution/resolvelib/provider.py +++ b/src/pip/_internal/resolution/resolvelib/provider.py @@ -1,6 +1,15 @@ import collections import math -from typing import TYPE_CHECKING, Dict, Iterable, Iterator, Mapping, Sequence, Union +from typing import ( + TYPE_CHECKING, + Dict, + Iterable, + Iterator, + Mapping, + Sequence, + TypeVar, + Union, +) from pip._vendor.resolvelib.providers import AbstractProvider @@ -37,6 +46,35 @@ # services to those objects (access to pip's finder and preparer). +D = TypeVar("D") +V = TypeVar("V") + + +def _get_with_identifier( + mapping: Mapping[str, V], + identifier: str, + default: D, +) -> Union[D, V]: + """Get item from a package name lookup mapping with a resolver identifier. + + This extra logic is needed when the target mapping is keyed by package + name, which cannot be directly looked up with an identifier (which may + contain requested extras). Additional logic is added to also look up a value + by "cleaning up" the extras from the identifier. + """ + if identifier in mapping: + return mapping[identifier] + # HACK: Theoretically we should check whether this identifier is a valid + # "NAME[EXTRAS]" format, and parse out the name part with packaging or + # some regular expression. But since pip's resolver only spits out three + # kinds of identifiers: normalized PEP 503 names, normalized names plus + # extras, and Requires-Python, we can cheat a bit here. + name, open_bracket, _ = identifier.partition("[") + if open_bracket and name in mapping: + return mapping[name] + return default + + class PipProvider(_ProviderBase): """Pip's provider implementation for resolvelib. @@ -150,28 +188,13 @@ def get_preference( # type: ignore identifier, ) - def _get_constraint(self, identifier: str) -> Constraint: - if identifier in self._constraints: - return self._constraints[identifier] - - # HACK: Theoretically we should check whether this identifier is a valid - # "NAME[EXTRAS]" format, and parse out the name part with packaging or - # some regular expression. But since pip's resolver only spits out - # three kinds of identifiers: normalized PEP 503 names, normalized names - # plus extras, and Requires-Python, we can cheat a bit here. - name, open_bracket, _ = identifier.partition("[") - if open_bracket and name in self._constraints: - return self._constraints[name] - - return Constraint.empty() - def find_matches( self, identifier: str, requirements: Mapping[str, Iterator[Requirement]], incompatibilities: Mapping[str, Iterator[Candidate]], ) -> Iterable[Candidate]: - def _eligible_for_upgrade(name: str) -> bool: + def _eligible_for_upgrade(identifier: str) -> bool: """Are upgrades allowed for this project? This checks the upgrade strategy, and whether the project was one @@ -185,13 +208,23 @@ def _eligible_for_upgrade(name: str) -> bool: if self._upgrade_strategy == "eager": return True elif self._upgrade_strategy == "only-if-needed": - return name in self._user_requested + user_order = _get_with_identifier( + self._user_requested, + identifier, + default=None, + ) + return user_order is not None return False + constraint = _get_with_identifier( + self._constraints, + identifier, + default=Constraint.empty(), + ) return self._factory.find_candidates( identifier=identifier, requirements=requirements, - constraint=self._get_constraint(identifier), + constraint=constraint, prefers_installed=(not _eligible_for_upgrade(identifier)), incompatibilities=incompatibilities, )