Cache wheels that pip wheel built locally

news/6852.bugfix

@@ -0,0 +1,2 @@
+Cache wheels that ``pip wheel`` built locally after downloading
+them as sdist. This matches what ``pip install`` does.

src/pip/_internal/wheel.py

@@ -773,63 +773,87 @@ def _contains_egg_info(
     return bool(_egg_info_re.search(s))
 
 
-def should_use_ephemeral_cache(
+def should_build(
     req,  # type: InstallRequirement
+    need_wheel,  # type: bool
     format_control,  # type: FormatControl
-    should_unpack,  # type: bool
-    cache_available  # type: bool
 ):
-    # type: (...) -> Optional[bool]
     """
-    Return whether to build an InstallRequirement object using the
-    ephemeral cache.
-
-    :param cache_available: whether a cache directory is available for the
-        should_unpack=True case.
-
-    :return: True or False to build the requirement with ephem_cache=True
-        or False, respectively; or None not to build the requirement.
+    Return whether a wheel should be built based on the requirement
+    characteristics, and if a wheel is ultimately needed or not.
     """
     if req.constraint:
         # never build requirements that are merely constraints
-        return None
+        return False
+
     if req.is_wheel:
-        if not should_unpack:
+        if need_wheel:
             logger.info(
                 'Skipping %s, due to already being wheel.', req.name,
             )
-        return None
-    if not should_unpack:
-        # i.e. pip wheel, not pip install;
-        # return False, knowing that the caller will never cache
-        # in this case anyway, so this return merely means "build it".
-        # TODO improve this behavior
+        # never build a requirement that is already a wheel
         return False
 
-    if req.editable or not req.source_dir:
-        return None
+    if need_wheel:
+        return True
+
+    if req.editable:
+        # don't build an editable if no wheel is needed
+        return False
+
+    if not req.source_dir:
+        # TODO explain what no source_dir means
+        return False
 
     if "binary" not in format_control.get_allowed_formats(
             canonicalize_name(req.name)):
         logger.info(
             "Skipping bdist_wheel for %s, due to binaries "
             "being disabled for it.", req.name,
         )
-        return None
+        return False
+
+    return True
+
+
+def should_cache(
+    req,  # type: InstallRequirement
+    format_control,  # type: FormatControl
+):
+    # type: (...) -> Optional[bool]
+    """
+    Return whether to build an InstallRequirement object using the
+    wheel cache, assuming the wheel cache is available.
+    """
+    if req.editable:
+        # don't cache editable requirements because they can
+        # be locally modified
+        return False
+
+    if not req.source_dir:
+        # TODO explain what no source_dir means
+        return False
+
+    if "binary" not in format_control.get_allowed_formats(
+            canonicalize_name(req.name)):
+        # TODO explain this
+        return False
 
     if req.link and req.link.is_vcs:
-        # VCS checkout. Build wheel just for this run.
-        return True
+        # VCS checkout. Build wheel just for this run
+        # unless it points to an immutable commit hash in which
+        # case it can be cached.
+        # TODO cache against the commit hash if we can.
+        return False
 
     link = req.link
     base, ext = link.splitext()
-    if cache_available and _contains_egg_info(base):
-        return False
+    if _contains_egg_info(base):
+        return True
 
     # Otherwise, build the wheel just for this run using the ephemeral
-    # cache since we are either in the case of e.g. a local directory, or
-    # no cache directory is available to use.
-    return True
+    # cache since we are either in the case of e.g. a local directory.
+    return False
 
 
 def format_command_result(
@@ -1062,22 +1086,23 @@ def build(
         cache_available = bool(self.wheel_cache.cache_dir)
 
         for req in requirements:
-            ephem_cache = should_use_ephemeral_cache(
+            if not should_build(
                 req,
+                need_wheel=not should_unpack,
                 format_control=format_control,
-                should_unpack=should_unpack,
-                cache_available=cache_available,
-            )
-            if ephem_cache is None:
+            ):
                 continue
-
-            buildset.append((req, ephem_cache))
+            use_ephemeral_cache = (
+                not cache_available or
+                not should_cache(req, format_control=format_control)
+            )
+            buildset.append((req, use_ephemeral_cache))
 
         if not buildset:
             return []
 
         # Is any wheel build not using the ephemeral cache?
-        if any(not ephem_cache for _, ephem_cache in buildset):
+        if any(not use_ephemeral_cache for _, use_ephemeral_cache in buildset):
             have_directory_for_build = self._wheel_dir or (
                 should_unpack and self.wheel_cache.cache_dir
             )
@@ -1094,23 +1119,21 @@ def build(
         _cache = self.wheel_cache  # shorter name
         with indent_log():
             build_success, build_failure = [], []
-            for req, ephem in buildset:
+            for req, use_ephemeral_cache in buildset:
                 python_tag = None
                 if should_unpack:
                     python_tag = pep425tags.implementation_tag
-                    if ephem:
-                        output_dir = _cache.get_ephem_path_for_link(req.link)
-                    else:
-                        output_dir = _cache.get_path_for_link(req.link)
-                    try:
-                        ensure_dir(output_dir)
-                    except OSError as e:
-                        logger.warning("Building wheel for %s failed: %s",
-                                       req.name, e)
-                        build_failure.append(req)
-                        continue
+                if use_ephemeral_cache:
+                    output_dir = _cache.get_ephem_path_for_link(req.link)
                 else:
-                    output_dir = self._wheel_dir
+                    output_dir = _cache.get_path_for_link(req.link)
+                try:
+                    ensure_dir(output_dir)
+                except OSError as e:
+                    logger.warning("Building wheel for %s failed: %s",
+                                   req.name, e)
+                    build_failure.append(req)
+                    continue
                 wheel_file = self._build_one(
                     req, output_dir,
                     python_tag=python_tag,
@@ -1139,6 +1162,9 @@ def build(
                         assert req.link.is_wheel
                         # extract the wheel into the dir
                         unpack_file_url(link=req.link, location=req.source_dir)
+                    else:
+                        shutil.copy(wheel_file, self._wheel_dir)
+                        logger.info('Stored in directory: %s', self._wheel_dir)
                 else:
                     build_failure.append(req)
 

tests/functional/test_wheel.py

@@ -61,6 +61,30 @@ def test_pip_wheel_success(script, data):
     assert "Successfully built simple" in result.stdout, result.stdout
 
 
+def test_pip_wheel_build_cache(script, data):
+    """
+    Test 'pip wheel' builds and caches.
+    """
+    result = script.pip(
+        'wheel', '--no-index', '-f', data.find_links,
+        'simple==3.0',
+    )
+    wheel_file_name = 'simple-3.0-py%s-none-any.whl' % pyversion[0]
+    wheel_file_path = script.scratch / wheel_file_name
+    assert wheel_file_path in result.files_created, result.stdout
+    assert "Successfully built simple" in result.stdout, result.stdout
+    # remove target file
+    (script.scratch_path / wheel_file_name).unlink()
+    # pip wheel again and test that no build occurs since
+    # we get the wheel from cache
+    result = script.pip(
+        'wheel', '--no-index', '-f', data.find_links,
+        'simple==3.0',
+    )
+    assert wheel_file_path in result.files_created, result.stdout
+    assert "Successfully built simple" not in result.stdout, result.stdout
+
+
 def test_basic_pip_wheel_downloads_wheels(script, data):
     """
     Test 'pip wheel' downloads wheels

tests/unit/test_wheel.py

@@ -38,6 +38,25 @@ def test_contains_egg_info(s, expected):
     assert result == expected
 
 
+class ReqMock:
+
+    def __init__(
+        self,
+        name="pendulum",
+        is_wheel=False,
+        editable=False,
+        link=None,
+        constraint=False,
+        source_dir="/tmp/pip-install-123/pendulum",
+    ):
+        self.name = name
+        self.is_wheel = is_wheel
+        self.editable = editable
+        self.link = link
+        self.constraint = constraint
+        self.source_dir = source_dir
+
+
 def make_test_install_req(base_name=None):
     """
     Return an InstallRequirement object for testing purposes.
@@ -78,76 +97,61 @@ def test_format_tag(file_tag, expected):
 
 
 @pytest.mark.parametrize(
-    "base_name, should_unpack, cache_available, expected",
+    "req, need_wheel, disallow_binaries, expected",
     [
-        ('pendulum-2.0.4', False, False, False),
-        # The following cases test should_unpack=True.
-        # Test _contains_egg_info() returning True.
-        ('pendulum-2.0.4', True, True, False),
-        ('pendulum-2.0.4', True, False, True),
-        # Test _contains_egg_info() returning False.
-        ('pendulum', True, True, True),
-        ('pendulum', True, False, True),
+        # pip wheel (need_wheel=True)
+        (ReqMock(), True, False, True),
+        (ReqMock(), True, True, True),
+        (ReqMock(constraint=True), True, False, False),
+        (ReqMock(is_wheel=True), True, False, False),
+        (ReqMock(editable=True), True, False, True),
+        (ReqMock(source_dir=None), True, False, True),
+        (ReqMock(link=Link("git+https://g.c/org/repo")), True, False, True),
+        (ReqMock(link=Link("git+https://g.c/org/repo")), True, True, True),
+        # pip install (need_wheel=False)
+        (ReqMock(), False, False, True),
+        (ReqMock(), False, True, False),
+        (ReqMock(constraint=True), False, False, False),
+        (ReqMock(is_wheel=True), False, False, False),
+        (ReqMock(editable=True), False, False, False),
+        (ReqMock(source_dir=None), False, False, False),
+        # By default (i.e. when binaries are allowed), VCS requirements
+        # should be built in install mode.
+        (ReqMock(link=Link("git+https://g.c/org/repo")), False, False, True),
+        # Disallowing binaries, however, should cause them not to be built.
+        (ReqMock(link=Link("git+https://g.c/org/repo")), False, True, False),
     ],
 )
-def test_should_use_ephemeral_cache__issue_6197(
-    base_name, should_unpack, cache_available, expected,
-):
-    """
-    Regression test for: https://github.com/pypa/pip/issues/6197
-    """
-    req = make_test_install_req(base_name=base_name)
-    assert not req.is_wheel
-    assert req.link.is_artifact
-
+def test_should_build(req, need_wheel, disallow_binaries, expected):
     format_control = FormatControl()
-    ephem_cache = wheel.should_use_ephemeral_cache(
-        req, format_control=format_control, should_unpack=should_unpack,
-        cache_available=cache_available,
+    if disallow_binaries:
+        format_control.disallow_binaries()
+    should_build = wheel.should_build(
+        req, need_wheel=need_wheel, format_control=format_control
     )
-    assert ephem_cache is expected
+    assert should_build is expected
 
 
 @pytest.mark.parametrize(
-    "disallow_binaries, expected",
+    "req, disallow_binaries, expected",
     [
-        # By default (i.e. when binaries are allowed), VCS requirements
-        # should be built.
-        (False, True),
-        # Disallowing binaries, however, should cause them not to be built.
-        (True, None),
+        (ReqMock(editable=True), False, False),
+        (ReqMock(source_dir=None), False, False),
+        (ReqMock(link=Link("git+https://g.c/org/repo")), False, False),
+        (ReqMock(link=Link("https://g.c/dist.tgz")), False, False),
+        (ReqMock(link=Link("https://g.c/dist-2.0.4.tgz")), False, True),
     ],
 )
-def test_should_use_ephemeral_cache__disallow_binaries_and_vcs_checkout(
-    disallow_binaries, expected,
+def test_should_cache(
+    req, disallow_binaries, expected
 ):
-    """
-    Test that disallowing binaries (e.g. from passing --global-option)
-    causes should_use_ephemeral_cache() to return None for VCS checkouts.
-    """
-    req = Requirement('pendulum')
-    link = Link(url='git+https://git.example.com/pendulum.git')
-    req = InstallRequirement(
-        req=req,
-        comes_from=None,
-        constraint=False,
-        editable=False,
-        link=link,
-        source_dir='/tmp/pip-install-9py5m2z1/pendulum',
-    )
-    assert not req.is_wheel
-    assert req.link.is_vcs
-
     format_control = FormatControl()
     if disallow_binaries:
         format_control.disallow_binaries()
-
-    # The cache_available value doesn't matter for this test.
-    ephem_cache = wheel.should_use_ephemeral_cache(
-        req, format_control=format_control, should_unpack=True,
-        cache_available=True,
+    should_cache = wheel.should_cache(
+        req, format_control=format_control,
     )
-    assert ephem_cache is expected
+    assert should_cache is expected
 
 
 def test_format_command_result__INFO(caplog):