From 9ec7e36dd6e1ef80c80c9fefde69bf97631f51c9 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Mon, 6 Nov 2023 17:13:58 +0100 Subject: [PATCH 1/2] Fixed bug in extras handling for link requirements --- news/12372.bugfix.rst | 1 + .../resolution/resolvelib/factory.py | 53 +++++++++++++------ tests/functional/test_install_reqs.py | 6 +-- tests/functional/test_new_resolver.py | 21 ++++++++ 4 files changed, 62 insertions(+), 19 deletions(-) create mode 100644 news/12372.bugfix.rst diff --git a/news/12372.bugfix.rst b/news/12372.bugfix.rst new file mode 100644 index 00000000000..6c0b48dec3c --- /dev/null +++ b/news/12372.bugfix.rst @@ -0,0 +1 @@ +Fix a bug in extras handling for link requirements diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 38c199448a1..241b74b59bb 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -36,7 +36,10 @@ from pip._internal.models.link import Link from pip._internal.models.wheel import Wheel from pip._internal.operations.prepare import RequirementPreparer -from pip._internal.req.constructors import install_req_from_link_and_ireq +from pip._internal.req.constructors import ( + install_req_drop_extras, + install_req_from_link_and_ireq, +) from pip._internal.req.req_install import ( InstallRequirement, check_invalid_constraint_type, @@ -176,6 +179,20 @@ def _make_candidate_from_link( name: Optional[NormalizedName], version: Optional[CandidateVersion], ) -> Optional[Candidate]: + base: Optional[BaseCandidate] = self._make_base_candidate_from_link( + link, template, name, version + ) + if not extras or base is None: + return base + return self._make_extras_candidate(base, extras, comes_from=template) + + def _make_base_candidate_from_link( + self, + link: Link, + template: InstallRequirement, + name: Optional[NormalizedName], + version: Optional[CandidateVersion], + ) -> Optional[BaseCandidate]: # TODO: Check already installed candidate, and use it if the link and # editable flag match. @@ -204,7 +221,7 @@ def _make_candidate_from_link( self._build_failures[link] = e return None - base: BaseCandidate = self._editable_candidate_cache[link] + return self._editable_candidate_cache[link] else: if link not in self._link_candidate_cache: try: @@ -224,11 +241,7 @@ def _make_candidate_from_link( ) self._build_failures[link] = e return None - base = self._link_candidate_cache[link] - - if not extras: - return base - return self._make_extras_candidate(base, extras, comes_from=template) + return self._link_candidate_cache[link] def _iter_found_candidates( self, @@ -362,9 +375,8 @@ def _iter_candidates_from_constraints( """ for link in constraint.links: self._fail_if_link_is_unsupported_wheel(link) - candidate = self._make_candidate_from_link( + candidate = self._make_base_candidate_from_link( link, - extras=frozenset(), template=install_req_from_link_and_ireq(link, template), name=canonicalize_name(identifier), version=None, @@ -454,10 +466,10 @@ def _make_requirements_from_install_req( Returns requirement objects associated with the given InstallRequirement. In most cases this will be a single object but the following special cases exist: - the InstallRequirement has markers that do not apply -> result is empty - - the InstallRequirement has both a constraint and extras -> result is split - in two requirement objects: one with the constraint and one with the - extra. This allows centralized constraint handling for the base, - resulting in fewer candidate rejections. + - the InstallRequirement has both a constraint (or link) and extras + -> result is split in two requirement objects: one with the constraint + (or link) and one with the extra. This allows centralized constraint + handling for the base, resulting in fewer candidate rejections. """ if not ireq.match_markers(requested_extras): logger.info( @@ -471,10 +483,13 @@ def _make_requirements_from_install_req( yield SpecifierRequirement(ireq) else: self._fail_if_link_is_unsupported_wheel(ireq.link) - cand = self._make_candidate_from_link( + # Always make the link candidate for the base requirement to make it + # available to `find_candidates` for explicit candidate lookup for any + # set of extras. + # The extras are required separately via a second requirement. + cand = self._make_base_candidate_from_link( ireq.link, - extras=frozenset(ireq.extras), - template=ireq, + template=install_req_drop_extras(ireq) if ireq.extras else ireq, name=canonicalize_name(ireq.name) if ireq.name else None, version=None, ) @@ -489,7 +504,13 @@ def _make_requirements_from_install_req( raise self._build_failures[ireq.link] yield UnsatisfiableRequirement(canonicalize_name(ireq.name)) else: + # require the base from the link yield self.make_requirement_from_candidate(cand) + if ireq.extras: + # require the extras on top of the base candidate + yield self.make_requirement_from_candidate( + self._make_extras_candidate(cand, frozenset(ireq.extras)) + ) def collect_root_requirements( self, root_ireqs: List[InstallRequirement] diff --git a/tests/functional/test_install_reqs.py b/tests/functional/test_install_reqs.py index c21b9ba83de..f649be00090 100644 --- a/tests/functional/test_install_reqs.py +++ b/tests/functional/test_install_reqs.py @@ -671,9 +671,9 @@ def test_install_distribution_union_with_versions( expect_error=(resolver_variant == "resolvelib"), ) if resolver_variant == "resolvelib": - assert "Cannot install localextras[bar]" in result.stderr - assert ("localextras[bar] 0.0.1 depends on localextras 0.0.1") in result.stdout - assert ("localextras[baz] 0.0.2 depends on localextras 0.0.2") in result.stdout + assert "Cannot install localextras" in result.stderr + assert ("The user requested localextras 0.0.1") in result.stdout + assert ("The user requested localextras 0.0.2") in result.stdout else: assert ( "Successfully installed LocalExtras-0.0.1 simple-3.0 singlemodule-0.0.1" diff --git a/tests/functional/test_new_resolver.py b/tests/functional/test_new_resolver.py index b5945edf89b..1e7672a46ae 100644 --- a/tests/functional/test_new_resolver.py +++ b/tests/functional/test_new_resolver.py @@ -2406,6 +2406,27 @@ def test_new_resolver_respect_user_requested_if_extra_is_installed( script.assert_installed(pkg3="1.0", pkg2="2.0", pkg1="1.0") +def test_new_resolver_constraint_on_link_with_extra( + script: PipTestEnvironment, +) -> None: + """ + Verify that installing works from a link with both an extra and a constraint. + """ + wheel: pathlib.Path = create_basic_wheel_for_package( + script, "pkg", "1.0", extras={"ext": []} + ) + + script.pip( + "install", + "--no-cache-dir", + # no index, no --find-links: only the explicit path + "--no-index", + f"{wheel}[ext]", + "pkg==1", + ) + script.assert_installed(pkg="1.0") + + def test_new_resolver_do_not_backtrack_on_build_failure( script: PipTestEnvironment, ) -> None: From 83e41d90afd2bfe941b48e5f9f3ed22eab7d1146 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Sun, 10 Dec 2023 20:43:20 +0100 Subject: [PATCH 2/2] added second test case --- tests/functional/test_new_resolver.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/functional/test_new_resolver.py b/tests/functional/test_new_resolver.py index 1e7672a46ae..dfcf97ab132 100644 --- a/tests/functional/test_new_resolver.py +++ b/tests/functional/test_new_resolver.py @@ -2427,6 +2427,30 @@ def test_new_resolver_constraint_on_link_with_extra( script.assert_installed(pkg="1.0") +def test_new_resolver_constraint_on_link_with_extra_indirect( + script: PipTestEnvironment, +) -> None: + """ + Verify that installing works from a link with an extra if there is an indirect + dependency on that same package with the same extra (#12372). + """ + wheel_one: pathlib.Path = create_basic_wheel_for_package( + script, "pkg1", "1.0", extras={"ext": []} + ) + wheel_two: pathlib.Path = create_basic_wheel_for_package( + script, "pkg2", "1.0", depends=["pkg1[ext]==1.0"] + ) + + script.pip( + "install", + "--no-cache-dir", + # no index, no --find-links: only the explicit path + wheel_two, + f"{wheel_one}[ext]", + ) + script.assert_installed(pkg1="1.0", pkg2="1.0") + + def test_new_resolver_do_not_backtrack_on_build_failure( script: PipTestEnvironment, ) -> None: