From ea517a2bb9be824da0bd5508dc7598928d13d92d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Tue, 13 Aug 2019 14:14:23 +0200 Subject: [PATCH] clarify WheelBuilder.build() a bit --- news/6869.trivial | 1 + src/pip/_internal/commands/install.py | 4 +-- src/pip/_internal/wheel.py | 42 ++++++++++++++++++--------- tests/unit/test_command_install.py | 6 ++-- tests/unit/test_wheel.py | 14 +++++---- 5 files changed, 43 insertions(+), 24 deletions(-) create mode 100644 news/6869.trivial diff --git a/news/6869.trivial b/news/6869.trivial new file mode 100644 index 000000000..1da3453fb --- /dev/null +++ b/news/6869.trivial @@ -0,0 +1 @@ +Clarify WheelBuilder.build() a bit \ No newline at end of file diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 46e667433..673c6d214 100644 --- a/src/pip/_internal/commands/install.py +++ b/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 diff --git a/src/pip/_internal/wheel.py b/src/pip/_internal/wheel.py index 71109e2cd..cd0d3460f 100644 --- a/src/pip/_internal/wheel.py +++ b/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. diff --git a/tests/unit/test_command_install.py b/tests/unit/test_command_install.py index 30469171f..1a3fee5c9 100644 --- a/tests/unit/test_command_install.py +++ b/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'] diff --git a/tests/unit/test_wheel.py b/tests/unit/test_wheel.py index ea4fe4eba..9775d6f31 100644 --- a/tests/unit/test_wheel.py +++ b/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])