From cf21401fd73fa74f690dac71386b09b6a8814760 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Thu, 26 Dec 2019 21:11:39 +0100 Subject: [PATCH 1/5] Make wheelbuilder.build return successes too Also, pluralize variable names for readability and consistency with similar variables in callers. --- src/pip/_internal/commands/install.py | 2 +- src/pip/_internal/commands/wheel.py | 2 +- src/pip/_internal/wheel_builder.py | 23 ++++++++++++----------- tests/unit/test_command_install.py | 2 +- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index c7dcf28df8a..747d243537d 100644 --- a/src/pip/_internal/commands/install.py +++ b/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, ) diff --git a/src/pip/_internal/commands/wheel.py b/src/pip/_internal/commands/wheel.py index 8b3ddd0dccf..af50b214f96 100644 --- a/src/pip/_internal/commands/wheel.py +++ b/src/pip/_internal/commands/wheel.py @@ -161,7 +161,7 @@ def run(self, options, args): build_options=options.build_options or [], global_options=options.global_options or [], ) - build_failures = wb.build( + _, build_failures = wb.build( requirement_set.requirements.values(), should_unpack=False, ) diff --git a/src/pip/_internal/wheel_builder.py b/src/pip/_internal/wheel_builder.py index 627cba30496..c61be727d09 100644 --- a/src/pip/_internal/wheel_builder.py +++ b/src/pip/_internal/wheel_builder.py @@ -43,6 +43,7 @@ from pip._vendor.pep517.wrappers import Pep517HookCaller BinaryAllowedPredicate = Callable[[InstallRequirement], bool] + BuildResult = Tuple[List[InstallRequirement], List[InstallRequirement]] logger = logging.getLogger(__name__) @@ -413,7 +414,7 @@ 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 @@ -437,7 +438,7 @@ def build( need_wheel=not should_unpack, ) if not buildset: - return [] + return [], [] # TODO by @pradyunsg # Should break up this method into 2 separate methods. @@ -449,7 +450,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: @@ -488,22 +489,22 @@ def build( "Building wheel for %s failed: %s", req.name, e, ) - build_failure.append(req) + build_failures.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 diff --git a/tests/unit/test_command_install.py b/tests/unit/test_command_install.py index 7f7154c94f1..9d862792bba 100644 --- a/tests/unit/test_command_install.py +++ b/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 From 9909b4069a41a74893d324e67e6ffabc18308d3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Thu, 26 Dec 2019 21:24:32 +0100 Subject: [PATCH 2/5] Move final copy operation from wheel_builder to wheel command --- src/pip/_internal/commands/wheel.py | 17 ++++++++++++++++- src/pip/_internal/wheel_builder.py | 12 ------------ 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/pip/_internal/commands/wheel.py b/src/pip/_internal/commands/wheel.py index af50b214f96..d3fe3430886 100644 --- a/src/pip/_internal/commands/wheel.py +++ b/src/pip/_internal/commands/wheel.py @@ -7,6 +7,7 @@ import logging import os +import shutil from pip._internal.cache import WheelCache from pip._internal.cli import cmdoptions @@ -14,6 +15,7 @@ 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" diff --git a/src/pip/_internal/wheel_builder.py b/src/pip/_internal/wheel_builder.py index c61be727d09..3f3123cb081 100644 --- a/src/pip/_internal/wheel_builder.py +++ b/src/pip/_internal/wheel_builder.py @@ -479,18 +479,6 @@ 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_failures.append(req) - continue build_successes.append(req) else: build_failures.append(req) From 158ae67910f3e769acd0663fd52b6a1647882034 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Thu, 26 Dec 2019 21:26:48 +0100 Subject: [PATCH 3/5] Remove unused _wheel_dir in WheelBuilder --- src/pip/_internal/wheel_builder.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/pip/_internal/wheel_builder.py b/src/pip/_internal/wheel_builder.py index 3f3123cb081..a3692e49787 100644 --- a/src/pip/_internal/wheel_builder.py +++ b/src/pip/_internal/wheel_builder.py @@ -319,8 +319,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 @@ -422,15 +420,6 @@ def build( for installation. :return: 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, From 7c2c58442f5de276e96cbaa66a86d798c5086de2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Fri, 27 Dec 2019 09:40:02 +0100 Subject: [PATCH 4/5] Update docstring --- src/pip/_internal/wheel_builder.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/wheel_builder.py b/src/pip/_internal/wheel_builder.py index a3692e49787..200e70fc986 100644 --- a/src/pip/_internal/wheel_builder.py +++ b/src/pip/_internal/wheel_builder.py @@ -418,7 +418,8 @@ def build( :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. """ buildset = _collect_buildset( requirements, From 1f39950f3a8021c1fadbe291042d4b31f56caa48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Sat, 28 Dec 2019 16:12:58 +0100 Subject: [PATCH 5/5] Add news file explaining the new pip wheel behavior --- news/7517.feature | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 news/7517.feature diff --git a/news/7517.feature b/news/7517.feature new file mode 100644 index 00000000000..089fbc38781 --- /dev/null +++ b/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.