Merge pull request #11119 from pradyunsg/move-add_requirement-to-legacy-resolver

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)
 

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:

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

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