From 5bebe850ea1db6ff165e4b95bafb1ee44e4a69e8 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Tue, 20 Jun 2023 17:13:18 +0200 Subject: [PATCH 01/40] take non-extra requirements into account for extra installs --- src/pip/_internal/resolution/resolvelib/factory.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 0331297b85b..c117a30c81c 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -385,8 +385,8 @@ def find_candidates( if ireq is not None: ireqs.append(ireq) - # If the current identifier contains extras, add explicit candidates - # from entries from extra-less identifier. + # If the current identifier contains extras, add requires and explicit + # candidates from entries from extra-less identifier. with contextlib.suppress(InvalidRequirement): parsed_requirement = get_requirement(identifier) explicit_candidates.update( @@ -395,6 +395,10 @@ def find_candidates( frozenset(parsed_requirement.extras), ), ) + for req in requirements.get(parsed_requirement.name, []): + _, ireq = req.get_candidate_lookup() + if ireq is not None: + ireqs.append(ireq) # Add explicit candidates from constraints. We only do this if there are # known ireqs, which represent requirements not already explicit. If From 937d8f0b61dbf41f23db9ba62586a6bf6d45c828 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 21 Jun 2023 17:34:30 +0200 Subject: [PATCH 02/40] partial improvement --- src/pip/_internal/req/constructors.py | 32 +++++++++++- .../resolution/resolvelib/candidates.py | 9 ++-- .../resolution/resolvelib/factory.py | 51 ++++++++++++++----- .../resolution/resolvelib/provider.py | 2 + .../resolution/resolvelib/requirements.py | 28 ++++++++-- .../resolution_resolvelib/test_requirement.py | 22 ++++---- 6 files changed, 109 insertions(+), 35 deletions(-) diff --git a/src/pip/_internal/req/constructors.py b/src/pip/_internal/req/constructors.py index c5ca2d85d51..f04a4cbbdbd 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -15,7 +15,7 @@ from pip._vendor.packaging.markers import Marker from pip._vendor.packaging.requirements import InvalidRequirement, Requirement -from pip._vendor.packaging.specifiers import Specifier +from pip._vendor.packaging.specifiers import Specifier, SpecifierSet from pip._internal.exceptions import InstallationError from pip._internal.models.index import PyPI, TestPyPI @@ -504,3 +504,33 @@ def install_req_from_link_and_ireq( config_settings=ireq.config_settings, user_supplied=ireq.user_supplied, ) + + +def install_req_without( + ireq: InstallRequirement, + *, + without_extras: bool = False, + without_specifier: bool = False, +) -> InstallRequirement: + # TODO: clean up hack + req = Requirement(str(ireq.req)) + if without_extras: + req.extras = {} + if without_specifier: + req.specifier = SpecifierSet(prereleases=req.specifier.prereleases) + return InstallRequirement( + req=req, + comes_from=ireq.comes_from, + editable=ireq.editable, + link=ireq.link, + markers=ireq.markers, + use_pep517=ireq.use_pep517, + isolated=ireq.isolated, + global_options=ireq.global_options, + hash_options=ireq.hash_options, + constraint=ireq.constraint, + extras=ireq.extras if not without_extras else [], + config_settings=ireq.config_settings, + user_supplied=ireq.user_supplied, + permit_editable_wheels=ireq.permit_editable_wheels, + ) diff --git a/src/pip/_internal/resolution/resolvelib/candidates.py b/src/pip/_internal/resolution/resolvelib/candidates.py index de04e1d73f2..5bac3d6df1f 100644 --- a/src/pip/_internal/resolution/resolvelib/candidates.py +++ b/src/pip/_internal/resolution/resolvelib/candidates.py @@ -237,10 +237,11 @@ def _prepare(self) -> BaseDistribution: self._check_metadata_consistency(dist) return dist + # TODO: add Explicit dependency on self to extra reqs can benefit from it? def iter_dependencies(self, with_requires: bool) -> Iterable[Optional[Requirement]]: requires = self.dist.iter_dependencies() if with_requires else () for r in requires: - yield self._factory.make_requirement_from_spec(str(r), self._ireq) + yield from self._factory.make_requirements_from_spec(str(r), self._ireq) yield self._factory.make_requires_python_requirement(self.dist.requires_python) def get_install_requirement(self) -> Optional[InstallRequirement]: @@ -392,7 +393,7 @@ def iter_dependencies(self, with_requires: bool) -> Iterable[Optional[Requiremen if not with_requires: return for r in self.dist.iter_dependencies(): - yield self._factory.make_requirement_from_spec(str(r), self._ireq) + yield from self._factory.make_requirements_from_spec(str(r), self._ireq) def get_install_requirement(self) -> Optional[InstallRequirement]: return None @@ -502,11 +503,9 @@ def iter_dependencies(self, with_requires: bool) -> Iterable[Optional[Requiremen ) for r in self.base.dist.iter_dependencies(valid_extras): - requirement = factory.make_requirement_from_spec( + yield from factory.make_requirements_from_spec( str(r), self.base._ireq, valid_extras ) - if requirement: - yield requirement def get_install_requirement(self) -> Optional[InstallRequirement]: # We don't return anything here, because we always diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index c117a30c81c..4c088209b29 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -441,18 +441,35 @@ def find_candidates( and all(req.is_satisfied_by(c) for req in requirements[identifier]) ) - def _make_requirement_from_install_req( + def _make_requirements_from_install_req( self, ireq: InstallRequirement, requested_extras: Iterable[str] - ) -> Optional[Requirement]: + ) -> list[Requirement]: + # TODO: docstring + """ + 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. + """ + # TODO: implement -> split in base req with constraint and extra req without if not ireq.match_markers(requested_extras): logger.info( "Ignoring %s: markers '%s' don't match your environment", ireq.name, ireq.markers, ) - return None + return [] if not ireq.link: - return SpecifierRequirement(ireq) + if ireq.extras and ireq.req.specifier: + return [ + SpecifierRequirement(ireq, drop_extras=True), + SpecifierRequirement(ireq, drop_specifier=True), + ] + else: + return [SpecifierRequirement(ireq)] self._fail_if_link_is_unsupported_wheel(ireq.link) cand = self._make_candidate_from_link( ireq.link, @@ -470,8 +487,9 @@ def _make_requirement_from_install_req( # 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) + return [UnsatisfiableRequirement(canonicalize_name(ireq.name))] + # TODO: here too + return [self.make_requirement_from_candidate(cand)] def collect_root_requirements( self, root_ireqs: List[InstallRequirement] @@ -492,15 +510,17 @@ def collect_root_requirements( else: collected.constraints[name] = Constraint.from_ireq(ireq) else: - req = self._make_requirement_from_install_req( + reqs = self._make_requirements_from_install_req( ireq, requested_extras=(), ) - if req is None: + if not reqs: continue - if ireq.user_supplied and req.name not in collected.user_requested: - collected.user_requested[req.name] = i - collected.requirements.append(req) + + # TODO: clean up reqs[0]? + if ireq.user_supplied and reqs[0].name not in collected.user_requested: + collected.user_requested[reqs[0].name] = i + collected.requirements.extend(reqs) return collected def make_requirement_from_candidate( @@ -508,14 +528,17 @@ def make_requirement_from_candidate( ) -> ExplicitRequirement: return ExplicitRequirement(candidate) - def make_requirement_from_spec( + def make_requirements_from_spec( self, specifier: str, comes_from: Optional[InstallRequirement], requested_extras: Iterable[str] = (), - ) -> Optional[Requirement]: + ) -> list[Requirement]: + # TODO: docstring + """ + """ ireq = self._make_install_req_from_spec(specifier, comes_from) - return self._make_requirement_from_install_req(ireq, requested_extras) + return self._make_requirements_from_install_req(ireq, requested_extras) def make_requires_python_requirement( self, diff --git a/src/pip/_internal/resolution/resolvelib/provider.py b/src/pip/_internal/resolution/resolvelib/provider.py index 315fb9c8902..121e48d071b 100644 --- a/src/pip/_internal/resolution/resolvelib/provider.py +++ b/src/pip/_internal/resolution/resolvelib/provider.py @@ -184,6 +184,8 @@ def get_preference( # the backtracking backtrack_cause = self.is_backtrack_cause(identifier, backtrack_causes) + # TODO: finally prefer base over extra for the same package + return ( not requires_python, not direct, diff --git a/src/pip/_internal/resolution/resolvelib/requirements.py b/src/pip/_internal/resolution/resolvelib/requirements.py index 06addc0ddce..fe9ae6ba661 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -2,6 +2,7 @@ from pip._vendor.packaging.utils import NormalizedName, canonicalize_name from pip._internal.req.req_install import InstallRequirement +from pip._internal.req.constructors import install_req_without from .base import Candidate, CandidateLookup, Requirement, format_name @@ -39,14 +40,27 @@ def is_satisfied_by(self, candidate: Candidate) -> bool: return candidate == self.candidate +# TODO: add some comments class SpecifierRequirement(Requirement): - def __init__(self, ireq: InstallRequirement) -> None: + # TODO: document additional options + def __init__( + self, + ireq: InstallRequirement, + *, + drop_extras: bool = False, + drop_specifier: bool = False, + ) -> None: assert ireq.link is None, "This is a link, not a specifier" - self._ireq = ireq - self._extras = frozenset(ireq.extras) + self._drop_extras: bool = drop_extras + self._original_extras = frozenset(ireq.extras) + # TODO: name + self._original_req = ireq.req + self._ireq = install_req_without( + ireq, without_extras=self._drop_extras, without_specifier=drop_specifier + ) def __str__(self) -> str: - return str(self._ireq.req) + return str(self._original_req) def __repr__(self) -> str: return "{class_name}({requirement!r})".format( @@ -59,9 +73,13 @@ def project_name(self) -> NormalizedName: assert self._ireq.req, "Specifier-backed ireq is always PEP 508" return canonicalize_name(self._ireq.req.name) + # TODO: make sure this can still be identified for error reporting purposes @property def name(self) -> str: - return format_name(self.project_name, self._extras) + return format_name( + self.project_name, + self._original_extras if not self._drop_extras else frozenset(), + ) def format_for_error(self) -> str: # Convert comma-separated specifiers into "A, B, ..., F and G" diff --git a/tests/unit/resolution_resolvelib/test_requirement.py b/tests/unit/resolution_resolvelib/test_requirement.py index 6864e70ea0a..ce48ab16c49 100644 --- a/tests/unit/resolution_resolvelib/test_requirement.py +++ b/tests/unit/resolution_resolvelib/test_requirement.py @@ -61,9 +61,9 @@ def test_new_resolver_requirement_has_name( ) -> None: """All requirements should have a name""" for spec, name, _ in test_cases: - req = factory.make_requirement_from_spec(spec, comes_from=None) - assert req is not None - assert req.name == name + reqs = factory.make_requirements_from_spec(spec, comes_from=None) + assert len(reqs) == 1 + assert reqs[0].name == name def test_new_resolver_correct_number_of_matches( @@ -71,8 +71,9 @@ def test_new_resolver_correct_number_of_matches( ) -> None: """Requirements should return the correct number of candidates""" for spec, _, match_count in test_cases: - req = factory.make_requirement_from_spec(spec, comes_from=None) - assert req is not None + reqs = factory.make_requirements_from_spec(spec, comes_from=None) + assert len(reqs) == 1 + req = reqs[0] matches = factory.find_candidates( req.name, {req.name: [req]}, @@ -88,8 +89,9 @@ def test_new_resolver_candidates_match_requirement( ) -> None: """Candidates returned from find_candidates should satisfy the requirement""" for spec, _, _ in test_cases: - req = factory.make_requirement_from_spec(spec, comes_from=None) - assert req is not None + reqs = factory.make_requirements_from_spec(spec, comes_from=None) + assert len(reqs) == 1 + req = reqs[0] candidates = factory.find_candidates( req.name, {req.name: [req]}, @@ -104,8 +106,8 @@ def test_new_resolver_candidates_match_requirement( def test_new_resolver_full_resolve(factory: Factory, provider: PipProvider) -> None: """A very basic full resolve""" - req = factory.make_requirement_from_spec("simplewheel", comes_from=None) - assert req is not None + reqs = factory.make_requirements_from_spec("simplewheel", comes_from=None) + assert len(reqs) == 1 r: Resolver[Requirement, Candidate, str] = Resolver(provider, BaseReporter()) - result = r.resolve([req]) + result = r.resolve([reqs[0]]) assert set(result.mapping.keys()) == {"simplewheel"} From 5f8f40eb1d0610e530d5e035ba8c7f99d9af9df1 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Thu, 22 Jun 2023 11:08:33 +0200 Subject: [PATCH 03/40] refinements --- src/pip/_internal/req/constructors.py | 3 +- .../resolution/resolvelib/candidates.py | 15 +++++++- .../resolution/resolvelib/factory.py | 38 +++++++++++-------- .../resolution/resolvelib/requirements.py | 18 ++++----- 4 files changed, 46 insertions(+), 28 deletions(-) diff --git a/src/pip/_internal/req/constructors.py b/src/pip/_internal/req/constructors.py index f04a4cbbdbd..908876c4cde 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -520,7 +520,8 @@ def install_req_without( req.specifier = SpecifierSet(prereleases=req.specifier.prereleases) return InstallRequirement( req=req, - comes_from=ireq.comes_from, + # TODO: document this!!!! + comes_from=ireq, editable=ireq.editable, link=ireq.link, markers=ireq.markers, diff --git a/src/pip/_internal/resolution/resolvelib/candidates.py b/src/pip/_internal/resolution/resolvelib/candidates.py index 5bac3d6df1f..23883484139 100644 --- a/src/pip/_internal/resolution/resolvelib/candidates.py +++ b/src/pip/_internal/resolution/resolvelib/candidates.py @@ -237,7 +237,6 @@ def _prepare(self) -> BaseDistribution: self._check_metadata_consistency(dist) return dist - # TODO: add Explicit dependency on self to extra reqs can benefit from it? def iter_dependencies(self, with_requires: bool) -> Iterable[Optional[Requirement]]: requires = self.dist.iter_dependencies() if with_requires else () for r in requires: @@ -428,9 +427,19 @@ def __init__( self, base: BaseCandidate, extras: FrozenSet[str], + ireq: Optional[InstallRequirement] = None, ) -> None: + """ + :param ireq: the InstallRequirement that led to this candidate, if it + differs from the base's InstallRequirement. This will often be the + case in the sense that this candidate's requirement has the extras + while the base's does not. Unlike the InstallRequirement backed + candidates, this requirement is used solely for reporting purposes, + it does not do any leg work. + """ self.base = base self.extras = extras + self._ireq = ireq def __str__(self) -> str: name, rest = str(self.base).split(" ", 1) @@ -504,7 +513,9 @@ def iter_dependencies(self, with_requires: bool) -> Iterable[Optional[Requiremen for r in self.base.dist.iter_dependencies(valid_extras): yield from factory.make_requirements_from_spec( - str(r), self.base._ireq, valid_extras + str(r), + self._ireq if self._ireq is not None else self.base._ireq, + valid_extras, ) def get_install_requirement(self) -> Optional[InstallRequirement]: diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 4c088209b29..45b8133878b 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -138,13 +138,16 @@ def _fail_if_link_is_unsupported_wheel(self, link: Link) -> None: raise UnsupportedWheel(msg) def _make_extras_candidate( - self, base: BaseCandidate, extras: FrozenSet[str] + self, + base: BaseCandidate, + extras: FrozenSet[str], + ireq: Optional[InstallRequirement] = None, ) -> ExtrasCandidate: cache_key = (id(base), extras) try: candidate = self._extras_candidate_cache[cache_key] except KeyError: - candidate = ExtrasCandidate(base, extras) + candidate = ExtrasCandidate(base, extras, ireq=ireq) self._extras_candidate_cache[cache_key] = candidate return candidate @@ -161,7 +164,7 @@ def _make_candidate_from_dist( self._installed_candidate_cache[dist.canonical_name] = base if not extras: return base - return self._make_extras_candidate(base, extras) + return self._make_extras_candidate(base, extras, ireq=template) def _make_candidate_from_link( self, @@ -223,7 +226,7 @@ def _make_candidate_from_link( if not extras: return base - return self._make_extras_candidate(base, extras) + return self._make_extras_candidate(base, extras, ireq=template) def _iter_found_candidates( self, @@ -389,16 +392,17 @@ def find_candidates( # candidates from entries from extra-less identifier. with contextlib.suppress(InvalidRequirement): parsed_requirement = get_requirement(identifier) - explicit_candidates.update( - self._iter_explicit_candidates_from_base( - requirements.get(parsed_requirement.name, ()), - frozenset(parsed_requirement.extras), - ), - ) - for req in requirements.get(parsed_requirement.name, []): - _, ireq = req.get_candidate_lookup() - if ireq is not None: - ireqs.append(ireq) + if parsed_requirement.name != identifier: + explicit_candidates.update( + self._iter_explicit_candidates_from_base( + requirements.get(parsed_requirement.name, ()), + frozenset(parsed_requirement.extras), + ), + ) + for req in requirements.get(parsed_requirement.name, []): + _, ireq = req.get_candidate_lookup() + if ireq is not None: + ireqs.append(ireq) # Add explicit candidates from constraints. We only do this if there are # known ireqs, which represent requirements not already explicit. If @@ -444,7 +448,6 @@ def find_candidates( def _make_requirements_from_install_req( self, ireq: InstallRequirement, requested_extras: Iterable[str] ) -> list[Requirement]: - # TODO: docstring """ Returns requirement objects associated with the given InstallRequirement. In most cases this will be a single object but the following special cases exist: @@ -454,7 +457,6 @@ def _make_requirements_from_install_req( extra. This allows centralized constraint handling for the base, resulting in fewer candidate rejections. """ - # TODO: implement -> split in base req with constraint and extra req without if not ireq.match_markers(requested_extras): logger.info( "Ignoring %s: markers '%s' don't match your environment", @@ -466,6 +468,10 @@ def _make_requirements_from_install_req( if ireq.extras and ireq.req.specifier: return [ SpecifierRequirement(ireq, drop_extras=True), + # TODO: put this all the way at the back to have even fewer candidates? + # TODO: probably best to keep specifier as it makes the report + # slightly more readable -> should also update SpecReq constructor + # and req.constructors.install_req_without SpecifierRequirement(ireq, drop_specifier=True), ] else: diff --git a/src/pip/_internal/resolution/resolvelib/requirements.py b/src/pip/_internal/resolution/resolvelib/requirements.py index fe9ae6ba661..180158128af 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -40,7 +40,6 @@ def is_satisfied_by(self, candidate: Candidate) -> bool: return candidate == self.candidate -# TODO: add some comments class SpecifierRequirement(Requirement): # TODO: document additional options def __init__( @@ -52,15 +51,17 @@ def __init__( ) -> None: assert ireq.link is None, "This is a link, not a specifier" self._drop_extras: bool = drop_extras - self._original_extras = frozenset(ireq.extras) - # TODO: name - self._original_req = ireq.req - self._ireq = install_req_without( - ireq, without_extras=self._drop_extras, without_specifier=drop_specifier + self._extras = frozenset(ireq.extras if not drop_extras else ()) + self._ireq = ( + ireq + if not drop_extras and not drop_specifier + else install_req_without( + ireq, without_extras=self._drop_extras, without_specifier=drop_specifier + ) ) def __str__(self) -> str: - return str(self._original_req) + return str(self._ireq) def __repr__(self) -> str: return "{class_name}({requirement!r})".format( @@ -73,12 +74,11 @@ def project_name(self) -> NormalizedName: assert self._ireq.req, "Specifier-backed ireq is always PEP 508" return canonicalize_name(self._ireq.req.name) - # TODO: make sure this can still be identified for error reporting purposes @property def name(self) -> str: return format_name( self.project_name, - self._original_extras if not self._drop_extras else frozenset(), + self._extras, ) def format_for_error(self) -> str: From d09431feb5049ec5e7a9b4ecb5d338a38a14ffc4 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Thu, 22 Jun 2023 14:42:05 +0200 Subject: [PATCH 04/40] fixes --- src/pip/_internal/req/constructors.py | 20 +++++++++++++++++-- .../resolution/resolvelib/resolver.py | 15 +++++++++++++- tests/functional/test_install.py | 6 +++--- 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/pip/_internal/req/constructors.py b/src/pip/_internal/req/constructors.py index 908876c4cde..9bf1c98440b 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -8,10 +8,11 @@ InstallRequirement. """ +import copy import logging import os import re -from typing import Dict, List, Optional, Set, Tuple, Union +from typing import Collection, Dict, List, Optional, Set, Tuple, Union from pip._vendor.packaging.markers import Marker from pip._vendor.packaging.requirements import InvalidRequirement, Requirement @@ -512,7 +513,6 @@ def install_req_without( without_extras: bool = False, without_specifier: bool = False, ) -> InstallRequirement: - # TODO: clean up hack req = Requirement(str(ireq.req)) if without_extras: req.extras = {} @@ -535,3 +535,19 @@ def install_req_without( user_supplied=ireq.user_supplied, permit_editable_wheels=ireq.permit_editable_wheels, ) + + +def install_req_extend_extras( + ireq: InstallRequirement, + extras: Collection[str], +) -> InstallRequirement: + """ + Returns a copy of an installation requirement with some additional extras. + Makes a shallow copy of the ireq object. + """ + result = copy.copy(ireq) + req = Requirement(str(ireq.req)) + req.extras.update(extras) + result.req = req + result.extras = {*ireq.extras, *extras} + return result diff --git a/src/pip/_internal/resolution/resolvelib/resolver.py b/src/pip/_internal/resolution/resolvelib/resolver.py index 47bbfecce36..c5de0e822c9 100644 --- a/src/pip/_internal/resolution/resolvelib/resolver.py +++ b/src/pip/_internal/resolution/resolvelib/resolver.py @@ -1,3 +1,4 @@ +import contextlib import functools import logging import os @@ -11,6 +12,7 @@ from pip._internal.cache import WheelCache from pip._internal.index.package_finder import PackageFinder from pip._internal.operations.prepare import RequirementPreparer +from pip._internal.req.constructors import install_req_extend_extras from pip._internal.req.req_install import InstallRequirement from pip._internal.req.req_set import RequirementSet from pip._internal.resolution.base import BaseResolver, InstallRequirementProvider @@ -19,6 +21,7 @@ PipDebuggingReporter, PipReporter, ) +from pip._internal.utils.packaging import get_requirement from .base import Candidate, Requirement from .factory import Factory @@ -101,9 +104,19 @@ def resolve( raise error from e req_set = RequirementSet(check_supported_wheels=check_supported_wheels) - for candidate in result.mapping.values(): + # sort to ensure base candidates come before candidates with extras + for candidate in sorted(result.mapping.values(), key=lambda c: c.name): ireq = candidate.get_install_requirement() if ireq is None: + if candidate.name != candidate.project_name: + # extend existing req's extras + with contextlib.suppress(KeyError): + req = req_set.get_requirement(candidate.project_name) + req_set.add_named_requirement( + install_req_extend_extras( + req, get_requirement(candidate.name).extras + ) + ) continue # Check if there is already an installation under the same name, diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index 8559d93684b..f5ac31a8e8c 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -2465,6 +2465,6 @@ def test_install_pip_prints_req_chain_pypi(script: PipTestEnvironment) -> None: ) assert ( - f"Collecting python-openid " - f"(from Paste[openid]==1.7.5.1->-r {req_path} (line 1))" in result.stdout - ) + "Collecting python-openid " + f"(from Paste[openid]->Paste[openid]==1.7.5.1->-r {req_path} (line 1))" + ) in result.stdout From 49027d7de3c9441b612c65ac68ec39e893a8385f Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Thu, 22 Jun 2023 14:59:43 +0200 Subject: [PATCH 05/40] cleanup --- src/pip/_internal/req/constructors.py | 20 +++++------ .../resolution/resolvelib/factory.py | 34 ++++++++++++------- .../resolution/resolvelib/requirements.py | 18 ++++------ 3 files changed, 37 insertions(+), 35 deletions(-) diff --git a/src/pip/_internal/req/constructors.py b/src/pip/_internal/req/constructors.py index 9bf1c98440b..8b1438afe1e 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -507,20 +507,16 @@ def install_req_from_link_and_ireq( ) -def install_req_without( - ireq: InstallRequirement, - *, - without_extras: bool = False, - without_specifier: bool = False, -) -> InstallRequirement: +def install_req_drop_extras(ireq: InstallRequirement) -> InstallRequirement: + """ + Creates a new InstallationRequirement using the given template but without + any extras. Sets the original requirement as the new one's parent + (comes_from). + """ req = Requirement(str(ireq.req)) - if without_extras: - req.extras = {} - if without_specifier: - req.specifier = SpecifierSet(prereleases=req.specifier.prereleases) + req.extras = {} return InstallRequirement( req=req, - # TODO: document this!!!! comes_from=ireq, editable=ireq.editable, link=ireq.link, @@ -530,7 +526,7 @@ def install_req_without( global_options=ireq.global_options, hash_options=ireq.hash_options, constraint=ireq.constraint, - extras=ireq.extras if not without_extras else [], + extras=[], config_settings=ireq.config_settings, user_supplied=ireq.user_supplied, permit_editable_wheels=ireq.permit_editable_wheels, diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 45b8133878b..0c2c6ab793d 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -468,18 +468,18 @@ def _make_requirements_from_install_req( if ireq.extras and ireq.req.specifier: return [ SpecifierRequirement(ireq, drop_extras=True), - # TODO: put this all the way at the back to have even fewer candidates? - # TODO: probably best to keep specifier as it makes the report - # slightly more readable -> should also update SpecReq constructor - # and req.constructors.install_req_without - SpecifierRequirement(ireq, drop_specifier=True), + # TODO: put this all the way at the back to have even fewer + # candidates? + SpecifierRequirement(ireq), ] else: return [SpecifierRequirement(ireq)] self._fail_if_link_is_unsupported_wheel(ireq.link) cand = self._make_candidate_from_link( ireq.link, - extras=frozenset(ireq.extras), + # make just the base candidate so the corresponding requirement can be split + # in case of extras (see docstring) + extras=frozenset(), template=ireq, name=canonicalize_name(ireq.name) if ireq.name else None, version=None, @@ -494,8 +494,12 @@ def _make_requirements_from_install_req( if not ireq.name: raise self._build_failures[ireq.link] return [UnsatisfiableRequirement(canonicalize_name(ireq.name))] - # TODO: here too - return [self.make_requirement_from_candidate(cand)] + return [ + self.make_requirement_from_candidate(cand), + self.make_requirement_from_candidate( + self._make_extras_candidate(cand, frozenset(ireq.extras), ireq) + ), + ] def collect_root_requirements( self, root_ireqs: List[InstallRequirement] @@ -523,9 +527,9 @@ def collect_root_requirements( if not reqs: continue - # TODO: clean up reqs[0]? - if ireq.user_supplied and reqs[0].name not in collected.user_requested: - collected.user_requested[reqs[0].name] = i + template = reqs[0] + if ireq.user_supplied and template.name not in collected.user_requested: + collected.user_requested[template.name] = i collected.requirements.extend(reqs) return collected @@ -540,8 +544,14 @@ def make_requirements_from_spec( comes_from: Optional[InstallRequirement], requested_extras: Iterable[str] = (), ) -> list[Requirement]: - # TODO: docstring """ + Returns requirement objects associated with the given specifier. In most cases + this will be a single object but the following special cases exist: + - the specifier has markers that do not apply -> result is empty + - the specifier 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. """ ireq = self._make_install_req_from_spec(specifier, comes_from) return self._make_requirements_from_install_req(ireq, requested_extras) diff --git a/src/pip/_internal/resolution/resolvelib/requirements.py b/src/pip/_internal/resolution/resolvelib/requirements.py index 180158128af..31a515da9ac 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -2,7 +2,7 @@ from pip._vendor.packaging.utils import NormalizedName, canonicalize_name from pip._internal.req.req_install import InstallRequirement -from pip._internal.req.constructors import install_req_without +from pip._internal.req.constructors import install_req_drop_extras from .base import Candidate, CandidateLookup, Requirement, format_name @@ -41,24 +41,20 @@ def is_satisfied_by(self, candidate: Candidate) -> bool: class SpecifierRequirement(Requirement): - # TODO: document additional options def __init__( self, ireq: InstallRequirement, *, drop_extras: bool = False, - drop_specifier: bool = False, ) -> None: + """ + :param drop_extras: Ignore any extras that are part of the install requirement, + making this a requirement on the base only. + """ assert ireq.link is None, "This is a link, not a specifier" self._drop_extras: bool = drop_extras - self._extras = frozenset(ireq.extras if not drop_extras else ()) - self._ireq = ( - ireq - if not drop_extras and not drop_specifier - else install_req_without( - ireq, without_extras=self._drop_extras, without_specifier=drop_specifier - ) - ) + self._ireq = ireq if not drop_extras else install_req_drop_extras(ireq) + self._extras = frozenset(self._ireq.extras) def __str__(self) -> str: return str(self._ireq) From cb0f97f70e8b7fbc89e63ed1d2fb5b2dd233fafb Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Thu, 22 Jun 2023 15:56:23 +0200 Subject: [PATCH 06/40] reverted troublesome changes --- src/pip/_internal/resolution/resolvelib/factory.py | 11 ++--------- tests/functional/test_install.py | 6 +++--- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 0c2c6ab793d..847cbee8dc8 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -477,9 +477,7 @@ def _make_requirements_from_install_req( self._fail_if_link_is_unsupported_wheel(ireq.link) cand = self._make_candidate_from_link( ireq.link, - # make just the base candidate so the corresponding requirement can be split - # in case of extras (see docstring) - extras=frozenset(), + extras=frozenset(ireq.extras), template=ireq, name=canonicalize_name(ireq.name) if ireq.name else None, version=None, @@ -494,12 +492,7 @@ def _make_requirements_from_install_req( if not ireq.name: raise self._build_failures[ireq.link] return [UnsatisfiableRequirement(canonicalize_name(ireq.name))] - return [ - self.make_requirement_from_candidate(cand), - self.make_requirement_from_candidate( - self._make_extras_candidate(cand, frozenset(ireq.extras), ireq) - ), - ] + return [self.make_requirement_from_candidate(cand)] def collect_root_requirements( self, root_ireqs: List[InstallRequirement] diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index f5ac31a8e8c..8559d93684b 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -2465,6 +2465,6 @@ def test_install_pip_prints_req_chain_pypi(script: PipTestEnvironment) -> None: ) assert ( - "Collecting python-openid " - f"(from Paste[openid]->Paste[openid]==1.7.5.1->-r {req_path} (line 1))" - ) in result.stdout + f"Collecting python-openid " + f"(from Paste[openid]==1.7.5.1->-r {req_path} (line 1))" in result.stdout + ) From 3160293193d947eecb16c2d69d754b04d98b2bab Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Thu, 22 Jun 2023 16:10:09 +0200 Subject: [PATCH 07/40] improvement --- src/pip/_internal/resolution/resolvelib/factory.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 847cbee8dc8..03820edde6a 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -468,8 +468,6 @@ def _make_requirements_from_install_req( if ireq.extras and ireq.req.specifier: return [ SpecifierRequirement(ireq, drop_extras=True), - # TODO: put this all the way at the back to have even fewer - # candidates? SpecifierRequirement(ireq), ] else: @@ -524,6 +522,15 @@ def collect_root_requirements( if ireq.user_supplied and template.name not in collected.user_requested: collected.user_requested[template.name] = i collected.requirements.extend(reqs) + # Put requirements with extras at the end of the root requires. This does not + # affect resolvelib's picking preference but it does affect its initial criteria + # population: by putting extras at the end we enable the candidate finder to + # present resolvelib with a smaller set of candidates to resolvelib, already + # taking into account any non-transient constraints on the associated base. This + # means resolvelib will have fewer candidates to visit and reject. + # Python's list sort is stable, meaning relative order is kept for objects with + # the same key. + collected.requirements.sort(key=lambda r: r.name != r.project_name) return collected def make_requirement_from_candidate( From 1038f15496f037181a18e5d67d8a0f33e5cb7cc9 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Thu, 22 Jun 2023 16:24:26 +0200 Subject: [PATCH 08/40] stray todo --- src/pip/_internal/resolution/resolvelib/provider.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/provider.py b/src/pip/_internal/resolution/resolvelib/provider.py index 121e48d071b..315fb9c8902 100644 --- a/src/pip/_internal/resolution/resolvelib/provider.py +++ b/src/pip/_internal/resolution/resolvelib/provider.py @@ -184,8 +184,6 @@ def get_preference( # the backtracking backtrack_cause = self.is_backtrack_cause(identifier, backtrack_causes) - # TODO: finally prefer base over extra for the same package - return ( not requires_python, not direct, From 8aa17580ed623d926795e0cfb8885b4a4b4e044e Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Thu, 13 Jul 2023 14:57:18 +0200 Subject: [PATCH 09/40] dropped unused attribute --- src/pip/_internal/resolution/resolvelib/requirements.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/pip/_internal/resolution/resolvelib/requirements.py b/src/pip/_internal/resolution/resolvelib/requirements.py index 31a515da9ac..e23b948ffc2 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -52,7 +52,6 @@ def __init__( making this a requirement on the base only. """ assert ireq.link is None, "This is a link, not a specifier" - self._drop_extras: bool = drop_extras self._ireq = ireq if not drop_extras else install_req_drop_extras(ireq) self._extras = frozenset(self._ireq.extras) From faa3289a94c59cdf647ce9d9c9277714c9363a62 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Thu, 13 Jul 2023 16:40:56 +0200 Subject: [PATCH 10/40] use regex for requirement update --- src/pip/_internal/req/constructors.py | 28 +++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/pip/_internal/req/constructors.py b/src/pip/_internal/req/constructors.py index 8b1438afe1e..ee38b9a6183 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -58,6 +58,26 @@ def convert_extras(extras: Optional[str]) -> Set[str]: return get_requirement("placeholder" + extras.lower()).extras +def _set_requirement_extras(req: Requirement, new_extras: Set[str]) -> Requirement: + """ + Returns a new requirement based on the given one, with the supplied extras. If the + given requirement already has extras those are replaced (or dropped if no new extras + are given). + """ + match: re.Match = re.fullmatch(r"([^;\[<>~=]+)(\[[^\]]*\])?(.*)", str(req)) + # ireq.req is a valid requirement so the regex should match + assert match is not None + pre: Optional[str] = match.group(1) + post: Optional[str] = match.group(3) + assert pre is not None and post is not None + extras: str = ( + "[%s]" % ",".join(sorted(new_extras)) + if new_extras + else "" + ) + return Requirement(pre + extras + post) + + def parse_editable(editable_req: str) -> Tuple[Optional[str], str, Set[str]]: """Parses an editable requirement into: - a requirement name @@ -513,10 +533,8 @@ def install_req_drop_extras(ireq: InstallRequirement) -> InstallRequirement: any extras. Sets the original requirement as the new one's parent (comes_from). """ - req = Requirement(str(ireq.req)) - req.extras = {} return InstallRequirement( - req=req, + req=_set_requirement_extras(ireq.req, set()), comes_from=ireq, editable=ireq.editable, link=ireq.link, @@ -542,8 +560,6 @@ def install_req_extend_extras( Makes a shallow copy of the ireq object. """ result = copy.copy(ireq) - req = Requirement(str(ireq.req)) - req.extras.update(extras) - result.req = req result.extras = {*ireq.extras, *extras} + result.req = _set_requirement_extras(ireq.req, result.extras) return result From 7e8da6176f9da32e44b8a1515e450ca8158a356a Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Thu, 13 Jul 2023 17:02:53 +0200 Subject: [PATCH 11/40] clarification --- src/pip/_internal/req/constructors.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/req/constructors.py b/src/pip/_internal/req/constructors.py index ee38b9a6183..f97bded9887 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -65,7 +65,7 @@ def _set_requirement_extras(req: Requirement, new_extras: Set[str]) -> Requireme are given). """ match: re.Match = re.fullmatch(r"([^;\[<>~=]+)(\[[^\]]*\])?(.*)", str(req)) - # ireq.req is a valid requirement so the regex should match + # ireq.req is a valid requirement so the regex should always match assert match is not None pre: Optional[str] = match.group(1) post: Optional[str] = match.group(3) From ff9aeae0d2e39720e40e8fffae942d659495fd84 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Tue, 25 Jul 2023 15:36:33 +0200 Subject: [PATCH 12/40] added resolver test case --- tests/functional/test_new_resolver.py | 82 +++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/tests/functional/test_new_resolver.py b/tests/functional/test_new_resolver.py index fc52ab9c8d8..88dd635ae26 100644 --- a/tests/functional/test_new_resolver.py +++ b/tests/functional/test_new_resolver.py @@ -2272,6 +2272,88 @@ def test_new_resolver_dont_backtrack_on_extra_if_base_constrained( script.assert_installed(pkg="1.0", dep="1.0") +@pytest.mark.parametrize("swap_order", (True, False)) +@pytest.mark.parametrize("two_extras", (True, False)) +def test_new_resolver_dont_backtrack_on_extra_if_base_constrained_in_requirement( + script: PipTestEnvironment, swap_order: bool, two_extras: bool +) -> None: + """ + Verify that a requirement with a constraint on a package (either on the base + on the base with an extra) causes the resolver to infer the same constraint for + any (other) extras with the same base. + + :param swap_order: swap the order the install specifiers appear in + :param two_extras: also add an extra for the constrained specifier + """ + create_basic_wheel_for_package(script, "dep", "1.0") + create_basic_wheel_for_package( + script, "pkg", "1.0", extras={"ext1": ["dep"], "ext2": ["dep"]} + ) + create_basic_wheel_for_package( + script, "pkg", "2.0", extras={"ext1": ["dep"], "ext2": ["dep"]} + ) + + to_install: tuple[str, str] = ( + "pkg[ext1]", "pkg[ext2]==1.0" if two_extras else "pkg==1.0" + ) + + result = script.pip( + "install", + "--no-cache-dir", + "--no-index", + "--find-links", + script.scratch_path, + *(to_install if not swap_order else reversed(to_install)), + ) + 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") + + +@pytest.mark.parametrize("swap_order", (True, False)) +@pytest.mark.parametrize("two_extras", (True, False)) +def test_new_resolver_dont_backtrack_on_conflicting_constraints_on_extras( + script: PipTestEnvironment, swap_order: bool, two_extras: bool +) -> None: + """ + Verify that conflicting constraints on the same package with different + extras cause the resolver to trivially reject the request rather than + trying any candidates. + + :param swap_order: swap the order the install specifiers appear in + :param two_extras: also add an extra for the second specifier + """ + create_basic_wheel_for_package(script, "dep", "1.0") + create_basic_wheel_for_package( + script, "pkg", "1.0", extras={"ext1": ["dep"], "ext2": ["dep"]} + ) + create_basic_wheel_for_package( + script, "pkg", "2.0", extras={"ext1": ["dep"], "ext2": ["dep"]} + ) + + to_install: tuple[str, str] = ( + "pkg[ext1]>1", "pkg[ext2]==1.0" if two_extras else "pkg==1.0" + ) + + result = script.pip( + "install", + "--no-cache-dir", + "--no-index", + "--find-links", + script.scratch_path, + *(to_install if not swap_order else reversed(to_install)), + expect_error=True, + ) + assert "pkg-2.0" not in result.stdout or "pkg-1.0" not in result.stdout, ( + "Should only try one of 1.0, 2.0 depending on order" + ) + assert "looking at multiple versions" not in result.stdout, ( + "Should not have to look at multiple versions to conclude conflict" + ) + assert "conflict is caused by" in result.stdout, ( + "Resolver should be trivially able to find conflict cause" + ) + + def test_new_resolver_respect_user_requested_if_extra_is_installed( script: PipTestEnvironment, ) -> None: From 3fa373c0789699233726c02c2e72643d66da26e0 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Tue, 25 Jul 2023 15:59:20 +0200 Subject: [PATCH 13/40] added test for comes-from reporting --- tests/functional/test_new_resolver.py | 28 +++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/functional/test_new_resolver.py b/tests/functional/test_new_resolver.py index 88dd635ae26..e597669b3a2 100644 --- a/tests/functional/test_new_resolver.py +++ b/tests/functional/test_new_resolver.py @@ -2429,3 +2429,31 @@ def test_new_resolver_works_when_failing_package_builds_are_disallowed( ) script.assert_installed(pkg2="1.0", pkg1="1.0") + + +@pytest.mark.parametrize("swap_order", (True, False)) +def test_new_resolver_comes_from_with_extra( + script: PipTestEnvironment, swap_order: bool +) -> None: + """ + Verify that reporting where a dependency comes from is accurate when it comes + from a package with an extra. + + :param swap_order: swap the order the install specifiers appear in + """ + create_basic_wheel_for_package(script, "dep", "1.0") + create_basic_wheel_for_package(script, "pkg", "1.0", extras={"ext": ["dep"]}) + + to_install: tuple[str, str] = ("pkg", "pkg[ext]") + + result = script.pip( + "install", + "--no-cache-dir", + "--no-index", + "--find-links", + script.scratch_path, + *(to_install if not swap_order else reversed(to_install)), + ) + assert "(from pkg[ext])" in result.stdout + assert "(from pkg)" not in result.stdout + script.assert_installed(pkg="1.0", dep="1.0") From e5690173515ce0dc82bcbc254d9211ca4124031c Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Tue, 25 Jul 2023 16:34:23 +0200 Subject: [PATCH 14/40] added test case for report bugfix --- tests/functional/test_install_report.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/functional/test_install_report.py b/tests/functional/test_install_report.py index 003b29d3821..1c3ffe80b70 100644 --- a/tests/functional/test_install_report.py +++ b/tests/functional/test_install_report.py @@ -64,14 +64,26 @@ def test_install_report_dep( assert _install_dict(report)["simple"]["requested"] is False +@pytest.mark.parametrize( + "specifiers", + [ + # result should be the same regardless of the method and order in which + # extras are specified + ("Paste[openid]==1.7.5.1",), + ("Paste==1.7.5.1", "Paste[openid]==1.7.5.1"), + ("Paste[openid]==1.7.5.1", "Paste==1.7.5.1"), + ], +) @pytest.mark.network -def test_install_report_index(script: PipTestEnvironment, tmp_path: Path) -> None: +def test_install_report_index( + script: PipTestEnvironment, tmp_path: Path, specifiers: tuple[str, ...] +) -> None: """Test report for sdist obtained from index.""" report_path = tmp_path / "report.json" script.pip( "install", "--dry-run", - "Paste[openid]==1.7.5.1", + *specifiers, "--report", str(report_path), ) From cc6a2bded22001a6a3996f741b674ab1bab835ff Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Tue, 25 Jul 2023 16:38:51 +0200 Subject: [PATCH 15/40] added second report test case --- tests/functional/test_install_report.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/functional/test_install_report.py b/tests/functional/test_install_report.py index 1c3ffe80b70..f9a2e27c033 100644 --- a/tests/functional/test_install_report.py +++ b/tests/functional/test_install_report.py @@ -105,6 +105,26 @@ def test_install_report_index( assert "requires_dist" in paste_report["metadata"] +@pytest.mark.network +def test_install_report_index_multiple_extras( + script: PipTestEnvironment, tmp_path: Path +) -> None: + """Test report for sdist obtained from index, with multiple extras requested.""" + report_path = tmp_path / "report.json" + script.pip( + "install", + "--dry-run", + "Paste[openid]", + "Paste[subprocess]", + "--report", + str(report_path), + ) + report = json.loads(report_path.read_text()) + install_dict = _install_dict(report) + assert "paste" in install_dict + assert install_dict["paste"]["requested_extras"] == ["openid", "subprocess"] + + @pytest.mark.network def test_install_report_direct_archive( script: PipTestEnvironment, tmp_path: Path, shared_data: TestData From 4ae829cb3f40d6a64c86988e2f591c3344123bcd Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Tue, 25 Jul 2023 17:14:50 +0200 Subject: [PATCH 16/40] news entries --- news/11924.bugfix.rst | 1 + news/12095.bugfix.rst | 1 + 2 files changed, 2 insertions(+) create mode 100644 news/11924.bugfix.rst create mode 100644 news/12095.bugfix.rst diff --git a/news/11924.bugfix.rst b/news/11924.bugfix.rst new file mode 100644 index 00000000000..30bc60e6bce --- /dev/null +++ b/news/11924.bugfix.rst @@ -0,0 +1 @@ +Improve extras resolution for multiple constraints on same base package. diff --git a/news/12095.bugfix.rst b/news/12095.bugfix.rst new file mode 100644 index 00000000000..1f5018326ba --- /dev/null +++ b/news/12095.bugfix.rst @@ -0,0 +1 @@ +Consistently report whether a dependency comes from an extra. From dc01a40d413351085410a39dc6f616c5e1e21002 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Tue, 25 Jul 2023 17:19:21 +0200 Subject: [PATCH 17/40] py38 compatibility --- src/pip/_internal/resolution/resolvelib/factory.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 03820edde6a..fdb5c4987ae 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -447,7 +447,7 @@ def find_candidates( def _make_requirements_from_install_req( self, ireq: InstallRequirement, requested_extras: Iterable[str] - ) -> list[Requirement]: + ) -> List[Requirement]: """ Returns requirement objects associated with the given InstallRequirement. In most cases this will be a single object but the following special cases exist: @@ -543,7 +543,7 @@ def make_requirements_from_spec( specifier: str, comes_from: Optional[InstallRequirement], requested_extras: Iterable[str] = (), - ) -> list[Requirement]: + ) -> List[Requirement]: """ Returns requirement objects associated with the given specifier. In most cases this will be a single object but the following special cases exist: From 292387f20b8c6e0d57e9eec940621ef0932499c8 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Tue, 25 Jul 2023 17:25:05 +0200 Subject: [PATCH 18/40] py37 compatibility --- tests/functional/test_install_report.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/functional/test_install_report.py b/tests/functional/test_install_report.py index f9a2e27c033..d7553ec0352 100644 --- a/tests/functional/test_install_report.py +++ b/tests/functional/test_install_report.py @@ -1,7 +1,7 @@ import json import textwrap from pathlib import Path -from typing import Any, Dict +from typing import Any, Dict, Tuple import pytest from packaging.utils import canonicalize_name @@ -76,7 +76,7 @@ def test_install_report_dep( ) @pytest.mark.network def test_install_report_index( - script: PipTestEnvironment, tmp_path: Path, specifiers: tuple[str, ...] + script: PipTestEnvironment, tmp_path: Path, specifiers: Tuple[str, ...] ) -> None: """Test report for sdist obtained from index.""" report_path = tmp_path / "report.json" From 39e1102800af8be86ed385aed7f93f6535262d29 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Tue, 25 Jul 2023 17:33:06 +0200 Subject: [PATCH 19/40] fixed minor type errors --- src/pip/_internal/req/constructors.py | 16 +++++++++++++--- .../_internal/resolution/resolvelib/factory.py | 2 +- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/pip/_internal/req/constructors.py b/src/pip/_internal/req/constructors.py index f97bded9887..3b7243f8256 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -64,7 +64,9 @@ def _set_requirement_extras(req: Requirement, new_extras: Set[str]) -> Requireme given requirement already has extras those are replaced (or dropped if no new extras are given). """ - match: re.Match = re.fullmatch(r"([^;\[<>~=]+)(\[[^\]]*\])?(.*)", str(req)) + match: Optional[re.Match[str]] = re.fullmatch( + r"([^;\[<>~=]+)(\[[^\]]*\])?(.*)", str(req) + ) # ireq.req is a valid requirement so the regex should always match assert match is not None pre: Optional[str] = match.group(1) @@ -534,7 +536,11 @@ def install_req_drop_extras(ireq: InstallRequirement) -> InstallRequirement: (comes_from). """ return InstallRequirement( - req=_set_requirement_extras(ireq.req, set()), + req=( + _set_requirement_extras(ireq.req, set()) + if ireq.req is not None + else None + ), comes_from=ireq, editable=ireq.editable, link=ireq.link, @@ -561,5 +567,9 @@ def install_req_extend_extras( """ result = copy.copy(ireq) result.extras = {*ireq.extras, *extras} - result.req = _set_requirement_extras(ireq.req, result.extras) + result.req = ( + _set_requirement_extras(ireq.req, result.extras) + if ireq.req is not None + else None + ) return result diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index fdb5c4987ae..2812fab57d9 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -465,7 +465,7 @@ def _make_requirements_from_install_req( ) return [] if not ireq.link: - if ireq.extras and ireq.req.specifier: + if ireq.extras and ireq.req is not None and ireq.req.specifier: return [ SpecifierRequirement(ireq, drop_extras=True), SpecifierRequirement(ireq), From e6333bb4d18edc8aec9b38601f81867ad1036807 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 26 Jul 2023 10:32:58 +0200 Subject: [PATCH 20/40] linting --- src/pip/_internal/req/constructors.py | 12 +++------- .../resolution/resolvelib/requirements.py | 2 +- tests/functional/test_new_resolver.py | 24 ++++++++++--------- 3 files changed, 17 insertions(+), 21 deletions(-) diff --git a/src/pip/_internal/req/constructors.py b/src/pip/_internal/req/constructors.py index 3b7243f8256..b5f176e6b38 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -16,7 +16,7 @@ from pip._vendor.packaging.markers import Marker from pip._vendor.packaging.requirements import InvalidRequirement, Requirement -from pip._vendor.packaging.specifiers import Specifier, SpecifierSet +from pip._vendor.packaging.specifiers import Specifier from pip._internal.exceptions import InstallationError from pip._internal.models.index import PyPI, TestPyPI @@ -72,11 +72,7 @@ def _set_requirement_extras(req: Requirement, new_extras: Set[str]) -> Requireme pre: Optional[str] = match.group(1) post: Optional[str] = match.group(3) assert pre is not None and post is not None - extras: str = ( - "[%s]" % ",".join(sorted(new_extras)) - if new_extras - else "" - ) + extras: str = "[%s]" % ",".join(sorted(new_extras)) if new_extras else "" return Requirement(pre + extras + post) @@ -537,9 +533,7 @@ def install_req_drop_extras(ireq: InstallRequirement) -> InstallRequirement: """ return InstallRequirement( req=( - _set_requirement_extras(ireq.req, set()) - if ireq.req is not None - else None + _set_requirement_extras(ireq.req, set()) if ireq.req is not None else None ), comes_from=ireq, editable=ireq.editable, diff --git a/src/pip/_internal/resolution/resolvelib/requirements.py b/src/pip/_internal/resolution/resolvelib/requirements.py index e23b948ffc2..ad9892a17a2 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -1,8 +1,8 @@ from pip._vendor.packaging.specifiers import SpecifierSet from pip._vendor.packaging.utils import NormalizedName, canonicalize_name -from pip._internal.req.req_install import InstallRequirement from pip._internal.req.constructors import install_req_drop_extras +from pip._internal.req.req_install import InstallRequirement from .base import Candidate, CandidateLookup, Requirement, format_name diff --git a/tests/functional/test_new_resolver.py b/tests/functional/test_new_resolver.py index e597669b3a2..77dede2fc5a 100644 --- a/tests/functional/test_new_resolver.py +++ b/tests/functional/test_new_resolver.py @@ -2294,7 +2294,8 @@ def test_new_resolver_dont_backtrack_on_extra_if_base_constrained_in_requirement ) to_install: tuple[str, str] = ( - "pkg[ext1]", "pkg[ext2]==1.0" if two_extras else "pkg==1.0" + "pkg[ext1]", + "pkg[ext2]==1.0" if two_extras else "pkg==1.0", ) result = script.pip( @@ -2331,7 +2332,8 @@ def test_new_resolver_dont_backtrack_on_conflicting_constraints_on_extras( ) to_install: tuple[str, str] = ( - "pkg[ext1]>1", "pkg[ext2]==1.0" if two_extras else "pkg==1.0" + "pkg[ext1]>1", + "pkg[ext2]==1.0" if two_extras else "pkg==1.0", ) result = script.pip( @@ -2343,15 +2345,15 @@ def test_new_resolver_dont_backtrack_on_conflicting_constraints_on_extras( *(to_install if not swap_order else reversed(to_install)), expect_error=True, ) - assert "pkg-2.0" not in result.stdout or "pkg-1.0" not in result.stdout, ( - "Should only try one of 1.0, 2.0 depending on order" - ) - assert "looking at multiple versions" not in result.stdout, ( - "Should not have to look at multiple versions to conclude conflict" - ) - assert "conflict is caused by" in result.stdout, ( - "Resolver should be trivially able to find conflict cause" - ) + assert ( + "pkg-2.0" not in result.stdout or "pkg-1.0" not in result.stdout + ), "Should only try one of 1.0, 2.0 depending on order" + assert ( + "looking at multiple versions" not in result.stdout + ), "Should not have to look at multiple versions to conclude conflict" + assert ( + "conflict is caused by" in result.stdout + ), "Resolver should be trivially able to find conflict cause" def test_new_resolver_respect_user_requested_if_extra_is_installed( From 12073891776472dd3517106da1c26483d64e1557 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 26 Jul 2023 10:33:43 +0200 Subject: [PATCH 21/40] made primary news fragment of type feature --- news/{11924.bugfix.rst => 11924.feature.rst} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename news/{11924.bugfix.rst => 11924.feature.rst} (100%) diff --git a/news/11924.bugfix.rst b/news/11924.feature.rst similarity index 100% rename from news/11924.bugfix.rst rename to news/11924.feature.rst From 6663b89a4d465f675b88bce52d4ad7cef9164c6a Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 26 Jul 2023 10:37:00 +0200 Subject: [PATCH 22/40] added final bugfix news entry --- news/11924.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/11924.bugfix.rst diff --git a/news/11924.bugfix.rst b/news/11924.bugfix.rst new file mode 100644 index 00000000000..7a9ee3151a4 --- /dev/null +++ b/news/11924.bugfix.rst @@ -0,0 +1 @@ +Include all requested extras in the install report (``--report``). From 314d7c12549a60c8460b1e2a8dac82fe0cca848a Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 26 Jul 2023 10:52:42 +0200 Subject: [PATCH 23/40] simplified regex --- src/pip/_internal/req/constructors.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/req/constructors.py b/src/pip/_internal/req/constructors.py index b5f176e6b38..c03ae718e90 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -65,7 +65,10 @@ def _set_requirement_extras(req: Requirement, new_extras: Set[str]) -> Requireme are given). """ match: Optional[re.Match[str]] = re.fullmatch( - r"([^;\[<>~=]+)(\[[^\]]*\])?(.*)", str(req) + # see https://peps.python.org/pep-0508/#complete-grammar + r"([\w\t .-]+)(\[[^\]]*\])?(.*)", + str(req), + flags=re.ASCII, ) # ireq.req is a valid requirement so the regex should always match assert match is not None From cc909e87e5ccada46b4eb8a2a90c329614dc9b01 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 26 Jul 2023 11:06:24 +0200 Subject: [PATCH 24/40] reverted unnecessary changes --- src/pip/_internal/resolution/resolvelib/requirements.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/requirements.py b/src/pip/_internal/resolution/resolvelib/requirements.py index ad9892a17a2..becbd6c4bcc 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -56,7 +56,7 @@ def __init__( self._extras = frozenset(self._ireq.extras) def __str__(self) -> str: - return str(self._ireq) + return str(self._ireq.req) def __repr__(self) -> str: return "{class_name}({requirement!r})".format( @@ -71,10 +71,7 @@ def project_name(self) -> NormalizedName: @property def name(self) -> str: - return format_name( - self.project_name, - self._extras, - ) + return format_name(self.project_name, self._extras) def format_for_error(self) -> str: # Convert comma-separated specifiers into "A, B, ..., F and G" From 6ed231a52be89295e3cecdb4f41eaf63f3152941 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 26 Jul 2023 11:34:37 +0200 Subject: [PATCH 25/40] added unit tests for install req manipulation --- tests/unit/test_req.py | 87 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_req.py b/tests/unit/test_req.py index 545828f8eea..b4819d8320a 100644 --- a/tests/unit/test_req.py +++ b/tests/unit/test_req.py @@ -6,7 +6,7 @@ import tempfile from functools import partial from pathlib import Path -from typing import Iterator, Optional, Tuple, cast +from typing import Iterator, Optional, Set, Tuple, cast from unittest import mock import pytest @@ -33,6 +33,8 @@ from pip._internal.req.constructors import ( _get_url_from_path, _looks_like_path, + install_req_drop_extras, + install_req_extend_extras, install_req_from_editable, install_req_from_line, install_req_from_parsed_requirement, @@ -763,6 +765,89 @@ def test_requirement_file(self) -> None: assert "appears to be a requirements file." in err_msg assert "If that is the case, use the '-r' flag to install" in err_msg + @pytest.mark.parametrize( + "inp, out", + [ + ("pkg", "pkg"), + ("pkg==1.0", "pkg==1.0"), + ("pkg ; python_version<='3.6'", "pkg"), + ("pkg[ext]", "pkg"), + ("pkg [ ext1, ext2 ]", "pkg"), + ("pkg [ ext1, ext2 ] @ https://example.com/", "pkg@ https://example.com/"), + ("pkg [ext] == 1.0; python_version<='3.6'", "pkg==1.0"), + ("pkg-all.allowed_chars0 ~= 2.0", "pkg-all.allowed_chars0~=2.0"), + ("pkg-all.allowed_chars0 [ext] ~= 2.0", "pkg-all.allowed_chars0~=2.0"), + ], + ) + def test_install_req_drop_extras(self, inp: str, out: str) -> None: + """ + Test behavior of install_req_drop_extras + """ + req = install_req_from_line(inp) + without_extras = install_req_drop_extras(req) + assert not without_extras.extras + assert str(without_extras.req) == out + # should always be a copy + assert req is not without_extras + assert req.req is not without_extras.req + # comes_from should point to original + assert without_extras.comes_from is req + # all else should be the same + assert without_extras.link == req.link + assert without_extras.markers == req.markers + assert without_extras.use_pep517 == req.use_pep517 + assert without_extras.isolated == req.isolated + assert without_extras.global_options == req.global_options + assert without_extras.hash_options == req.hash_options + assert without_extras.constraint == req.constraint + assert without_extras.config_settings == req.config_settings + assert without_extras.user_supplied == req.user_supplied + assert without_extras.permit_editable_wheels == req.permit_editable_wheels + + @pytest.mark.parametrize( + "inp, extras, out", + [ + ("pkg", {}, "pkg"), + ("pkg==1.0", {}, "pkg==1.0"), + ("pkg[ext]", {}, "pkg[ext]"), + ("pkg", {"ext"}, "pkg[ext]"), + ("pkg==1.0", {"ext"}, "pkg[ext]==1.0"), + ("pkg==1.0", {"ext1", "ext2"}, "pkg[ext1,ext2]==1.0"), + ("pkg; python_version<='3.6'", {"ext"}, "pkg[ext]"), + ("pkg[ext1,ext2]==1.0", {"ext2", "ext3"}, "pkg[ext1,ext2,ext3]==1.0"), + ( + "pkg-all.allowed_chars0 [ ext1 ] @ https://example.com/", + {"ext2"}, + "pkg-all.allowed_chars0[ext1,ext2]@ https://example.com/", + ), + ], + ) + def test_install_req_extend_extras( + self, inp: str, extras: Set[str], out: str + ) -> None: + """ + Test behavior of install_req_extend_extras + """ + req = install_req_from_line(inp) + extended = install_req_extend_extras(req, extras) + assert str(extended.req) == out + assert extended.req is not None + assert set(extended.extras) == set(extended.req.extras) + # should always be a copy + assert req is not extended + assert req.req is not extended.req + # all else should be the same + assert extended.link == req.link + assert extended.markers == req.markers + assert extended.use_pep517 == req.use_pep517 + assert extended.isolated == req.isolated + assert extended.global_options == req.global_options + assert extended.hash_options == req.hash_options + assert extended.constraint == req.constraint + assert extended.config_settings == req.config_settings + assert extended.user_supplied == req.user_supplied + assert extended.permit_editable_wheels == req.permit_editable_wheels + @mock.patch("pip._internal.req.req_install.os.path.abspath") @mock.patch("pip._internal.req.req_install.os.path.exists") From 55e9762873d824608ae52570ef3dcbb65e75f833 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 26 Jul 2023 14:02:50 +0200 Subject: [PATCH 26/40] windows compatibility --- tests/lib/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/__init__.py b/tests/lib/__init__.py index 7c06feaf38c..d424a5e8dae 100644 --- a/tests/lib/__init__.py +++ b/tests/lib/__init__.py @@ -645,7 +645,7 @@ def run( cwd = cwd or self.cwd if sys.platform == "win32": # Partial fix for ScriptTest.run using `shell=True` on Windows. - args = tuple(str(a).replace("^", "^^").replace("&", "^&") for a in args) + args = tuple(str(a).replace("^", "^^").replace("&", "^&").replace(">", "^>") for a in args) if allow_error: kw["expect_error"] = True From 504485c27644f7a8b44cec179bfc0fac181b0d9e Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 26 Jul 2023 14:03:26 +0200 Subject: [PATCH 27/40] lint --- tests/lib/__init__.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/lib/__init__.py b/tests/lib/__init__.py index d424a5e8dae..b6996f31d91 100644 --- a/tests/lib/__init__.py +++ b/tests/lib/__init__.py @@ -645,7 +645,10 @@ def run( cwd = cwd or self.cwd if sys.platform == "win32": # Partial fix for ScriptTest.run using `shell=True` on Windows. - args = tuple(str(a).replace("^", "^^").replace("&", "^&").replace(">", "^>") for a in args) + args = tuple( + str(a).replace("^", "^^").replace("&", "^&").replace(">", "^>") + for a in args + ) if allow_error: kw["expect_error"] = True From f4a7c0c569caf665c11379cd9629e5a5163867f5 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 26 Jul 2023 14:12:38 +0200 Subject: [PATCH 28/40] cleaned up windows fix --- tests/lib/__init__.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/lib/__init__.py b/tests/lib/__init__.py index b6996f31d91..a7f2ade1a1d 100644 --- a/tests/lib/__init__.py +++ b/tests/lib/__init__.py @@ -645,10 +645,7 @@ def run( cwd = cwd or self.cwd if sys.platform == "win32": # Partial fix for ScriptTest.run using `shell=True` on Windows. - args = tuple( - str(a).replace("^", "^^").replace("&", "^&").replace(">", "^>") - for a in args - ) + args = tuple(re.sub("([&|()<>^])", r"^\1", str(a)) for a in args) if allow_error: kw["expect_error"] = True From 32e95be2130333e4f543778302fdc4d0c47043ad Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 26 Jul 2023 14:31:12 +0200 Subject: [PATCH 29/40] exclude brackets --- tests/lib/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/__init__.py b/tests/lib/__init__.py index a7f2ade1a1d..018152930c2 100644 --- a/tests/lib/__init__.py +++ b/tests/lib/__init__.py @@ -645,7 +645,7 @@ def run( cwd = cwd or self.cwd if sys.platform == "win32": # Partial fix for ScriptTest.run using `shell=True` on Windows. - args = tuple(re.sub("([&|()<>^])", r"^\1", str(a)) for a in args) + args = tuple(re.sub("([&|<>^])", r"^\1", str(a)) for a in args) if allow_error: kw["expect_error"] = True From 21bfe401a96ddecb2a827b9b3cd5ff1b833b151f Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 6 Sep 2023 11:50:10 +0200 Subject: [PATCH 30/40] use more stable sort key --- src/pip/_internal/resolution/resolvelib/resolver.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/resolver.py b/src/pip/_internal/resolution/resolvelib/resolver.py index 4c53dfb25c1..2e4941da814 100644 --- a/src/pip/_internal/resolution/resolvelib/resolver.py +++ b/src/pip/_internal/resolution/resolvelib/resolver.py @@ -104,8 +104,9 @@ def resolve( raise error from e req_set = RequirementSet(check_supported_wheels=check_supported_wheels) - # sort to ensure base candidates come before candidates with extras - for candidate in sorted(result.mapping.values(), key=lambda c: c.name): + # process candidates with extras last to ensure their base equivalent is already in the req_set if appropriate + # Python's sort is stable so using a binary key function keeps relative order within both subsets + for candidate in sorted(result.mapping.values(), key=lambda c: c.name != c.project_name): ireq = candidate.get_install_requirement() if ireq is None: if candidate.name != candidate.project_name: From 0de374e4df5707955fc6bf9602ee86ea21c8f258 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 6 Sep 2023 13:52:41 +0200 Subject: [PATCH 31/40] review comment: return iterator instead of list --- .../resolution/resolvelib/factory.py | 64 +++++++++---------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 0eb7a1c662e..81f482c8626 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -1,4 +1,5 @@ import contextlib +import itertools import functools import logging from typing import ( @@ -447,7 +448,7 @@ def find_candidates( def _make_requirements_from_install_req( self, ireq: InstallRequirement, requested_extras: Iterable[str] - ) -> List[Requirement]: + ) -> Iterator[Requirement]: """ Returns requirement objects associated with the given InstallRequirement. In most cases this will be a single object but the following special cases exist: @@ -463,34 +464,32 @@ def _make_requirements_from_install_req( ireq.name, ireq.markers, ) - return [] - if not ireq.link: + yield from () + elif not ireq.link: if ireq.extras and ireq.req is not None and ireq.req.specifier: - return [ - SpecifierRequirement(ireq, drop_extras=True), - SpecifierRequirement(ireq), - ] + yield SpecifierRequirement(ireq, drop_extras=True), + yield SpecifierRequirement(ireq) + else: + self._fail_if_link_is_unsupported_wheel(ireq.link) + cand = self._make_candidate_from_link( + ireq.link, + extras=frozenset(ireq.extras), + template=ireq, + 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] + yield UnsatisfiableRequirement(canonicalize_name(ireq.name)) else: - return [SpecifierRequirement(ireq)] - self._fail_if_link_is_unsupported_wheel(ireq.link) - cand = self._make_candidate_from_link( - ireq.link, - extras=frozenset(ireq.extras), - template=ireq, - 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)] + yield self.make_requirement_from_candidate(cand) def collect_root_requirements( self, root_ireqs: List[InstallRequirement] @@ -511,13 +510,14 @@ def collect_root_requirements( else: collected.constraints[name] = Constraint.from_ireq(ireq) else: - reqs = self._make_requirements_from_install_req( - ireq, - requested_extras=(), + reqs = list( + self._make_requirements_from_install_req( + ireq, + requested_extras=(), + ) ) if not reqs: continue - template = reqs[0] if ireq.user_supplied and template.name not in collected.user_requested: collected.user_requested[template.name] = i @@ -543,7 +543,7 @@ def make_requirements_from_spec( specifier: str, comes_from: Optional[InstallRequirement], requested_extras: Iterable[str] = (), - ) -> List[Requirement]: + ) -> Iterator[Requirement]: """ Returns requirement objects associated with the given specifier. In most cases this will be a single object but the following special cases exist: From 5a0167902261b97df768cbcf4757b665cd39229e Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 6 Sep 2023 13:54:28 +0200 Subject: [PATCH 32/40] Update src/pip/_internal/req/constructors.py Co-authored-by: Tzu-ping Chung --- src/pip/_internal/req/constructors.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/req/constructors.py b/src/pip/_internal/req/constructors.py index c03ae718e90..a40191954f8 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -76,7 +76,7 @@ def _set_requirement_extras(req: Requirement, new_extras: Set[str]) -> Requireme post: Optional[str] = match.group(3) assert pre is not None and post is not None extras: str = "[%s]" % ",".join(sorted(new_extras)) if new_extras else "" - return Requirement(pre + extras + post) + return Requirement(f"{pre}{extras}{post}") def parse_editable(editable_req: str) -> Tuple[Optional[str], str, Set[str]]: From 4e73e3e96e99f79d8458517278f67e33796a7fd0 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 6 Sep 2023 14:39:51 +0200 Subject: [PATCH 33/40] review comment: subclass instead of constructor flag --- .../resolution/resolvelib/factory.py | 4 ++-- .../resolution/resolvelib/requirements.py | 24 ++++++++++--------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 81f482c8626..af13a33215b 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -1,5 +1,4 @@ import contextlib -import itertools import functools import logging from typing import ( @@ -63,6 +62,7 @@ ExplicitRequirement, RequiresPythonRequirement, SpecifierRequirement, + SpecifierWithoutExtrasRequirement, UnsatisfiableRequirement, ) @@ -467,7 +467,7 @@ def _make_requirements_from_install_req( yield from () elif not ireq.link: if ireq.extras and ireq.req is not None and ireq.req.specifier: - yield SpecifierRequirement(ireq, drop_extras=True), + yield SpecifierWithoutExtrasRequirement(ireq), yield SpecifierRequirement(ireq) else: self._fail_if_link_is_unsupported_wheel(ireq.link) diff --git a/src/pip/_internal/resolution/resolvelib/requirements.py b/src/pip/_internal/resolution/resolvelib/requirements.py index becbd6c4bcc..9c2512823a3 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -41,18 +41,9 @@ def is_satisfied_by(self, candidate: Candidate) -> bool: class SpecifierRequirement(Requirement): - def __init__( - self, - ireq: InstallRequirement, - *, - drop_extras: bool = False, - ) -> None: - """ - :param drop_extras: Ignore any extras that are part of the install requirement, - making this a requirement on the base only. - """ + def __init__(self, ireq: InstallRequirement) -> None: assert ireq.link is None, "This is a link, not a specifier" - self._ireq = ireq if not drop_extras else install_req_drop_extras(ireq) + self._ireq = ireq self._extras = frozenset(self._ireq.extras) def __str__(self) -> str: @@ -102,6 +93,17 @@ def is_satisfied_by(self, candidate: Candidate) -> bool: return spec.contains(candidate.version, prereleases=True) +class SpecifierWithoutExtrasRequirement(SpecifierRequirement): + """ + Requirement backed by an install requirement on a base package. Trims extras from its install requirement if there are any. + """ + + def __init__(self, ireq: InstallRequirement) -> None: + assert ireq.link is None, "This is a link, not a specifier" + self._ireq = install_req_drop_extras(ireq) + self._extras = frozenset(self._ireq.extras) + + class RequiresPythonRequirement(Requirement): """A requirement representing Requires-Python metadata.""" From 50cd318cefcdd2a451b0a70a3bf3f31a3ecc6b99 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 6 Sep 2023 15:06:19 +0200 Subject: [PATCH 34/40] review comment: renamed and moved up ExtrasCandidate._ireq --- src/pip/_internal/resolution/resolvelib/candidates.py | 9 +++++---- src/pip/_internal/resolution/resolvelib/factory.py | 9 +++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/pip/_internal/resolution/resolvelib/candidates.py b/src/pip/_internal/resolution/resolvelib/candidates.py index 23883484139..d658be37284 100644 --- a/src/pip/_internal/resolution/resolvelib/candidates.py +++ b/src/pip/_internal/resolution/resolvelib/candidates.py @@ -427,10 +427,11 @@ def __init__( self, base: BaseCandidate, extras: FrozenSet[str], - ireq: Optional[InstallRequirement] = None, + *, + comes_from: Optional[InstallRequirement] = None, ) -> None: """ - :param ireq: the InstallRequirement that led to this candidate, if it + :param ireq: the InstallRequirement that led to this candidate if it differs from the base's InstallRequirement. This will often be the case in the sense that this candidate's requirement has the extras while the base's does not. Unlike the InstallRequirement backed @@ -439,7 +440,7 @@ def __init__( """ self.base = base self.extras = extras - self._ireq = ireq + self._comes_from = comes_from if comes_from is not None else self.base._ireq def __str__(self) -> str: name, rest = str(self.base).split(" ", 1) @@ -514,7 +515,7 @@ def iter_dependencies(self, with_requires: bool) -> Iterable[Optional[Requiremen for r in self.base.dist.iter_dependencies(valid_extras): yield from factory.make_requirements_from_spec( str(r), - self._ireq if self._ireq is not None else self.base._ireq, + self._comes_from, valid_extras, ) diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index af13a33215b..8c5a779911f 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -142,13 +142,14 @@ def _make_extras_candidate( self, base: BaseCandidate, extras: FrozenSet[str], - ireq: Optional[InstallRequirement] = None, + *, + comes_from: Optional[InstallRequirement] = None, ) -> ExtrasCandidate: cache_key = (id(base), extras) try: candidate = self._extras_candidate_cache[cache_key] except KeyError: - candidate = ExtrasCandidate(base, extras, ireq=ireq) + candidate = ExtrasCandidate(base, extras, comes_from=comes_from) self._extras_candidate_cache[cache_key] = candidate return candidate @@ -165,7 +166,7 @@ def _make_candidate_from_dist( self._installed_candidate_cache[dist.canonical_name] = base if not extras: return base - return self._make_extras_candidate(base, extras, ireq=template) + return self._make_extras_candidate(base, extras, comes_from=template) def _make_candidate_from_link( self, @@ -227,7 +228,7 @@ def _make_candidate_from_link( if not extras: return base - return self._make_extras_candidate(base, extras, ireq=template) + return self._make_extras_candidate(base, extras, comes_from=template) def _iter_found_candidates( self, From f5602fa0b8a26733cc144b5e1449730fdf620c31 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 6 Sep 2023 15:12:17 +0200 Subject: [PATCH 35/40] added message to invariant assertions --- src/pip/_internal/req/constructors.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/req/constructors.py b/src/pip/_internal/req/constructors.py index a40191954f8..f0f043b0021 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -71,10 +71,10 @@ def _set_requirement_extras(req: Requirement, new_extras: Set[str]) -> Requireme flags=re.ASCII, ) # ireq.req is a valid requirement so the regex should always match - assert match is not None + assert match is not None, f"regex match on requirement {req} failed, this should never happen" pre: Optional[str] = match.group(1) post: Optional[str] = match.group(3) - assert pre is not None and post is not None + assert pre is not None and post is not None, f"regex group selection for requirement {req} failed, this should never happen" extras: str = "[%s]" % ",".join(sorted(new_extras)) if new_extras else "" return Requirement(f"{pre}{extras}{post}") From 449522a8286d28d2c88776dca3cc67b3064982d3 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 6 Sep 2023 15:16:22 +0200 Subject: [PATCH 36/40] minor fixes and linting --- src/pip/_internal/req/constructors.py | 8 ++++++-- src/pip/_internal/resolution/resolvelib/factory.py | 2 +- .../_internal/resolution/resolvelib/requirements.py | 3 ++- src/pip/_internal/resolution/resolvelib/resolver.py | 10 +++++++--- tests/unit/resolution_resolvelib/test_requirement.py | 8 ++++---- 5 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/pip/_internal/req/constructors.py b/src/pip/_internal/req/constructors.py index f0f043b0021..b52c9a456bb 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -71,10 +71,14 @@ def _set_requirement_extras(req: Requirement, new_extras: Set[str]) -> Requireme flags=re.ASCII, ) # ireq.req is a valid requirement so the regex should always match - assert match is not None, f"regex match on requirement {req} failed, this should never happen" + assert ( + match is not None + ), f"regex match on requirement {req} failed, this should never happen" pre: Optional[str] = match.group(1) post: Optional[str] = match.group(3) - assert pre is not None and post is not None, f"regex group selection for requirement {req} failed, this should never happen" + assert ( + pre is not None and post is not None + ), f"regex group selection for requirement {req} failed, this should never happen" extras: str = "[%s]" % ",".join(sorted(new_extras)) if new_extras else "" return Requirement(f"{pre}{extras}{post}") diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 8c5a779911f..905449f68db 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -468,7 +468,7 @@ def _make_requirements_from_install_req( yield from () elif not ireq.link: if ireq.extras and ireq.req is not None and ireq.req.specifier: - yield SpecifierWithoutExtrasRequirement(ireq), + yield SpecifierWithoutExtrasRequirement(ireq) yield SpecifierRequirement(ireq) else: self._fail_if_link_is_unsupported_wheel(ireq.link) diff --git a/src/pip/_internal/resolution/resolvelib/requirements.py b/src/pip/_internal/resolution/resolvelib/requirements.py index 9c2512823a3..02cdf65f144 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -95,7 +95,8 @@ def is_satisfied_by(self, candidate: Candidate) -> bool: class SpecifierWithoutExtrasRequirement(SpecifierRequirement): """ - Requirement backed by an install requirement on a base package. Trims extras from its install requirement if there are any. + Requirement backed by an install requirement on a base package. + Trims extras from its install requirement if there are any. """ def __init__(self, ireq: InstallRequirement) -> None: diff --git a/src/pip/_internal/resolution/resolvelib/resolver.py b/src/pip/_internal/resolution/resolvelib/resolver.py index 2e4941da814..c12beef0b2a 100644 --- a/src/pip/_internal/resolution/resolvelib/resolver.py +++ b/src/pip/_internal/resolution/resolvelib/resolver.py @@ -104,9 +104,13 @@ def resolve( raise error from e req_set = RequirementSet(check_supported_wheels=check_supported_wheels) - # process candidates with extras last to ensure their base equivalent is already in the req_set if appropriate - # Python's sort is stable so using a binary key function keeps relative order within both subsets - for candidate in sorted(result.mapping.values(), key=lambda c: c.name != c.project_name): + # process candidates with extras last to ensure their base equivalent is + # already in the req_set if appropriate. + # Python's sort is stable so using a binary key function keeps relative order + # within both subsets. + for candidate in sorted( + result.mapping.values(), key=lambda c: c.name != c.project_name + ): ireq = candidate.get_install_requirement() if ireq is None: if candidate.name != candidate.project_name: diff --git a/tests/unit/resolution_resolvelib/test_requirement.py b/tests/unit/resolution_resolvelib/test_requirement.py index ce48ab16c49..642136a54fd 100644 --- a/tests/unit/resolution_resolvelib/test_requirement.py +++ b/tests/unit/resolution_resolvelib/test_requirement.py @@ -61,7 +61,7 @@ def test_new_resolver_requirement_has_name( ) -> None: """All requirements should have a name""" for spec, name, _ in test_cases: - reqs = factory.make_requirements_from_spec(spec, comes_from=None) + reqs = list(factory.make_requirements_from_spec(spec, comes_from=None)) assert len(reqs) == 1 assert reqs[0].name == name @@ -71,7 +71,7 @@ def test_new_resolver_correct_number_of_matches( ) -> None: """Requirements should return the correct number of candidates""" for spec, _, match_count in test_cases: - reqs = factory.make_requirements_from_spec(spec, comes_from=None) + reqs = list(factory.make_requirements_from_spec(spec, comes_from=None)) assert len(reqs) == 1 req = reqs[0] matches = factory.find_candidates( @@ -89,7 +89,7 @@ def test_new_resolver_candidates_match_requirement( ) -> None: """Candidates returned from find_candidates should satisfy the requirement""" for spec, _, _ in test_cases: - reqs = factory.make_requirements_from_spec(spec, comes_from=None) + reqs = list(factory.make_requirements_from_spec(spec, comes_from=None)) assert len(reqs) == 1 req = reqs[0] candidates = factory.find_candidates( @@ -106,7 +106,7 @@ def test_new_resolver_candidates_match_requirement( def test_new_resolver_full_resolve(factory: Factory, provider: PipProvider) -> None: """A very basic full resolve""" - reqs = factory.make_requirements_from_spec("simplewheel", comes_from=None) + reqs = list(factory.make_requirements_from_spec("simplewheel", comes_from=None)) assert len(reqs) == 1 r: Resolver[Requirement, Candidate, str] = Resolver(provider, BaseReporter()) result = r.resolve([reqs[0]]) From 952ab6d837fbece1a221a1d0409eb27f2bb8c544 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Thu, 7 Sep 2023 10:31:49 +0200 Subject: [PATCH 37/40] Update src/pip/_internal/resolution/resolvelib/factory.py Co-authored-by: Tzu-ping Chung --- src/pip/_internal/resolution/resolvelib/factory.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 905449f68db..2b51aab67df 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -465,7 +465,6 @@ def _make_requirements_from_install_req( ireq.name, ireq.markers, ) - yield from () elif not ireq.link: if ireq.extras and ireq.req is not None and ireq.req.specifier: yield SpecifierWithoutExtrasRequirement(ireq) From fbda0a2ba7e6676f286e515c11b77ded8de996b0 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Fri, 8 Sep 2023 16:32:42 +0200 Subject: [PATCH 38/40] Update tests/unit/resolution_resolvelib/test_requirement.py Co-authored-by: Pradyun Gedam --- tests/unit/resolution_resolvelib/test_requirement.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/resolution_resolvelib/test_requirement.py b/tests/unit/resolution_resolvelib/test_requirement.py index 642136a54fd..b8cd13cb566 100644 --- a/tests/unit/resolution_resolvelib/test_requirement.py +++ b/tests/unit/resolution_resolvelib/test_requirement.py @@ -109,5 +109,5 @@ def test_new_resolver_full_resolve(factory: Factory, provider: PipProvider) -> N reqs = list(factory.make_requirements_from_spec("simplewheel", comes_from=None)) assert len(reqs) == 1 r: Resolver[Requirement, Candidate, str] = Resolver(provider, BaseReporter()) - result = r.resolve([reqs[0]]) + result = r.resolve(reqs) assert set(result.mapping.keys()) == {"simplewheel"} From ce949466c96086a36aefb8ed1106113fca731fa6 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 13 Sep 2023 15:14:07 +0200 Subject: [PATCH 39/40] fixed argument name in docstring --- src/pip/_internal/resolution/resolvelib/candidates.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/resolution/resolvelib/candidates.py b/src/pip/_internal/resolution/resolvelib/candidates.py index d658be37284..bf89a515da4 100644 --- a/src/pip/_internal/resolution/resolvelib/candidates.py +++ b/src/pip/_internal/resolution/resolvelib/candidates.py @@ -431,7 +431,7 @@ def __init__( comes_from: Optional[InstallRequirement] = None, ) -> None: """ - :param ireq: the InstallRequirement that led to this candidate if it + :param comes_from: the InstallRequirement that led to this candidate if it differs from the base's InstallRequirement. This will often be the case in the sense that this candidate's requirement has the extras while the base's does not. Unlike the InstallRequirement backed From 0f543e3c7e05d40e1ecf684cade068fed1c200f9 Mon Sep 17 00:00:00 2001 From: Sander Van Balen Date: Wed, 13 Sep 2023 16:48:16 +0200 Subject: [PATCH 40/40] made assertions more robust --- tests/functional/test_new_resolver.py | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/tests/functional/test_new_resolver.py b/tests/functional/test_new_resolver.py index 77dede2fc5a..b5945edf89b 100644 --- a/tests/functional/test_new_resolver.py +++ b/tests/functional/test_new_resolver.py @@ -6,6 +6,7 @@ import pytest +from tests.conftest import ScriptFactory from tests.lib import ( PipTestEnvironment, create_basic_sdist_for_package, @@ -13,6 +14,7 @@ create_test_package_with_setup, ) from tests.lib.direct_url import get_created_direct_url +from tests.lib.venv import VirtualEnvironment from tests.lib.wheel import make_wheel if TYPE_CHECKING: @@ -2313,7 +2315,11 @@ def test_new_resolver_dont_backtrack_on_extra_if_base_constrained_in_requirement @pytest.mark.parametrize("swap_order", (True, False)) @pytest.mark.parametrize("two_extras", (True, False)) def test_new_resolver_dont_backtrack_on_conflicting_constraints_on_extras( - script: PipTestEnvironment, swap_order: bool, two_extras: bool + tmpdir: pathlib.Path, + virtualenv: VirtualEnvironment, + script_factory: ScriptFactory, + swap_order: bool, + two_extras: bool, ) -> None: """ Verify that conflicting constraints on the same package with different @@ -2323,6 +2329,11 @@ def test_new_resolver_dont_backtrack_on_conflicting_constraints_on_extras( :param swap_order: swap the order the install specifiers appear in :param two_extras: also add an extra for the second specifier """ + script: PipTestEnvironment = script_factory( + tmpdir.joinpath("workspace"), + virtualenv, + {**os.environ, "PIP_RESOLVER_DEBUG": "1"}, + ) create_basic_wheel_for_package(script, "dep", "1.0") create_basic_wheel_for_package( script, "pkg", "1.0", extras={"ext1": ["dep"], "ext2": ["dep"]} @@ -2348,9 +2359,13 @@ def test_new_resolver_dont_backtrack_on_conflicting_constraints_on_extras( assert ( "pkg-2.0" not in result.stdout or "pkg-1.0" not in result.stdout ), "Should only try one of 1.0, 2.0 depending on order" + assert "Reporter.starting()" in result.stdout, ( + "This should never fail unless the debug reporting format has changed," + " in which case the other assertions in this test need to be reviewed." + ) assert ( - "looking at multiple versions" not in result.stdout - ), "Should not have to look at multiple versions to conclude conflict" + "Reporter.rejecting_candidate" not in result.stdout + ), "Should be able to conclude conflict before even selecting a candidate" assert ( "conflict is caused by" in result.stdout ), "Resolver should be trivially able to find conflict cause"