Merge pull request #7517 from sbidoul/wheel-builder-disentangle-6-sbi

Move final wheel builder copy operation to wheel command

Author: chrahunt

news/7517.feature

@@ -0,0 +1,4 @@
+The build step of ``pip wheel`` now builds all wheels to a cache first,
+then copies them to the wheel directory all at once.
+Before, it built them to a temporary direcory and moved
+them to the wheel directory one by one.

src/pip/_internal/commands/install.py

@@ -83,7 +83,7 @@ def build_wheels(
     should_build_legacy = is_wheel_installed()
 
     # Always build PEP 517 requirements
-    build_failures = builder.build(
+    _, build_failures = builder.build(
         pep517_requirements,
         should_unpack=True,
     )

src/pip/_internal/commands/wheel.py

@@ -7,13 +7,15 @@
 
 import logging
 import os
+import shutil
 
 from pip._internal.cache import WheelCache
 from pip._internal.cli import cmdoptions
 from pip._internal.cli.req_command import RequirementCommand
 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
 from pip._internal.utils.temp_dir import TempDirectory
 from pip._internal.utils.typing import MYPY_CHECK_RUNNING
 from pip._internal.wheel_builder import WheelBuilder
@@ -161,10 +163,23 @@ def run(self, options, args):
                     build_options=options.build_options or [],
                     global_options=options.global_options or [],
                 )
-                build_failures = wb.build(
+                build_successes, build_failures = wb.build(
                     requirement_set.requirements.values(),
                     should_unpack=False,
                 )
+                for req in build_successes:
+                    assert req.link and req.link.is_wheel
+                    assert req.local_file_path
+                    # copy from cache to target directory
+                    try:
+                        ensure_dir(options.wheel_dir)
+                        shutil.copy(req.local_file_path, options.wheel_dir)
+                    except OSError as e:
+                        logger.warning(
+                            "Building wheel for %s failed: %s",
+                            req.name, e,
+                        )
+                        build_failures.append(req)
                 if len(build_failures) != 0:
                     raise CommandError(
                         "Failed to build one or more wheels"

src/pip/_internal/wheel_builder.py

@@ -38,6 +38,7 @@
     from pip._vendor.pep517.wrappers import Pep517HookCaller
 
     BinaryAllowedPredicate = Callable[[InstallRequirement], bool]
+    BuildResult = Tuple[List[InstallRequirement], List[InstallRequirement]]
 
 logger = logging.getLogger(__name__)
 
@@ -216,8 +217,6 @@ def __init__(
         self.preparer = preparer
         self.wheel_cache = wheel_cache
 
-        self._wheel_dir = preparer.wheel_download_dir
-
         self.build_options = build_options or []
         self.global_options = global_options or []
         self.check_binary_allowed = check_binary_allowed
@@ -311,31 +310,23 @@ def build(
         requirements,  # type: Iterable[InstallRequirement]
         should_unpack,  # type: bool
     ):
-        # type: (...) -> List[InstallRequirement]
+        # type: (...) -> BuildResult
         """Build wheels.
 
         :param should_unpack: If True, after building the wheel, unpack it
             and replace the sdist with the unpacked version in preparation
             for installation.
-        :return: The list of InstallRequirement that failed to build.
+        :return: The list of InstallRequirement that succeeded to build and
+            the list of InstallRequirement that failed to build.
         """
-        # 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 = _collect_buildset(
             requirements,
             wheel_cache=self.wheel_cache,
             check_binary_allowed=self.check_binary_allowed,
             need_wheel=not should_unpack,
         )
         if not buildset:
-            return []
+            return [], []
 
         # TODO by @pradyunsg
         # Should break up this method into 2 separate methods.
@@ -347,7 +338,7 @@ def build(
         )
 
         with indent_log():
-            build_success, build_failure = [], []
+            build_successes, build_failures = [], []
             for req, cache_dir in buildset:
                 wheel_file = self._build_one(req, cache_dir)
                 if wheel_file:
@@ -376,32 +367,20 @@ def build(
                         )
                         # extract the wheel into the dir
                         unpack_file(req.link.file_path, req.source_dir)
-                    else:
-                        # copy from cache to target directory
-                        try:
-                            ensure_dir(self._wheel_dir)
-                            shutil.copy(wheel_file, self._wheel_dir)
-                        except OSError as e:
-                            logger.warning(
-                                "Building wheel for %s failed: %s",
-                                req.name, e,
-                            )
-                            build_failure.append(req)
-                            continue
-                    build_success.append(req)
+                    build_successes.append(req)
                 else:
-                    build_failure.append(req)
+                    build_failures.append(req)
 
         # notify success/failure
-        if build_success:
+        if build_successes:
             logger.info(
                 'Successfully built %s',
-                ' '.join([req.name for req in build_success]),
+                ' '.join([req.name for req in build_successes]),
             )
-        if build_failure:
+        if build_failures:
             logger.info(
                 'Failed to build %s',
-                ' '.join([req.name for req in build_failure]),
+                ' '.join([req.name for req in build_failures]),
             )
         # Return a list of requirements that failed to build
-        return build_failure
+        return build_successes, build_failures

tests/unit/test_command_install.py

@@ -26,7 +26,7 @@ def check_build_wheels(
         """
         def build(reqs, **kwargs):
             # Fail the first requirement.
-            return [reqs[0]]
+            return ([], [reqs[0]])
 
         builder = Mock()
         builder.build.side_effect = build