Merge pull request #7324 from pradyunsg/refactor/require_hashes

Refactor handling of require_hashes in the Resolver+RequirementPreparer

Author: chrahunt

src/pip/_internal/cli/req_command.py

@@ -173,6 +173,7 @@ def make_requirement_preparer(
             req_tracker=req_tracker,
             session=session,
             finder=finder,
+            require_hashes=options.require_hashes,
         )
 
     @staticmethod
@@ -210,7 +211,6 @@ def make_resolver(
             force_reinstall=force_reinstall,
             upgrade_strategy=upgrade_strategy,
             py_version_info=py_version_info,
-            require_hashes=options.require_hashes,
         )
 
     def populate_requirement_set(
@@ -226,9 +226,6 @@ def populate_requirement_set(
         """
         Marshal cmd line args into a requirement set.
         """
-        # NOTE: As a side-effect, options.require_hashes and
-        #       requirement_set.require_hashes may be updated
-
         for filename in options.constraints:
             for req_to_add in parse_requirements(
                     filename,
@@ -256,6 +253,7 @@ def populate_requirement_set(
             req_to_add.is_direct = True
             requirement_set.add_requirement(req_to_add)
 
+        # NOTE: options.require_hashes may be set if --require-hashes is True
         for filename in options.requirements:
             for req_to_add in parse_requirements(
                     filename,
@@ -265,6 +263,14 @@ def populate_requirement_set(
                 req_to_add.is_direct = True
                 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())
+        )
+        if any(req.has_hash_options for req in requirements):
+            options.require_hashes = True
+
         if not (args or options.editables or options.requirements):
             opts = {'name': self.name}
             if options.find_links:

src/pip/_internal/legacy_resolve.py

@@ -124,7 +124,6 @@ def __init__(
         force_reinstall,  # type: bool
         upgrade_strategy,  # type: str
         py_version_info=None,  # type: Optional[Tuple[int, ...]]
-        require_hashes=False,  # type: bool
     ):
         # type: (...) -> None
         super(Resolver, self).__init__()
@@ -140,8 +139,6 @@ def __init__(
         self.preparer = preparer
         self.finder = finder
 
-        self.require_hashes_option = require_hashes
-
         self.upgrade_strategy = upgrade_strategy
         self.force_reinstall = force_reinstall
         self.ignore_dependencies = ignore_dependencies
@@ -176,11 +173,6 @@ def resolve(self, requirement_set):
             list(requirement_set.requirements.values())
         )
 
-        require_hashes = (
-            self.require_hashes_option or
-            any(req.has_hash_options for req in root_reqs)
-        )
-
         # Actually prepare the files, and collect any exceptions. Most hash
         # exceptions cannot be checked ahead of time, because
         # req.populate_link() needs to be called before we can make decisions
@@ -189,9 +181,7 @@ def resolve(self, requirement_set):
         hash_errors = HashErrors()
         for req in chain(root_reqs, discovered_reqs):
             try:
-                discovered_reqs.extend(
-                    self._resolve_one(requirement_set, req, require_hashes)
-                )
+                discovered_reqs.extend(self._resolve_one(requirement_set, req))
             except HashError as exc:
                 exc.req = req
                 hash_errors.append(exc)
@@ -273,14 +263,14 @@ def _check_skip_installed(self, req_to_install):
         self._set_req_to_reinstall(req_to_install)
         return None
 
-    def _get_abstract_dist_for(self, req, require_hashes):
-        # type: (InstallRequirement, bool) -> AbstractDistribution
+    def _get_abstract_dist_for(self, req):
+        # type: (InstallRequirement) -> AbstractDistribution
         """Takes a InstallRequirement and returns a single AbstractDist \
         representing a prepared variant of the same.
         """
         if req.editable:
             return self.preparer.prepare_editable_requirement(
-                req, require_hashes, self.use_user_site,
+                req, self.use_user_site
             )
 
         # satisfied_by is only evaluated by calling _check_skip_installed,
@@ -290,16 +280,15 @@ def _get_abstract_dist_for(self, req, require_hashes):
 
         if req.satisfied_by:
             return self.preparer.prepare_installed_requirement(
-                req, require_hashes, skip_reason
+                req, skip_reason
             )
 
         upgrade_allowed = self._is_upgrade_allowed(req)
 
         # We eagerly populate the link, since that's our "legacy" behavior.
+        require_hashes = self.preparer.require_hashes
         req.populate_link(self.finder, upgrade_allowed, require_hashes)
-        abstract_dist = self.preparer.prepare_linked_requirement(
-            req, require_hashes
-        )
+        abstract_dist = self.preparer.prepare_linked_requirement(req)
 
         # NOTE
         # The following portion is for determining if a certain package is
@@ -333,7 +322,6 @@ def _resolve_one(
         self,
         requirement_set,  # type: RequirementSet
         req_to_install,  # type: InstallRequirement
-        require_hashes,  # type: bool
     ):
         # type: (...) -> List[InstallRequirement]
         """Prepare a single requirements file.
@@ -351,9 +339,7 @@ def _resolve_one(
         # register tmp src for cleanup in case something goes wrong
         requirement_set.reqs_to_cleanup.append(req_to_install)
 
-        abstract_dist = self._get_abstract_dist_for(
-            req_to_install, require_hashes
-        )
+        abstract_dist = self._get_abstract_dist_for(req_to_install)
 
         # Parse and return dependencies
         dist = abstract_dist.get_pkg_resources_distribution()

src/pip/_internal/operations/prepare.py

@@ -510,6 +510,7 @@ def __init__(
         req_tracker,  # type: RequirementTracker
         session,  # type: PipSession
         finder,  # type: PackageFinder
+        require_hashes,  # type: bool
     ):
         # type: (...) -> None
         super(RequirementPreparer, self).__init__()
@@ -543,6 +544,9 @@ def __init__(
         # Is build isolation allowed?
         self.build_isolation = build_isolation
 
+        # Should hash-checking be required?
+        self.require_hashes = require_hashes
+
     @property
     def _download_should_save(self):
         # type: () -> bool
@@ -560,7 +564,6 @@ def _download_should_save(self):
     def prepare_linked_requirement(
         self,
         req,  # type: InstallRequirement
-        require_hashes,  # type: bool
     ):
         # type: (...) -> AbstractDistribution
         """Prepare a requirement that would be obtained from req.link
@@ -600,7 +603,7 @@ def prepare_linked_requirement(
             # requirements we have and raise some more informative errors
             # than otherwise. (For example, we can raise VcsHashUnsupported
             # for a VCS URL rather than HashMissing.)
-            if require_hashes:
+            if self.require_hashes:
                 # We could check these first 2 conditions inside
                 # unpack_url and save repetition of conditions, but then
                 # we would report less-useful error messages for
@@ -620,8 +623,8 @@ def prepare_linked_requirement(
                     # about them not being pinned.
                     raise HashUnpinned()
 
-            hashes = req.hashes(trust_internet=not require_hashes)
-            if require_hashes and not hashes:
+            hashes = req.hashes(trust_internet=not self.require_hashes)
+            if self.require_hashes and not hashes:
                 # Known-good hashes are missing for this requirement, so
                 # shim it with a facade object that will provoke hash
                 # computation and then raise a HashMissing exception
@@ -679,7 +682,6 @@ def prepare_linked_requirement(
     def prepare_editable_requirement(
         self,
         req,  # type: InstallRequirement
-        require_hashes,  # type: bool
         use_user_site,  # type: bool
     ):
         # type: (...) -> AbstractDistribution
@@ -690,7 +692,7 @@ def prepare_editable_requirement(
         logger.info('Obtaining %s', req)
 
         with indent_log():
-            if require_hashes:
+            if self.require_hashes:
                 raise InstallationError(
                     'The editable requirement {} cannot be installed when '
                     'requiring hashes, because there is no single file to '
@@ -712,7 +714,6 @@ def prepare_editable_requirement(
     def prepare_installed_requirement(
         self,
         req,  # type: InstallRequirement
-        require_hashes,  # type: bool
         skip_reason  # type: str
     ):
         # type: (...) -> AbstractDistribution
@@ -728,7 +729,7 @@ def prepare_installed_requirement(
             skip_reason, req, req.satisfied_by.version
         )
         with indent_log():
-            if require_hashes:
+            if self.require_hashes:
                 logger.debug(
                     'Since it is already installed, we are trusting this '
                     'package without checking its hash. To ensure a '

tests/functional/test_install.py

@@ -1,6 +1,7 @@
 import distutils
 import glob
 import os
+import re
 import shutil
 import ssl
 import sys
@@ -498,6 +499,41 @@ def test_hashed_install_failure(script, tmpdir):
     assert len(result.files_created) == 0
 
 
+def assert_re_match(pattern, text):
+    assert re.search(pattern, text), (
+        "Could not find {!r} in {!r}".format(pattern, text)
+    )
+
+
+@pytest.mark.network
+def test_hashed_install_failure_later_flag(script, tmpdir):
+    with requirements_file(
+        "blessings==1.0\n"
+        "tracefront==0.1 --hash=sha256:somehash\n"
+        "https://files.pythonhosted.org/packages/source/m/more-itertools/"
+        "more-itertools-1.0.tar.gz#md5=b21850c3cfa7efbb70fd662ab5413bdd\n"
+        "https://files.pythonhosted.org/"
+        "packages/source/p/peep/peep-3.1.1.tar.gz\n",
+        tmpdir,
+    ) as reqs_file:
+        result = script.pip(
+            "install", "-r", reqs_file.resolve(), expect_error=True
+        )
+
+    assert_re_match(
+        r'Hashes are required in --require-hashes mode, but they are '
+        r'missing .*\n'
+        r'    https://files\.pythonhosted\.org/packages/source/p/peep/peep'
+        r'-3\.1\.1\.tar\.gz --hash=sha256:[0-9a-f]+\n'
+        r'    blessings==1.0 --hash=sha256:[0-9a-f]+\n'
+        r'THESE PACKAGES DO NOT MATCH THE HASHES.*\n'
+        r'    tracefront==0.1 .*:\n'
+        r'        Expected sha256 somehash\n'
+        r'             Got        [0-9a-f]+',
+        result.stderr,
+    )
+
+
 def test_install_from_local_directory_with_symlinks_to_directories(
         script, data):
     """

tests/unit/test_req.py

@@ -72,6 +72,7 @@ def _basic_resolver(self, finder, require_hashes=False):
             req_tracker=RequirementTracker(),
             session=PipSession(),
             finder=finder,
+            require_hashes=require_hashes,
         )
         make_install_req = partial(
             install_req_from_req_string,
@@ -86,7 +87,6 @@ def _basic_resolver(self, finder, require_hashes=False):
             use_user_site=False, upgrade_strategy="to-satisfy-only",
             ignore_dependencies=False, ignore_installed=False,
             ignore_requires_python=False, force_reinstall=False,
-            require_hashes=require_hashes,
         )
 
     def test_no_reuse_existing_build_dir(self, data):
@@ -131,51 +131,6 @@ def test_environment_marker_extras(self, data):
         else:
             assert not reqset.has_requirement('simple')
 
-    @pytest.mark.network
-    def test_missing_hash_checking(self):
-        """Make sure prepare_files() raises an error when a requirement has no
-        hash in implicit hash-checking mode.
-        """
-        reqset = RequirementSet()
-        # No flags here. This tests that detection of later flags nonetheless
-        # requires earlier packages to have hashes:
-        reqset.add_requirement(get_processed_req_from_line(
-            'blessings==1.0', lineno=1
-        ))
-        # This flag activates --require-hashes mode:
-        reqset.add_requirement(get_processed_req_from_line(
-            'tracefront==0.1 --hash=sha256:somehash', lineno=2,
-        ))
-        # This hash should be accepted because it came from the reqs file, not
-        # from the internet:
-        reqset.add_requirement(get_processed_req_from_line(
-            'https://files.pythonhosted.org/packages/source/m/more-itertools/'
-            'more-itertools-1.0.tar.gz#md5=b21850c3cfa7efbb70fd662ab5413bdd',
-            lineno=3,
-        ))
-        # The error text should list this as a URL and not `peep==3.1.1`:
-        reqset.add_requirement(get_processed_req_from_line(
-            'https://files.pythonhosted.org/'
-            'packages/source/p/peep/peep-3.1.1.tar.gz',
-            lineno=4,
-        ))
-        finder = make_test_finder(index_urls=['https://pypi.org/simple/'])
-        resolver = self._basic_resolver(finder)
-        assert_raises_regexp(
-            HashErrors,
-            r'Hashes are required in --require-hashes mode, but they are '
-            r'missing .*\n'
-            r'    https://files\.pythonhosted\.org/packages/source/p/peep/peep'
-            r'-3\.1\.1\.tar\.gz --hash=sha256:[0-9a-f]+\n'
-            r'    blessings==1.0 --hash=sha256:[0-9a-f]+\n'
-            r'THESE PACKAGES DO NOT MATCH THE HASHES.*\n'
-            r'    tracefront==0.1 .*:\n'
-            r'        Expected sha256 somehash\n'
-            r'             Got        [0-9a-f]+$',
-            resolver.resolve,
-            reqset
-        )
-
     def test_missing_hash_with_require_hashes(self, data):
         """Setting --require-hashes explicitly should raise errors if hashes
         are missing.
@@ -232,7 +187,7 @@ def test_unsupported_hashes(self, data):
             lineno=2,
         ))
         finder = make_test_finder(find_links=[data.find_links])
-        resolver = self._basic_resolver(finder)
+        resolver = self._basic_resolver(finder, require_hashes=True)
         sep = os.path.sep
         if sep == '\\':
             sep = '\\\\'  # This needs to be escaped for the regex
@@ -266,7 +221,7 @@ def test_unpinned_hash_checking(self, data):
             lineno=2,
         ))
         finder = make_test_finder(find_links=[data.find_links])
-        resolver = self._basic_resolver(finder)
+        resolver = self._basic_resolver(finder, require_hashes=True)
         assert_raises_regexp(
             HashErrors,
             # Make sure all failing requirements are listed:
@@ -285,7 +240,7 @@ def test_hash_mismatch(self, data):
             '%s --hash=sha256:badbad' % file_url, lineno=1,
         ))
         finder = make_test_finder(find_links=[data.find_links])
-        resolver = self._basic_resolver(finder)
+        resolver = self._basic_resolver(finder, require_hashes=True)
         assert_raises_regexp(
             HashErrors,
             r'THESE PACKAGES DO NOT MATCH THE HASHES.*\n'
@@ -301,7 +256,7 @@ def test_unhashed_deps_on_require_hashes(self, data):
         dependencies get complained about when --require-hashes is on."""
         reqset = RequirementSet()
         finder = make_test_finder(find_links=[data.find_links])
-        resolver = self._basic_resolver(finder)
+        resolver = self._basic_resolver(finder, require_hashes=True)
         reqset.add_requirement(get_processed_req_from_line(
             'TopoRequires2==0.0.1 '  # requires TopoRequires
             '--hash=sha256:eaf9a01242c9f2f42cf2bd82a6a848cd'