From d673aa14284788ea12a789b34846353b7cb3d46f Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Fri, 13 May 2022 08:46:45 +0100 Subject: [PATCH] Move `RequirementSet.add_requirement` into `LegacyResolver` This was the only call-site for this method and, realistically, it is highly coupled with the legacy resolver's dependency resolution strategy/approach; so it makes sense for this code to live as part of the resolver, rather than the container object the various resolvers. --- src/pip/_internal/req/req_set.py | 122 +--------------- .../_internal/resolution/legacy/resolver.py | 130 +++++++++++++++++- tests/unit/test_req.py | 63 ++------- tests/unit/test_resolution_legacy_resolver.py | 127 +++++++++++++---- 4 files changed, 238 insertions(+), 204 deletions(-) diff --git a/src/pip/_internal/req/req_set.py b/src/pip/_internal/req/req_set.py index 6626c37e2..0f550bf1e 100644 --- a/src/pip/_internal/req/req_set.py +++ b/src/pip/_internal/req/req_set.py @@ -1,13 +1,10 @@ import logging from collections import OrderedDict -from typing import Dict, Iterable, List, Optional, Tuple +from typing import Dict, List from pip._vendor.packaging.utils import canonicalize_name -from pip._internal.exceptions import InstallationError -from pip._internal.models.wheel import Wheel from pip._internal.req.req_install import InstallRequirement -from pip._internal.utils import compatibility_tags logger = logging.getLogger(__name__) @@ -51,123 +48,6 @@ class RequirementSet: project_name = canonicalize_name(install_req.name) self.requirements[project_name] = install_req - def add_requirement( - self, - install_req: InstallRequirement, - parent_req_name: Optional[str] = None, - extras_requested: Optional[Iterable[str]] = None, - ) -> Tuple[List[InstallRequirement], Optional[InstallRequirement]]: - """Add install_req as a requirement to install. - - :param parent_req_name: The name of the requirement that needed this - added. The name is used because when multiple unnamed requirements - resolve to the same name, we could otherwise end up with dependency - links that point outside the Requirements set. parent_req must - already be added. Note that None implies that this is a user - supplied requirement, vs an inferred one. - :param extras_requested: an iterable of extras used to evaluate the - environment markers. - :return: Additional requirements to scan. That is either [] if - the requirement is not applicable, or [install_req] if the - requirement is applicable and has just been added. - """ - # If the markers do not match, ignore this requirement. - if not install_req.match_markers(extras_requested): - logger.info( - "Ignoring %s: markers '%s' don't match your environment", - install_req.name, - install_req.markers, - ) - return [], None - - # If the wheel is not supported, raise an error. - # Should check this after filtering out based on environment markers to - # allow specifying different wheels based on the environment/OS, in a - # single requirements file. - if install_req.link and install_req.link.is_wheel: - wheel = Wheel(install_req.link.filename) - tags = compatibility_tags.get_supported() - if self.check_supported_wheels and not wheel.supported(tags): - raise InstallationError( - "{} is not a supported wheel on this platform.".format( - wheel.filename - ) - ) - - # This next bit is really a sanity check. - assert ( - not install_req.user_supplied or parent_req_name is None - ), "a user supplied req shouldn't have a parent" - - # Unnamed requirements are scanned again and the requirement won't be - # added as a dependency until after scanning. - if not install_req.name: - self.add_unnamed_requirement(install_req) - return [install_req], None - - try: - existing_req: Optional[InstallRequirement] = self.get_requirement( - install_req.name - ) - except KeyError: - existing_req = None - - has_conflicting_requirement = ( - parent_req_name is None - and existing_req - and not existing_req.constraint - and existing_req.extras == install_req.extras - and existing_req.req - and install_req.req - and existing_req.req.specifier != install_req.req.specifier - ) - if has_conflicting_requirement: - raise InstallationError( - "Double requirement given: {} (already in {}, name={!r})".format( - install_req, existing_req, install_req.name - ) - ) - - # When no existing requirement exists, add the requirement as a - # dependency and it will be scanned again after. - if not existing_req: - self.add_named_requirement(install_req) - # We'd want to rescan this requirement later - return [install_req], install_req - - # Assume there's no need to scan, and that we've already - # encountered this for scanning. - if install_req.constraint or not existing_req.constraint: - return [], existing_req - - does_not_satisfy_constraint = install_req.link and not ( - existing_req.link and install_req.link.path == existing_req.link.path - ) - if does_not_satisfy_constraint: - raise InstallationError( - "Could not satisfy constraints for '{}': " - "installation from path or url cannot be " - "constrained to a version".format(install_req.name) - ) - # If we're now installing a constraint, mark the existing - # object for real installation. - existing_req.constraint = False - # If we're now installing a user supplied requirement, - # mark the existing object as such. - if install_req.user_supplied: - existing_req.user_supplied = True - existing_req.extras = tuple( - sorted(set(existing_req.extras) | set(install_req.extras)) - ) - logger.debug( - "Setting %s extras to: %s", - existing_req, - existing_req.extras, - ) - # Return the existing requirement for addition to the parent and - # scanning again. - return [existing_req], existing_req - def has_requirement(self, name: str) -> bool: project_name = canonicalize_name(name) diff --git a/src/pip/_internal/resolution/legacy/resolver.py b/src/pip/_internal/resolution/legacy/resolver.py index 8c149d437..1225ae70f 100644 --- a/src/pip/_internal/resolution/legacy/resolver.py +++ b/src/pip/_internal/resolution/legacy/resolver.py @@ -28,12 +28,14 @@ from pip._internal.exceptions import ( DistributionNotFound, HashError, HashErrors, + InstallationError, NoneMetadataError, UnsupportedPythonVersion, ) from pip._internal.index.package_finder import PackageFinder from pip._internal.metadata import BaseDistribution from pip._internal.models.link import Link +from pip._internal.models.wheel import Wheel from pip._internal.operations.prepare import RequirementPreparer from pip._internal.req.req_install import ( InstallRequirement, @@ -41,6 +43,7 @@ from pip._internal.req.req_install import ( ) from pip._internal.req.req_set import RequirementSet from pip._internal.resolution.base import BaseResolver, InstallRequirementProvider +from pip._internal.utils import compatibility_tags from pip._internal.utils.compatibility_tags import get_supported from pip._internal.utils.logging import indent_log from pip._internal.utils.misc import normalize_version_info @@ -168,7 +171,7 @@ class Resolver(BaseResolver): for req in root_reqs: if req.constraint: check_invalid_constraint_type(req) - requirement_set.add_requirement(req) + self._add_requirement_to_set(requirement_set, req) # Actually prepare the files, and collect any exceptions. Most hash # exceptions cannot be checked ahead of time, because @@ -188,6 +191,124 @@ class Resolver(BaseResolver): return requirement_set + def _add_requirement_to_set( + self, + requirement_set: RequirementSet, + install_req: InstallRequirement, + parent_req_name: Optional[str] = None, + extras_requested: Optional[Iterable[str]] = None, + ) -> Tuple[List[InstallRequirement], Optional[InstallRequirement]]: + """Add install_req as a requirement to install. + + :param parent_req_name: The name of the requirement that needed this + added. The name is used because when multiple unnamed requirements + resolve to the same name, we could otherwise end up with dependency + links that point outside the Requirements set. parent_req must + already be added. Note that None implies that this is a user + supplied requirement, vs an inferred one. + :param extras_requested: an iterable of extras used to evaluate the + environment markers. + :return: Additional requirements to scan. That is either [] if + the requirement is not applicable, or [install_req] if the + requirement is applicable and has just been added. + """ + # If the markers do not match, ignore this requirement. + if not install_req.match_markers(extras_requested): + logger.info( + "Ignoring %s: markers '%s' don't match your environment", + install_req.name, + install_req.markers, + ) + return [], None + + # If the wheel is not supported, raise an error. + # Should check this after filtering out based on environment markers to + # allow specifying different wheels based on the environment/OS, in a + # single requirements file. + if install_req.link and install_req.link.is_wheel: + wheel = Wheel(install_req.link.filename) + tags = compatibility_tags.get_supported() + if requirement_set.check_supported_wheels and not wheel.supported(tags): + raise InstallationError( + "{} is not a supported wheel on this platform.".format( + wheel.filename + ) + ) + + # This next bit is really a sanity check. + assert ( + not install_req.user_supplied or parent_req_name is None + ), "a user supplied req shouldn't have a parent" + + # Unnamed requirements are scanned again and the requirement won't be + # added as a dependency until after scanning. + if not install_req.name: + requirement_set.add_unnamed_requirement(install_req) + return [install_req], None + + try: + existing_req: Optional[ + InstallRequirement + ] = requirement_set.get_requirement(install_req.name) + except KeyError: + existing_req = None + + has_conflicting_requirement = ( + parent_req_name is None + and existing_req + and not existing_req.constraint + and existing_req.extras == install_req.extras + and existing_req.req + and install_req.req + and existing_req.req.specifier != install_req.req.specifier + ) + if has_conflicting_requirement: + raise InstallationError( + "Double requirement given: {} (already in {}, name={!r})".format( + install_req, existing_req, install_req.name + ) + ) + + # When no existing requirement exists, add the requirement as a + # dependency and it will be scanned again after. + if not existing_req: + requirement_set.add_named_requirement(install_req) + # We'd want to rescan this requirement later + return [install_req], install_req + + # Assume there's no need to scan, and that we've already + # encountered this for scanning. + if install_req.constraint or not existing_req.constraint: + return [], existing_req + + does_not_satisfy_constraint = install_req.link and not ( + existing_req.link and install_req.link.path == existing_req.link.path + ) + if does_not_satisfy_constraint: + raise InstallationError( + "Could not satisfy constraints for '{}': " + "installation from path or url cannot be " + "constrained to a version".format(install_req.name) + ) + # If we're now installing a constraint, mark the existing + # object for real installation. + existing_req.constraint = False + # If we're now installing a user supplied requirement, + # mark the existing object as such. + if install_req.user_supplied: + existing_req.user_supplied = True + existing_req.extras = tuple( + sorted(set(existing_req.extras) | set(install_req.extras)) + ) + logger.debug( + "Setting %s extras to: %s", + existing_req, + existing_req.extras, + ) + # Return the existing requirement for addition to the parent and + # scanning again. + return [existing_req], existing_req + def _is_upgrade_allowed(self, req: InstallRequirement) -> bool: if self.upgrade_strategy == "to-satisfy-only": return False @@ -393,7 +514,8 @@ class Resolver(BaseResolver): # the legacy resolver so I'm just not going to bother refactoring. sub_install_req = self._make_install_req(str(subreq), req_to_install) parent_req_name = req_to_install.name - to_scan_again, add_to_parent = requirement_set.add_requirement( + to_scan_again, add_to_parent = self._add_requirement_to_set( + requirement_set, sub_install_req, parent_req_name=parent_req_name, extras_requested=extras_requested, @@ -410,7 +532,9 @@ class Resolver(BaseResolver): # 'unnamed' requirements can only come from being directly # provided by the user. assert req_to_install.user_supplied - requirement_set.add_requirement(req_to_install, parent_req_name=None) + self._add_requirement_to_set( + requirement_set, req_to_install, parent_req_name=None + ) if not self.ignore_dependencies: if req_to_install.extras: diff --git a/tests/unit/test_req.py b/tests/unit/test_req.py index 74f4a8bab..975c1c12d 100644 --- a/tests/unit/test_req.py +++ b/tests/unit/test_req.py @@ -123,7 +123,7 @@ class TestRequirementSet: reqset = RequirementSet() req = install_req_from_line("simple") req.user_supplied = True - reqset.add_requirement(req) + reqset.add_named_requirement(req) finder = make_test_finder(find_links=[data.find_links]) with self._basic_resolver(finder) as resolver: with pytest.raises( @@ -144,7 +144,7 @@ class TestRequirementSet: reqset = RequirementSet() req = install_req_from_editable(data.packages.joinpath("LocalEnvironMarker")) req.user_supplied = True - reqset.add_requirement(req) + reqset.add_unnamed_requirement(req) finder = make_test_finder(find_links=[data.find_links]) with self._basic_resolver(finder) as resolver: reqset = resolver.resolve(reqset.all_requirements, True) @@ -155,7 +155,9 @@ class TestRequirementSet: are missing. """ reqset = RequirementSet() - reqset.add_requirement(get_processed_req_from_line("simple==1.0", lineno=1)) + reqset.add_named_requirement( + get_processed_req_from_line("simple==1.0", lineno=1) + ) finder = make_test_finder(find_links=[data.find_links]) @@ -194,14 +196,14 @@ class TestRequirementSet: """ reqset = RequirementSet() - reqset.add_requirement( + reqset.add_unnamed_requirement( get_processed_req_from_line( "git+git://github.com/pypa/pip-test-package --hash=sha256:123", lineno=1, ) ) dir_path = data.packages.joinpath("FSPkg") - reqset.add_requirement( + reqset.add_unnamed_requirement( get_processed_req_from_line( f"file://{dir_path}", lineno=2, @@ -235,7 +237,7 @@ class TestRequirementSet: """ reqset = RequirementSet() # Test that there must be exactly 1 specifier: - reqset.add_requirement( + reqset.add_named_requirement( get_processed_req_from_line( "simple --hash=sha256:a90427ae31f5d1d0d7ec06ee97d9fcf2d0fc9a786985" "250c1c83fd68df5911dd", @@ -243,7 +245,7 @@ class TestRequirementSet: ) ) # Test that the operator must be ==: - reqset.add_requirement( + reqset.add_named_requirement( get_processed_req_from_line( "simple2>1.0 --hash=sha256:3ad45e1e9aa48b4462af0" "123f6a7e44a9115db1ef945d4d92c123dfe21815a06", @@ -267,7 +269,7 @@ class TestRequirementSet: """A hash mismatch should raise an error.""" file_url = path_to_url((data.packages / "simple-1.0.tar.gz").resolve()) reqset = RequirementSet() - reqset.add_requirement( + reqset.add_unnamed_requirement( get_processed_req_from_line( f"{file_url} --hash=sha256:badbad", lineno=1, @@ -292,7 +294,7 @@ class TestRequirementSet: dependencies get complained about when --require-hashes is on.""" reqset = RequirementSet() finder = make_test_finder(find_links=[data.find_links]) - reqset.add_requirement( + reqset.add_named_requirement( get_processed_req_from_line( "TopoRequires2==0.0.1 " # requires TopoRequires "--hash=sha256:eaf9a01242c9f2f42cf2bd82a6a848cd" @@ -322,7 +324,7 @@ class TestRequirementSet: """ reqset = RequirementSet() - reqset.add_requirement( + reqset.add_named_requirement( get_processed_req_from_line( "TopoRequires2==0.0.1 " # requires TopoRequires "--hash=sha256:eaf9a01242c9f2f42cf2bd82a6a848cd" @@ -330,7 +332,7 @@ class TestRequirementSet: lineno=1, ) ) - reqset.add_requirement( + reqset.add_named_requirement( get_processed_req_from_line( "TopoRequires==0.0.1 " "--hash=sha256:d6dd1e22e60df512fdcf3640ced3039b3b02a56ab2cee81ebcb" @@ -382,32 +384,6 @@ class TestInstallRequirement: assert req.link.scheme == "https" assert req.link.url == url - def test_unsupported_wheel_link_requirement_raises(self) -> None: - reqset = RequirementSet() - req = install_req_from_line( - "https://whatever.com/peppercorn-0.4-py2.py3-bogus-any.whl", - ) - assert req.link is not None - assert req.link.is_wheel - assert req.link.scheme == "https" - - with pytest.raises(InstallationError): - reqset.add_requirement(req) - - def test_unsupported_wheel_local_file_requirement_raises( - self, data: TestData - ) -> None: - reqset = RequirementSet() - req = install_req_from_line( - data.packages.joinpath("simple.dist-0.1-py1-none-invalid.whl"), - ) - assert req.link is not None - assert req.link.is_wheel - assert req.link.scheme == "file" - - with pytest.raises(InstallationError): - reqset.add_requirement(req) - def test_str(self) -> None: req = install_req_from_line("simple==0.1") assert str(req) == "simple==0.1" @@ -658,19 +634,6 @@ def test_parse_editable_local_extras( ) -def test_exclusive_environment_markers() -> None: - """Make sure RequirementSet accepts several excluding env markers""" - eq36 = install_req_from_line("Django>=1.6.10,<1.7 ; python_version == '3.6'") - eq36.user_supplied = True - ne36 = install_req_from_line("Django>=1.6.10,<1.8 ; python_version != '3.6'") - ne36.user_supplied = True - - req_set = RequirementSet() - req_set.add_requirement(eq36) - req_set.add_requirement(ne36) - assert req_set.has_requirement("Django") - - def test_mismatched_versions(caplog: pytest.LogCaptureFixture) -> None: req = InstallRequirement( req=Requirement("simplewheel==2.0"), diff --git a/tests/unit/test_resolution_legacy_resolver.py b/tests/unit/test_resolution_legacy_resolver.py index 7b5ba6372..134ff65b4 100644 --- a/tests/unit/test_resolution_legacy_resolver.py +++ b/tests/unit/test_resolution_legacy_resolver.py @@ -7,15 +7,20 @@ import pytest from pip._vendor.packaging.specifiers import SpecifierSet from pip._vendor.packaging.utils import NormalizedName -from pip._internal.exceptions import NoneMetadataError, UnsupportedPythonVersion +from pip._internal.exceptions import ( + InstallationError, + NoneMetadataError, + UnsupportedPythonVersion, +) from pip._internal.metadata import BaseDistribution from pip._internal.models.candidate import InstallationCandidate from pip._internal.req.constructors import install_req_from_line +from pip._internal.req.req_set import RequirementSet from pip._internal.resolution.legacy.resolver import ( Resolver, _check_dist_requires_python, ) -from tests.lib import make_test_finder +from tests.lib import TestData, make_test_finder from tests.lib.index import make_mock_candidate T = TypeVar("T") @@ -50,8 +55,94 @@ def make_fake_dist( return klass(metadata) # type: ignore[call-arg] -class TestCheckDistRequiresPython: +def make_test_resolver( + monkeypatch: pytest.MonkeyPatch, + mock_candidates: List[InstallationCandidate], +) -> Resolver: + def _find_candidates(project_name: str) -> List[InstallationCandidate]: + return mock_candidates + finder = make_test_finder() + monkeypatch.setattr(finder, "find_all_candidates", _find_candidates) + + return Resolver( + finder=finder, + preparer=mock.Mock(), # Not used. + make_install_req=install_req_from_line, + wheel_cache=None, + use_user_site=False, + force_reinstall=False, + ignore_dependencies=False, + ignore_installed=False, + ignore_requires_python=False, + upgrade_strategy="to-satisfy-only", + ) + + +class TestAddRequirement: + """ + Test _add_requirement_to_set(). + """ + + def test_unsupported_wheel_link_requirement_raises( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + # GIVEN + resolver = make_test_resolver(monkeypatch, []) + requirement_set = RequirementSet(check_supported_wheels=True) + + install_req = install_req_from_line( + "https://whatever.com/peppercorn-0.4-py2.py3-bogus-any.whl", + ) + assert install_req.link is not None + assert install_req.link.is_wheel + assert install_req.link.scheme == "https" + + # WHEN / THEN + with pytest.raises(InstallationError): + resolver._add_requirement_to_set(requirement_set, install_req) + + def test_unsupported_wheel_local_file_requirement_raises( + self, data: TestData, monkeypatch: pytest.MonkeyPatch + ) -> None: + # GIVEN + resolver = make_test_resolver(monkeypatch, []) + requirement_set = RequirementSet(check_supported_wheels=True) + + install_req = install_req_from_line( + data.packages.joinpath("simple.dist-0.1-py1-none-invalid.whl"), + ) + assert install_req.link is not None + assert install_req.link.is_wheel + assert install_req.link.scheme == "file" + + # WHEN / THEN + with pytest.raises(InstallationError): + resolver._add_requirement_to_set(requirement_set, install_req) + + def test_exclusive_environment_markers( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Make sure excluding environment markers are handled correctly.""" + # GIVEN + resolver = make_test_resolver(monkeypatch, []) + requirement_set = RequirementSet(check_supported_wheels=True) + + eq36 = install_req_from_line("Django>=1.6.10,<1.7 ; python_version == '3.6'") + eq36.user_supplied = True + ne36 = install_req_from_line("Django>=1.6.10,<1.8 ; python_version != '3.6'") + ne36.user_supplied = True + + # WHEN + resolver._add_requirement_to_set(requirement_set, eq36) + resolver._add_requirement_to_set(requirement_set, ne36) + + # THEN + assert requirement_set.has_requirement("Django") + assert len(requirement_set.all_requirements) == 1 + + +class TestCheckDistRequiresPython: """ Test _check_dist_requires_python(). """ @@ -179,30 +270,6 @@ class TestYankedWarning: Test _populate_link() emits warning if one or more candidates are yanked. """ - def _make_test_resolver( - self, - monkeypatch: pytest.MonkeyPatch, - mock_candidates: List[InstallationCandidate], - ) -> Resolver: - def _find_candidates(project_name: str) -> List[InstallationCandidate]: - return mock_candidates - - finder = make_test_finder() - monkeypatch.setattr(finder, "find_all_candidates", _find_candidates) - - return Resolver( - finder=finder, - preparer=mock.Mock(), # Not used. - make_install_req=install_req_from_line, - wheel_cache=None, - use_user_site=False, - force_reinstall=False, - ignore_dependencies=False, - ignore_installed=False, - ignore_requires_python=False, - upgrade_strategy="to-satisfy-only", - ) - def test_sort_best_candidate__has_non_yanked( self, caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -219,7 +286,7 @@ class TestYankedWarning: ] ireq = install_req_from_line("pkg") - resolver = self._make_test_resolver(monkeypatch, candidates) + resolver = make_test_resolver(monkeypatch, candidates) resolver._populate_link(ireq) assert ireq.link == candidates[0].link @@ -243,7 +310,7 @@ class TestYankedWarning: ] ireq = install_req_from_line("pkg") - resolver = self._make_test_resolver(monkeypatch, candidates) + resolver = make_test_resolver(monkeypatch, candidates) resolver._populate_link(ireq) assert ireq.link == candidates[1].link @@ -287,7 +354,7 @@ class TestYankedWarning: ] ireq = install_req_from_line("pkg") - resolver = self._make_test_resolver(monkeypatch, candidates) + resolver = make_test_resolver(monkeypatch, candidates) resolver._populate_link(ireq) assert ireq.link == candidates[0].link