Merge pull request #10615 from uranusjr/new-resolver-respect-extra-in-user-requested

Author: pradyunsg

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.

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

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