clarify WheelBuilder.build() a bit

news/6869.trivial

@@ -0,0 +1 @@
+Clarify WheelBuilder.build() a bit

src/pip/_internal/commands/install.py

@@ -78,7 +78,7 @@ def build_wheels(
     # Always build PEP 517 requirements
     build_failures = builder.build(
         pep517_requirements,
-        autobuilding=True,
+        should_unpack=True,
     )
 
     if should_build_legacy:
@@ -87,7 +87,7 @@ def build_wheels(
         # install for those.
         builder.build(
             legacy_requirements,
-            autobuilding=True,
+            should_unpack=True,
         )
 
     return build_failures

src/pip/_internal/wheel.py

@@ -774,7 +774,7 @@ def _contains_egg_info(
 def should_use_ephemeral_cache(
     req,  # type: InstallRequirement
     format_control,  # type: FormatControl
-    autobuilding,  # type: bool
+    should_unpack,  # type: bool
     cache_available  # type: bool
 ):
     # type: (...) -> Optional[bool]
@@ -783,20 +783,25 @@ def should_use_ephemeral_cache(
     ephemeral cache.
 
     :param cache_available: whether a cache directory is available for the
-        autobuilding=True case.
+        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.
     """
     if req.constraint:
+        # never build requirements that are merely constraints
         return None
     if req.is_wheel:
-        if not autobuilding:
+        if not should_unpack:
             logger.info(
                 'Skipping %s, due to already being wheel.', req.name,
             )
         return None
-    if not autobuilding:
+    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
         return False
 
     if req.editable or not req.source_dir:
@@ -1031,23 +1036,34 @@ class WheelBuilder(object):
     def build(
         self,
         requirements,  # type: Iterable[InstallRequirement]
-        autobuilding=False  # type: bool
+        should_unpack=False  # type: bool
     ):
         # type: (...) -> List[InstallRequirement]
         """Build wheels.
 
-        :param unpack: If True, replace the sdist we built from with the
-            newly built wheel, in preparation for installation.
+        :param should_unpack: If True, after building the wheel, unpack it
+            and replace the sdist with the unpacked version in preparation
+            for installation.
         :return: True if all the wheels built correctly.
         """
+        # pip install uses should_unpack=True.
+        # pip install never provides a _wheel_dir.
+        # pip wheel uses should_unpack=False.
+        # pip wheel always provides a _wheel_dir (via the preparer).
+        assert (
+            (should_unpack and not self._wheel_dir) or
+            (not should_unpack and self._wheel_dir)
+        )
+
         buildset = []
         format_control = self.finder.format_control
-        # Whether a cache directory is available for autobuilding=True.
-        cache_available = bool(self._wheel_dir or self.wheel_cache.cache_dir)
+        cache_available = bool(self.wheel_cache.cache_dir)
 
         for req in requirements:
             ephem_cache = should_use_ephemeral_cache(
-                req, format_control=format_control, autobuilding=autobuilding,
+                req,
+                format_control=format_control,
+                should_unpack=should_unpack,
                 cache_available=cache_available,
             )
             if ephem_cache is None:
@@ -1061,7 +1077,7 @@ class WheelBuilder(object):
         # Is any wheel build not using the ephemeral cache?
         if any(not ephem_cache for _, ephem_cache in buildset):
             have_directory_for_build = self._wheel_dir or (
-                autobuilding and self.wheel_cache.cache_dir
+                should_unpack and self.wheel_cache.cache_dir
             )
             assert have_directory_for_build
 
@@ -1078,7 +1094,7 @@ class WheelBuilder(object):
             build_success, build_failure = [], []
             for req, ephem in buildset:
                 python_tag = None
-                if autobuilding:
+                if should_unpack:
                     python_tag = pep425tags.implementation_tag
                     if ephem:
                         output_dir = _cache.get_ephem_path_for_link(req.link)
@@ -1099,7 +1115,7 @@ class WheelBuilder(object):
                 )
                 if wheel_file:
                     build_success.append(req)
-                    if autobuilding:
+                    if should_unpack:
                         # XXX: This is mildly duplicative with prepare_files,
                         # but not close enough to pull out to a single common
                         # method.

tests/unit/test_command_install.py

@@ -39,8 +39,8 @@ class TestWheelCache:
 
         # Legacy requirements were built.
         assert mock_calls == [
-            call(['a', 'b'], autobuilding=True),
-            call(['c', 'd'], autobuilding=True),
+            call(['a', 'b'], should_unpack=True),
+            call(['c', 'd'], should_unpack=True),
         ]
 
         # Legacy build failures are not included in the return value.
@@ -57,7 +57,7 @@ class TestWheelCache:
 
         # Legacy requirements were not built.
         assert mock_calls == [
-            call(['a', 'b'], autobuilding=True),
+            call(['a', 'b'], should_unpack=True),
         ]
 
         assert build_failures == ['a']

tests/unit/test_wheel.py

@@ -78,10 +78,10 @@ def test_format_tag(file_tag, expected):
 
 
 @pytest.mark.parametrize(
-    "base_name, autobuilding, cache_available, expected",
+    "base_name, should_unpack, cache_available, expected",
     [
         ('pendulum-2.0.4', False, False, False),
-        # The following cases test autobuilding=True.
+        # 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),
@@ -91,7 +91,7 @@ def test_format_tag(file_tag, expected):
     ],
 )
 def test_should_use_ephemeral_cache__issue_6197(
-    base_name, autobuilding, cache_available, expected,
+    base_name, should_unpack, cache_available, expected,
 ):
     """
     Regression test for: https://github.com/pypa/pip/issues/6197
@@ -102,7 +102,7 @@ def test_should_use_ephemeral_cache__issue_6197(
 
     format_control = FormatControl()
     ephem_cache = wheel.should_use_ephemeral_cache(
-        req, format_control=format_control, autobuilding=autobuilding,
+        req, format_control=format_control, should_unpack=should_unpack,
         cache_available=cache_available,
     )
     assert ephem_cache is expected
@@ -145,7 +145,7 @@ def test_should_use_ephemeral_cache__disallow_binaries_and_vcs_checkout(
 
     # The cache_available value doesn't matter for this test.
     ephem_cache = wheel.should_use_ephemeral_cache(
-        req, format_control=format_control, autobuilding=True,
+        req, format_control=format_control, should_unpack=True,
         cache_available=True,
     )
     assert ephem_cache is expected
@@ -697,7 +697,9 @@ class TestWheelBuilder(object):
                 as mock_build_one:
             wheel_req = Mock(is_wheel=True, editable=False, constraint=False)
             wb = wheel.WheelBuilder(
-                finder=Mock(), preparer=Mock(), wheel_cache=None,
+                finder=Mock(),
+                preparer=Mock(),
+                wheel_cache=Mock(cache_dir=None),
             )
             with caplog.at_level(logging.INFO):
                 wb.build([wheel_req])