Merge pull request #7694 from chrahunt/refactor/move-up-file-unpacking

Move file unpacking out of lower level functions in operations.prepare

src/pip/_internal/operations/prepare.py

@@ -134,14 +134,20 @@ def _copy_file(filename, location, link):
         logger.info('Saved %s', display_path(download_location))
 
 
-def unpack_http_url(
+class File(object):
+    def __init__(self, path, content_type):
+        # type: (str, str) -> None
+        self.path = path
+        self.content_type = content_type
+
+
+def get_http_url(
     link,  # type: Link
-    location,  # type: str
     downloader,  # type: Downloader
     download_dir=None,  # type: Optional[str]
     hashes=None,  # type: Optional[Hashes]
 ):
-    # type: (...) -> str
+    # type: (...) -> File
     temp_dir = TempDirectory(kind="unpack", globally_managed=True)
     # If a download dir is specified, is the file already downloaded there?
     already_downloaded_path = None
@@ -159,11 +165,7 @@ def unpack_http_url(
             link, downloader, temp_dir.path, hashes
         )
 
-    # unpack the archive to the build dir location. even when only
-    # downloading archives, they have to be unpacked to parse dependencies
-    unpack_file(from_path, location, content_type)
-
-    return from_path
+    return File(from_path, content_type)
 
 
 def _copy2_ignoring_special_files(src, dest):
@@ -207,23 +209,14 @@ def ignore(d, names):
     shutil.copytree(source, target, **kwargs)
 
 
-def unpack_file_url(
+def get_file_url(
     link,  # type: Link
-    location,  # type: str
     download_dir=None,  # type: Optional[str]
     hashes=None  # type: Optional[Hashes]
 ):
-    # type: (...) -> Optional[str]
-    """Unpack link into location.
+    # type: (...) -> File
+    """Get file and optionally check its hash.
     """
-    link_path = link.file_path
-    # If it's a url to a local directory
-    if link.is_existing_dir():
-        if os.path.isdir(location):
-            rmtree(location)
-        _copy_source_tree(link_path, location)
-        return None
-
     # If a download dir is specified, is the file already there and valid?
     already_downloaded_path = None
     if download_dir:
@@ -234,7 +227,7 @@ def unpack_file_url(
     if already_downloaded_path:
         from_path = already_downloaded_path
     else:
-        from_path = link_path
+        from_path = link.file_path
 
     # If --require-hashes is off, `hashes` is either empty, the
     # link's embedded hash, or MissingHashes; it is required to
@@ -246,11 +239,7 @@ def unpack_file_url(
 
     content_type = mimetypes.guess_type(from_path)[0]
 
-    # unpack the archive to the build dir location. even when only downloading
-    # archives, they have to be unpacked to parse dependencies
-    unpack_file(from_path, location, content_type)
-
-    return from_path
+    return File(from_path, content_type)
 
 
 def unpack_url(
@@ -260,7 +249,7 @@ def unpack_url(
     download_dir=None,  # type: Optional[str]
     hashes=None,  # type: Optional[Hashes]
 ):
-    # type: (...) -> Optional[str]
+    # type: (...) -> Optional[File]
     """Unpack link into location, downloading if required.
 
     :param hashes: A Hashes object, one of whose embedded hashes must match,
@@ -273,20 +262,32 @@ def unpack_url(
         unpack_vcs_link(link, location)
         return None
 
+    # If it's a url to a local directory
+    if link.is_existing_dir():
+        if os.path.isdir(location):
+            rmtree(location)
+        _copy_source_tree(link.file_path, location)
+        return None
+
     # file urls
-    elif link.is_file:
-        return unpack_file_url(link, location, download_dir, hashes=hashes)
+    if link.is_file:
+        file = get_file_url(link, download_dir, hashes=hashes)
 
     # http urls
     else:
-        return unpack_http_url(
+        file = get_http_url(
             link,
-            location,
             downloader,
             download_dir,
             hashes=hashes,
         )
 
+    # unpack the archive to the build dir location. even when only downloading
+    # archives, they have to be unpacked to parse dependencies
+    unpack_file(file.path, location, file.content_type)
+
+    return file
+
 
 def _download_http_url(
     link,  # type: Link
@@ -477,7 +478,7 @@ def prepare_linked_requirement(
                 download_dir = self.wheel_download_dir
 
             try:
-                local_path = unpack_url(
+                local_file = unpack_url(
                     link, req.source_dir, self.downloader, download_dir,
                     hashes=hashes,
                 )
@@ -494,8 +495,8 @@ def prepare_linked_requirement(
 
             # For use in later processing, preserve the file path on the
             # requirement.
-            if local_path:
-                req.local_file_path = local_path
+            if local_file:
+                req.local_file_path = local_file.path
 
             if link.is_wheel:
                 if download_dir:
@@ -519,10 +520,10 @@ def prepare_linked_requirement(
             if download_dir:
                 if link.is_existing_dir():
                     logger.info('Link is a directory, ignoring download_dir')
-                elif local_path and not os.path.exists(
+                elif local_file and not os.path.exists(
                     os.path.join(download_dir, link.filename)
                 ):
-                    _copy_file(local_path, download_dir, link)
+                    _copy_file(local_file.path, download_dir, link)
 
             if self._download_should_save:
                 # Make a .zip of the source_dir we already created.

tests/unit/test_operations_prepare.py

@@ -13,8 +13,7 @@
 from pip._internal.operations.prepare import (
     _copy_source_tree,
     _download_http_url,
-    unpack_file_url,
-    unpack_http_url,
+    unpack_url,
 )
 from pip._internal.utils.hashes import Hashes
 from pip._internal.utils.urls import path_to_url
@@ -27,7 +26,7 @@
 from tests.lib.requests_mocks import MockResponse
 
 
-def test_unpack_http_url_with_urllib_response_without_content_type(data):
+def test_unpack_url_with_urllib_response_without_content_type(data):
     """
     It should download and unpack files even if no Content-Type header exists
     """
@@ -46,7 +45,7 @@ def _fake_session_get(*args, **kwargs):
     link = Link(uri)
     temp_dir = mkdtemp()
     try:
-        unpack_http_url(
+        unpack_url(
             link,
             temp_dir,
             downloader=downloader,
@@ -172,7 +171,7 @@ def test_copy_source_tree_with_unreadable_dir_fails(clean_project, tmpdir):
     assert expected_files == copied_files
 
 
-class Test_unpack_file_url(object):
+class Test_unpack_url(object):
 
     def prep(self, tmpdir, data):
         self.build_dir = tmpdir.joinpath('build')
@@ -185,41 +184,44 @@ def prep(self, tmpdir, data):
         self.dist_path2 = data.packages.joinpath(self.dist_file2)
         self.dist_url = Link(path_to_url(self.dist_path))
         self.dist_url2 = Link(path_to_url(self.dist_path2))
+        self.no_downloader = Mock(side_effect=AssertionError)
 
-    def test_unpack_file_url_no_download(self, tmpdir, data):
+    def test_unpack_url_no_download(self, tmpdir, data):
         self.prep(tmpdir, data)
-        unpack_file_url(self.dist_url, self.build_dir)
+        unpack_url(self.dist_url, self.build_dir, self.no_downloader)
         assert os.path.isdir(os.path.join(self.build_dir, 'simple'))
         assert not os.path.isfile(
             os.path.join(self.download_dir, self.dist_file))
 
-    def test_unpack_file_url_bad_hash(self, tmpdir, data,
-                                      monkeypatch):
+    def test_unpack_url_bad_hash(self, tmpdir, data,
+                                 monkeypatch):
         """
         Test when the file url hash fragment is wrong
         """
         self.prep(tmpdir, data)
         url = '{}#md5=bogus'.format(self.dist_url.url)
         dist_url = Link(url)
         with pytest.raises(HashMismatch):
-            unpack_file_url(dist_url,
-                            self.build_dir,
-                            hashes=Hashes({'md5': ['bogus']}))
+            unpack_url(dist_url,
+                       self.build_dir,
+                       downloader=self.no_downloader,
+                       hashes=Hashes({'md5': ['bogus']}))
 
-    def test_unpack_file_url_thats_a_dir(self, tmpdir, data):
+    def test_unpack_url_thats_a_dir(self, tmpdir, data):
         self.prep(tmpdir, data)
         dist_path = data.packages.joinpath("FSPkg")
         dist_url = Link(path_to_url(dist_path))
-        unpack_file_url(dist_url, self.build_dir,
-                        download_dir=self.download_dir)
+        unpack_url(dist_url, self.build_dir,
+                   downloader=self.no_downloader,
+                   download_dir=self.download_dir)
         assert os.path.isdir(os.path.join(self.build_dir, 'fspkg'))
 
 
 @pytest.mark.parametrize('exclude_dir', [
     '.nox',
     '.tox'
 ])
-def test_unpack_file_url_excludes_expected_dirs(tmpdir, exclude_dir):
+def test_unpack_url_excludes_expected_dirs(tmpdir, exclude_dir):
     src_dir = tmpdir / 'src'
     dst_dir = tmpdir / 'dst'
     src_included_file = src_dir.joinpath('file.txt')
@@ -239,9 +241,10 @@ def test_unpack_file_url_excludes_expected_dirs(tmpdir, exclude_dir):
     dst_included_dir = dst_dir.joinpath('subdir', exclude_dir)
 
     src_link = Link(path_to_url(src_dir))
-    unpack_file_url(
+    unpack_url(
         src_link,
         dst_dir,
+        Mock(side_effect=AssertionError),
         download_dir=None
     )
     assert not os.path.isdir(dst_excluded_dir)