Merge pull request #7704 from chrahunt/refactor/simpler-resolver-interface

Reduce usage of RequirementSet in commands

Author: xavfernandez

src/pip/_internal/cli/req_command.py

@@ -25,6 +25,7 @@
     install_req_from_req_string,
 )
 from pip._internal.req.req_file import parse_requirements
+from pip._internal.req.req_set import RequirementSet
 from pip._internal.self_outdated_check import (
     make_link_collector,
     pip_self_version_check,
@@ -38,7 +39,7 @@
 
     from pip._internal.cache import WheelCache
     from pip._internal.models.target_python import TargetPython
-    from pip._internal.req.req_set import RequirementSet
+    from pip._internal.req.req_install import InstallRequirement
     from pip._internal.req.req_tracker import RequirementTracker
     from pip._internal.utils.temp_dir import (
         TempDirectory,
@@ -269,19 +270,22 @@ def make_resolver(
             py_version_info=py_version_info,
         )
 
-    def populate_requirement_set(
+    def get_requirements(
         self,
-        requirement_set,  # type: RequirementSet
         args,             # type: List[str]
         options,          # type: Values
         finder,           # type: PackageFinder
         session,          # type: PipSession
         wheel_cache,      # type: Optional[WheelCache]
+        check_supported_wheels=True,  # type: bool
     ):
-        # type: (...) -> None
+        # type: (...) -> List[InstallRequirement]
         """
-        Marshal cmd line args into a requirement set.
+        Parse command-line arguments into the corresponding requirements.
         """
+        requirement_set = RequirementSet(
+            check_supported_wheels=check_supported_wheels
+        )
         for filename in options.constraints:
             for req_to_add in parse_requirements(
                     filename,
@@ -320,10 +324,7 @@ def populate_requirement_set(
                 requirement_set.add_requirement(req_to_add)
 
         # If any requirement has hash options, enable hash checking.
-        requirements = (
-            requirement_set.unnamed_requirements +
-            list(requirement_set.requirements.values())
-        )
+        requirements = requirement_set.all_requirements
         if any(req.has_hash_options for req in requirements):
             options.require_hashes = True
 
@@ -339,6 +340,8 @@ def populate_requirement_set(
                     'You must give at least one requirement to %(name)s '
                     '(see "pip help %(name)s")' % opts)
 
+        return requirements
+
     @staticmethod
     def trace_basic_info(finder):
         # type: (PackageFinder) -> None

src/pip/_internal/commands/download.py

@@ -9,7 +9,6 @@
 from pip._internal.cli import cmdoptions
 from pip._internal.cli.cmdoptions import make_target_python
 from pip._internal.cli.req_command import RequirementCommand, with_cleanup
-from pip._internal.req import RequirementSet
 from pip._internal.req.req_tracker import get_requirement_tracker
 from pip._internal.utils.misc import ensure_dir, normalize_path, write_output
 from pip._internal.utils.temp_dir import TempDirectory
@@ -102,10 +101,7 @@ def run(self, options, args):
         with get_requirement_tracker() as req_tracker, TempDirectory(
             options.build_dir, delete=build_delete, kind="download"
         ) as directory:
-
-            requirement_set = RequirementSet()
-            self.populate_requirement_set(
-                requirement_set,
+            reqs = self.get_requirements(
                 args,
                 options,
                 finder,
@@ -132,7 +128,9 @@ def run(self, options, args):
 
             self.trace_basic_info(finder)
 
-            resolver.resolve(requirement_set)
+            requirement_set = resolver.resolve(
+                reqs, check_supported_wheels=True
+            )
 
             downloaded = ' '.join([
                 req.name for req in requirement_set.successfully_downloaded

src/pip/_internal/commands/install.py

@@ -30,7 +30,7 @@
 )
 from pip._internal.locations import distutils_scheme
 from pip._internal.operations.check import check_install_conflicts
-from pip._internal.req import RequirementSet, install_given_reqs
+from pip._internal.req import install_given_reqs
 from pip._internal.req.req_tracker import get_requirement_tracker
 from pip._internal.utils.deprecation import deprecated
 from pip._internal.utils.distutils_args import parse_distutils_args
@@ -291,18 +291,14 @@ def run(self, options, args):
         with get_requirement_tracker() as req_tracker, TempDirectory(
             options.build_dir, delete=build_delete, kind="install"
         ) as directory:
-            requirement_set = RequirementSet(
-                check_supported_wheels=not options.target_dir,
-            )
-
             try:
-                self.populate_requirement_set(
-                    requirement_set, args, options, finder, session,
-                    wheel_cache
+                reqs = self.get_requirements(
+                    args, options, finder, session,
+                    wheel_cache, check_supported_wheels=not options.target_dir,
                 )
 
                 warn_deprecated_install_options(
-                    requirement_set, options.install_options
+                    reqs, options.install_options
                 )
 
                 preparer = self.make_requirement_preparer(
@@ -328,7 +324,9 @@ def run(self, options, args):
 
                 self.trace_basic_info(finder)
 
-                resolver.resolve(requirement_set)
+                requirement_set = resolver.resolve(
+                    reqs, check_supported_wheels=not options.target_dir
+                )
 
                 try:
                     pip_req = requirement_set.get_requirement("pip")
@@ -605,20 +603,15 @@ def decide_user_install(
     return True
 
 
-def warn_deprecated_install_options(requirement_set, options):
-    # type: (RequirementSet, Optional[List[str]]) -> None
+def warn_deprecated_install_options(requirements, options):
+    # type: (List[InstallRequirement], Optional[List[str]]) -> None
     """If any location-changing --install-option arguments were passed for
     requirements or on the command-line, then show a deprecation warning.
     """
     def format_options(option_names):
         # type: (Iterable[str]) -> List[str]
         return ["--{}".format(name.replace("_", "-")) for name in option_names]
 
-    requirements = (
-        requirement_set.unnamed_requirements +
-        list(requirement_set.requirements.values())
-    )
-
     offenders = []
 
     for requirement in requirements:

src/pip/_internal/commands/wheel.py

@@ -13,7 +13,6 @@
 from pip._internal.cli import cmdoptions
 from pip._internal.cli.req_command import RequirementCommand, with_cleanup
 from pip._internal.exceptions import CommandError, PreviousBuildDirError
-from pip._internal.req import RequirementSet
 from pip._internal.req.req_tracker import get_requirement_tracker
 from pip._internal.utils.misc import ensure_dir, normalize_path
 from pip._internal.utils.temp_dir import TempDirectory
@@ -128,12 +127,9 @@ def run(self, options, args):
         with get_requirement_tracker() as req_tracker, TempDirectory(
             options.build_dir, delete=build_delete, kind="wheel"
         ) as directory:
-
-            requirement_set = RequirementSet()
-
             try:
-                self.populate_requirement_set(
-                    requirement_set, args, options, finder, session,
+                reqs = self.get_requirements(
+                    args, options, finder, session,
                     wheel_cache
                 )
 
@@ -158,7 +154,9 @@ def run(self, options, args):
 
                 self.trace_basic_info(finder)
 
-                resolver.resolve(requirement_set)
+                requirement_set = resolver.resolve(
+                    reqs, check_supported_wheels=True
+                )
 
                 reqs_to_build = [
                     r for r in requirement_set.requirements.values()

src/pip/_internal/legacy_resolve.py

@@ -28,6 +28,7 @@
     HashErrors,
     UnsupportedPythonVersion,
 )
+from pip._internal.req.req_set import RequirementSet
 from pip._internal.utils.logging import indent_log
 from pip._internal.utils.misc import dist_in_usersite, normalize_version_info
 from pip._internal.utils.packaging import (
@@ -44,7 +45,6 @@
     from pip._internal.index.package_finder import PackageFinder
     from pip._internal.operations.prepare import RequirementPreparer
     from pip._internal.req.req_install import InstallRequirement
-    from pip._internal.req.req_set import RequirementSet
 
     InstallRequirementProvider = Callable[
         [str, InstallRequirement], InstallRequirement
@@ -147,8 +147,8 @@ def __init__(
         self._discovered_dependencies = \
             defaultdict(list)  # type: DiscoveredDependencies
 
-    def resolve(self, requirement_set):
-        # type: (RequirementSet) -> None
+    def resolve(self, root_reqs, check_supported_wheels):
+        # type: (List[InstallRequirement], bool) -> RequirementSet
         """Resolve what operations need to be done
 
         As a side-effect of this method, the packages (and their dependencies)
@@ -159,12 +159,11 @@ def resolve(self, requirement_set):
         possible to move the preparation to become a step separated from
         dependency resolution.
         """
-        # If any top-level requirement has a hash specified, enter
-        # hash-checking mode, which requires hashes from all.
-        root_reqs = (
-            requirement_set.unnamed_requirements +
-            list(requirement_set.requirements.values())
+        requirement_set = RequirementSet(
+            check_supported_wheels=check_supported_wheels
         )
+        for req in root_reqs:
+            requirement_set.add_requirement(req)
 
         # Actually prepare the files, and collect any exceptions. Most hash
         # exceptions cannot be checked ahead of time, because
@@ -182,6 +181,8 @@ def resolve(self, requirement_set):
         if hash_errors:
             raise hash_errors
 
+        return requirement_set
+
     def _is_upgrade_allowed(self, req):
         # type: (InstallRequirement) -> bool
         if self.upgrade_strategy == "to-satisfy-only":

src/pip/_internal/req/req_set.py

@@ -196,3 +196,8 @@ def get_requirement(self, name):
             return self.requirements[project_name]
 
         raise KeyError("No project with the name %r" % name)
+
+    @property
+    def all_requirements(self):
+        # type: () -> List[InstallRequirement]
+        return self.unnamed_requirements + list(self.requirements.values())

tests/unit/test_command_install.py

@@ -10,7 +10,6 @@
     warn_deprecated_install_options,
 )
 from pip._internal.req.req_install import InstallRequirement
-from pip._internal.req.req_set import RequirementSet
 
 
 class TestDecideUserInstall:
@@ -47,8 +46,7 @@ def test_most_cases(
 
 def test_deprecation_notice_for_pip_install_options(recwarn):
     install_options = ["--prefix=/hello"]
-    req_set = RequirementSet()
-    warn_deprecated_install_options(req_set, install_options)
+    warn_deprecated_install_options([], install_options)
 
     assert len(recwarn) == 1
     message = recwarn[0].message.args[0]
@@ -57,21 +55,20 @@ def test_deprecation_notice_for_pip_install_options(recwarn):
 
 def test_deprecation_notice_for_requirement_options(recwarn):
     install_options = []
-    req_set = RequirementSet()
 
     bad_named_req_options = {"install_options": ["--home=/wow"]}
     bad_named_req = InstallRequirement(
         Requirement("hello"), "requirements.txt", options=bad_named_req_options
     )
-    req_set.add_named_requirement(bad_named_req)
 
     bad_unnamed_req_options = {"install_options": ["--install-lib=/lib"]}
     bad_unnamed_req = InstallRequirement(
         None, "requirements2.txt", options=bad_unnamed_req_options
     )
-    req_set.add_unnamed_requirement(bad_unnamed_req)
 
-    warn_deprecated_install_options(req_set, install_options)
+    warn_deprecated_install_options(
+        [bad_named_req, bad_unnamed_req], install_options
+    )
 
     assert len(recwarn) == 1
     message = recwarn[0].message.args[0]

tests/unit/test_req.py

@@ -112,7 +112,8 @@ def test_no_reuse_existing_build_dir(self, data):
                 r"pip can't proceed with [\s\S]*%s[\s\S]*%s" %
                 (req, build_dir.replace('\\', '\\\\')),
                 resolver.resolve,
-                reqset,
+                reqset.all_requirements,
+                True,
             )
 
     # TODO: Update test when Python 2.7 is dropped.
@@ -129,7 +130,7 @@ def test_environment_marker_extras(self, data):
         reqset.add_requirement(req)
         finder = make_test_finder(find_links=[data.find_links])
         with self._basic_resolver(finder) as resolver:
-            resolver.resolve(reqset)
+            reqset = resolver.resolve(reqset.all_requirements, True)
         # This is hacky but does test both case in py2 and py3
         if sys.version_info[:2] == (2, 7):
             assert reqset.has_requirement('simple')
@@ -155,21 +156,21 @@ def test_missing_hash_with_require_hashes(self, data):
                 r'    simple==1.0 --hash=sha256:393043e672415891885c9a2a0929b1'
                 r'af95fb866d6ca016b42d2e6ce53619b653$',
                 resolver.resolve,
-                reqset
+                reqset.all_requirements,
+                True,
             )
 
     def test_missing_hash_with_require_hashes_in_reqs_file(self, data, tmpdir):
         """--require-hashes in a requirements file should make its way to the
         RequirementSet.
         """
-        req_set = RequirementSet()
         finder = make_test_finder(find_links=[data.find_links])
         session = finder._link_collector.session
         command = create_command('install')
         with requirements_file('--require-hashes', tmpdir) as reqs_file:
             options, args = command.parse_args(['-r', reqs_file])
-            command.populate_requirement_set(
-                req_set, args, options, finder, session, wheel_cache=None,
+            command.get_requirements(
+                args, options, finder, session, wheel_cache=None,
             )
         assert options.require_hashes
 
@@ -209,7 +210,8 @@ def test_unsupported_hashes(self, data):
                 r"    file://.*{sep}data{sep}packages{sep}FSPkg "
                 r"\(from -r file \(line 2\)\)".format(sep=sep),
                 resolver.resolve,
-                reqset,
+                reqset.all_requirements,
+                True,
             )
 
     def test_unpinned_hash_checking(self, data):
@@ -237,7 +239,8 @@ def test_unpinned_hash_checking(self, data):
                 r'    simple .* \(from -r file \(line 1\)\)\n'
                 r'    simple2>1.0 .* \(from -r file \(line 2\)\)',
                 resolver.resolve,
-                reqset,
+                reqset.all_requirements,
+                True,
             )
 
     def test_hash_mismatch(self, data):
@@ -258,7 +261,8 @@ def test_hash_mismatch(self, data):
                 r'             Got        393043e672415891885c9a2a0929b1af95fb'
                 r'866d6ca016b42d2e6ce53619b653$',
                 resolver.resolve,
-                reqset,
+                reqset.all_requirements,
+                True,
             )
 
     def test_unhashed_deps_on_require_hashes(self, data):
@@ -280,7 +284,8 @@ def test_unhashed_deps_on_require_hashes(self, data):
                 r'versions pinned.*\n'
                 r'    TopoRequires from .*$',
                 resolver.resolve,
-                reqset,
+                reqset.all_requirements,
+                True,
             )
 
     def test_hashed_deps_on_require_hashes(self):