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 0331297b8..c117a30c8 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -385,8 +385,8 @@ class Factory: 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 @@ class Factory: 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 c5ca2d85d..f04a4cbbd 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -15,7 +15,7 @@ from typing import Dict, List, Optional, Set, Tuple, Union 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 de04e1d73..5bac3d6df 100644 --- a/src/pip/_internal/resolution/resolvelib/candidates.py +++ b/src/pip/_internal/resolution/resolvelib/candidates.py @@ -237,10 +237,11 @@ class _InstallRequirementBackedCandidate(Candidate): 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 @@ class AlreadyInstalledCandidate(Candidate): 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 @@ class ExtrasCandidate(Candidate): ) 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 c117a30c8..4c088209b 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -441,18 +441,35 @@ class Factory: 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 @@ class Factory: # 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 @@ class Factory: 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 @@ class Factory: ) -> 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 315fb9c89..121e48d07 100644 --- a/src/pip/_internal/resolution/resolvelib/provider.py +++ b/src/pip/_internal/resolution/resolvelib/provider.py @@ -184,6 +184,8 @@ class PipProvider(_ProviderBase): # 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 06addc0dd..fe9ae6ba6 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.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_without from .base import Candidate, CandidateLookup, Requirement, format_name @@ -39,14 +40,27 @@ class ExplicitRequirement(Requirement): 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 @@ class SpecifierRequirement(Requirement): 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 6864e70ea..ce48ab16c 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 f04a4cbbd..908876c4c 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 5bac3d6df..238834841 100644 --- a/src/pip/_internal/resolution/resolvelib/candidates.py +++ b/src/pip/_internal/resolution/resolvelib/candidates.py @@ -237,7 +237,6 @@ class _InstallRequirementBackedCandidate(Candidate): 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 @@ class ExtrasCandidate(Candidate): 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 @@ class ExtrasCandidate(Candidate): 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 4c088209b..45b813387 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -138,13 +138,16 @@ class Factory: 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 @@ class Factory: 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 @@ class Factory: 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 @@ class Factory: # 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 @@ class Factory: 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 @@ class Factory: 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 @@ class Factory: 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 fe9ae6ba6..180158128 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -40,7 +40,6 @@ class ExplicitRequirement(Requirement): return candidate == self.candidate -# TODO: add some comments class SpecifierRequirement(Requirement): # TODO: document additional options def __init__( @@ -52,15 +51,17 @@ class SpecifierRequirement(Requirement): ) -> 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 @@ class SpecifierRequirement(Requirement): 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 908876c4c..9bf1c9844 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -8,10 +8,11 @@ These are meant to be used elsewhere within pip to create instances of 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 47bbfecce..c5de0e822 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._vendor.resolvelib.structs import DirectedGraph 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 @@ from pip._internal.resolution.resolvelib.reporter import ( PipDebuggingReporter, PipReporter, ) +from pip._internal.utils.packaging import get_requirement from .base import Candidate, Requirement from .factory import Factory @@ -101,9 +104,19 @@ class Resolver(BaseResolver): 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 8559d9368..f5ac31a8e 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 9bf1c9844..8b1438afe 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 45b813387..0c2c6ab79 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -468,18 +468,18 @@ class Factory: 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 @@ class Factory: 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 @@ class Factory: 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 @@ class Factory: 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 180158128..31a515da9 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.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_without +from pip._internal.req.constructors import install_req_drop_extras from .base import Candidate, CandidateLookup, Requirement, format_name @@ -41,24 +41,20 @@ class ExplicitRequirement(Requirement): 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 0c2c6ab79..847cbee8d 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -477,9 +477,7 @@ class Factory: 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 @@ class Factory: 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 f5ac31a8e..8559d9368 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 847cbee8d..03820edde 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -468,8 +468,6 @@ class Factory: 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 @@ class Factory: 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 121e48d07..315fb9c89 100644 --- a/src/pip/_internal/resolution/resolvelib/provider.py +++ b/src/pip/_internal/resolution/resolvelib/provider.py @@ -184,8 +184,6 @@ class PipProvider(_ProviderBase): # 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 31a515da9..e23b948ff 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -52,7 +52,6 @@ class SpecifierRequirement(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._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 8b1438afe..ee38b9a61 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 ee38b9a61..f97bded98 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 fc52ab9c8..88dd635ae 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 88dd635ae..e597669b3 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 003b29d38..1c3ffe80b 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 1c3ffe80b..f9a2e27c0 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 000000000..30bc60e6b --- /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 000000000..1f5018326 --- /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 03820edde..fdb5c4987 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -447,7 +447,7 @@ class Factory: 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 @@ class Factory: 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 f9a2e27c0..d7553ec03 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 f97bded98..3b7243f82 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 fdb5c4987..2812fab57 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -465,7 +465,7 @@ class Factory: ) 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 3b7243f82..b5f176e6b 100644 --- a/src/pip/_internal/req/constructors.py +++ b/src/pip/_internal/req/constructors.py @@ -16,7 +16,7 @@ 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 -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 e23b948ff..ad9892a17 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 e597669b3..77dede2fc 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 000000000..7a9ee3151 --- /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 b5f176e6b..c03ae718e 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 ad9892a17..becbd6c4b 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -56,7 +56,7 @@ class SpecifierRequirement(Requirement): 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 @@ class SpecifierRequirement(Requirement): @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 545828f8e..b4819d832 100644 --- a/tests/unit/test_req.py +++ b/tests/unit/test_req.py @@ -6,7 +6,7 @@ import sys 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 import InstallRequirement, RequirementSet 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 @@ class TestInstallRequirement: 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 7c06feaf3..d424a5e8d 100644 --- a/tests/lib/__init__.py +++ b/tests/lib/__init__.py @@ -645,7 +645,7 @@ class PipTestEnvironment(TestFileEnvironment): 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 d424a5e8d..b6996f31d 100644 --- a/tests/lib/__init__.py +++ b/tests/lib/__init__.py @@ -645,7 +645,10 @@ class PipTestEnvironment(TestFileEnvironment): 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 b6996f31d..a7f2ade1a 100644 --- a/tests/lib/__init__.py +++ b/tests/lib/__init__.py @@ -645,10 +645,7 @@ class PipTestEnvironment(TestFileEnvironment): 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 a7f2ade1a..018152930 100644 --- a/tests/lib/__init__.py +++ b/tests/lib/__init__.py @@ -645,7 +645,7 @@ class PipTestEnvironment(TestFileEnvironment): 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 4c53dfb25..2e4941da8 100644 --- a/src/pip/_internal/resolution/resolvelib/resolver.py +++ b/src/pip/_internal/resolution/resolvelib/resolver.py @@ -104,8 +104,9 @@ class Resolver(BaseResolver): 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 0eb7a1c66..81f482c86 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 @@ class Factory: 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 @@ class Factory: 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 @@ class Factory: 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 @@ class Factory: 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 c03ae718e..a40191954 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 81f482c86..af13a3321 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 @@ from .requirements import ( ExplicitRequirement, RequiresPythonRequirement, SpecifierRequirement, + SpecifierWithoutExtrasRequirement, UnsatisfiableRequirement, ) @@ -467,7 +467,7 @@ class Factory: 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 becbd6c4b..9c2512823 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -41,18 +41,9 @@ class ExplicitRequirement(Requirement): 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 @@ class SpecifierRequirement(Requirement): 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 238834841..d658be372 100644 --- a/src/pip/_internal/resolution/resolvelib/candidates.py +++ b/src/pip/_internal/resolution/resolvelib/candidates.py @@ -427,10 +427,11 @@ class ExtrasCandidate(Candidate): 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 @@ class ExtrasCandidate(Candidate): """ 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 @@ class ExtrasCandidate(Candidate): 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 af13a3321..8c5a77991 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -142,13 +142,14 @@ class Factory: 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 @@ class Factory: 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 @@ class Factory: 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 a40191954..f0f043b00 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 f0f043b00..b52c9a456 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 8c5a77991..905449f68 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -468,7 +468,7 @@ class Factory: 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 9c2512823..02cdf65f1 100644 --- a/src/pip/_internal/resolution/resolvelib/requirements.py +++ b/src/pip/_internal/resolution/resolvelib/requirements.py @@ -95,7 +95,8 @@ class SpecifierRequirement(Requirement): 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 2e4941da8..c12beef0b 100644 --- a/src/pip/_internal/resolution/resolvelib/resolver.py +++ b/src/pip/_internal/resolution/resolvelib/resolver.py @@ -104,9 +104,13 @@ class Resolver(BaseResolver): 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 ce48ab16c..642136a54 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 905449f68..2b51aab67 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -465,7 +465,6 @@ class Factory: 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 642136a54..b8cd13cb5 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 d658be372..bf89a515d 100644 --- a/src/pip/_internal/resolution/resolvelib/candidates.py +++ b/src/pip/_internal/resolution/resolvelib/candidates.py @@ -431,7 +431,7 @@ class ExtrasCandidate(Candidate): 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 77dede2fc..b5945edf8 100644 --- a/tests/functional/test_new_resolver.py +++ b/tests/functional/test_new_resolver.py @@ -6,6 +6,7 @@ from typing import TYPE_CHECKING, Callable, Dict, List, Tuple import pytest +from tests.conftest import ScriptFactory from tests.lib import ( PipTestEnvironment, create_basic_sdist_for_package, @@ -13,6 +14,7 @@ from tests.lib import ( 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"