From 0e1e0ef5662cae83fa5596bb5bc8386f395dd756 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Sun, 12 Jan 2020 14:20:58 +0100 Subject: [PATCH] _should_cache does not depend on check_binary_allowed _should_cache is only called by _get_cache_dir. In pip install mode, _get_cache_dir is never called when check_binary_allowed returns False because in that case should_build_for_install_command has returned False before and the build was skipped. In pip wheel mode, check_binary_allowed always returns True (because it is not passed to the build function). So _should_cache can use _always_true for check_binary_allowed. *Alternative* Alternatively, we could have passed check_binary_allowed to build in pip wheel mode. The only difference is that wheels built locally from *legacy* packages would then not be cached, when pip wheel is used with --no-binary. --- src/pip/_internal/commands/install.py | 1 - src/pip/_internal/wheel_builder.py | 18 ++---------- tests/unit/test_wheel_builder.py | 42 +++++++-------------------- 3 files changed, 13 insertions(+), 48 deletions(-) diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 82f94213e..02a187c8a 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -358,7 +358,6 @@ class InstallCommand(RequirementCommand): wheel_cache=wheel_cache, build_options=[], global_options=[], - check_binary_allowed=check_binary_allowed, ) # If we're using PEP 517, we cannot do a direct install diff --git a/src/pip/_internal/wheel_builder.py b/src/pip/_internal/wheel_builder.py index 5940e4ad9..7c7820d4f 100644 --- a/src/pip/_internal/wheel_builder.py +++ b/src/pip/_internal/wheel_builder.py @@ -107,7 +107,6 @@ def should_build_for_install_command( def _should_cache( req, # type: InstallRequirement - check_binary_allowed, # type: BinaryAllowedPredicate ): # type: (...) -> Optional[bool] """ @@ -116,7 +115,7 @@ def _should_cache( has determined a wheel needs to be built. """ if not should_build_for_install_command( - req, check_binary_allowed=check_binary_allowed + req, check_binary_allowed=_always_true ): # never cache if pip install would not have built # (editable mode, etc) @@ -144,17 +143,13 @@ def _should_cache( def _get_cache_dir( req, # type: InstallRequirement wheel_cache, # type: WheelCache - check_binary_allowed, # type: BinaryAllowedPredicate ): # type: (...) -> str """Return the persistent or temporary cache directory where the built wheel need to be stored. """ cache_available = bool(wheel_cache.cache_dir) - if ( - cache_available and - _should_cache(req, check_binary_allowed) - ): + if cache_available and _should_cache(req): cache_dir = wheel_cache.get_path_for_link(req.link) else: cache_dir = wheel_cache.get_ephem_path_for_link(req.link) @@ -263,7 +258,6 @@ def build( wheel_cache, # type: WheelCache build_options, # type: List[str] global_options, # type: List[str] - check_binary_allowed=None, # type: Optional[BinaryAllowedPredicate] ): # type: (...) -> BuildResult """Build wheels. @@ -271,10 +265,6 @@ def build( :return: The list of InstallRequirement that succeeded to build and the list of InstallRequirement that failed to build. """ - if check_binary_allowed is None: - # Binaries allowed by default. - check_binary_allowed = _always_true - if not requirements: return [], [] @@ -287,9 +277,7 @@ def build( with indent_log(): build_successes, build_failures = [], [] for req in requirements: - cache_dir = _get_cache_dir( - req, wheel_cache, check_binary_allowed - ) + cache_dir = _get_cache_dir(req, wheel_cache) wheel_file = _build_one( req, cache_dir, build_options, global_options ) diff --git a/tests/unit/test_wheel_builder.py b/tests/unit/test_wheel_builder.py index 7d70e8dcb..dcaa1e793 100644 --- a/tests/unit/test_wheel_builder.py +++ b/tests/unit/test_wheel_builder.py @@ -113,35 +113,17 @@ def test_should_build_legacy_wheel_installed(is_wheel_installed): @pytest.mark.parametrize( - "req, disallow_binaries, expected", + "req, expected", [ - (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), - (ReqMock(editable=True), True, False), - (ReqMock(source_dir=None), True, False), - (ReqMock(link=Link("git+https://g.c/org/repo")), True, False), - (ReqMock(link=Link("https://g.c/dist.tgz")), True, False), - (ReqMock(link=Link("https://g.c/dist-2.0.4.tgz")), True, False), + (ReqMock(editable=True), False), + (ReqMock(source_dir=None), False), + (ReqMock(link=Link("git+https://g.c/org/repo")), False), + (ReqMock(link=Link("https://g.c/dist.tgz")), False), + (ReqMock(link=Link("https://g.c/dist-2.0.4.tgz")), True), ], ) -def test_should_cache( - req, disallow_binaries, expected -): - def check_binary_allowed(req): - return not disallow_binaries - - should_cache = wheel_builder._should_cache( - req, check_binary_allowed - ) - if not wheel_builder.should_build_for_install_command( - req, check_binary_allowed=check_binary_allowed - ): - # never cache if pip install would not have built) - assert not should_cache - assert should_cache is expected +def test_should_cache(req, expected): + assert wheel_builder._should_cache(req) is expected def test_should_cache_git_sha(script, tmpdir): @@ -153,16 +135,12 @@ def test_should_cache_git_sha(script, tmpdir): # a link referencing a sha should be cached url = "git+https://g.c/o/r@" + commit + "#egg=mypkg" req = ReqMock(link=Link(url), source_dir=repo_path) - assert wheel_builder._should_cache( - req, check_binary_allowed=lambda r: True, - ) + assert wheel_builder._should_cache(req) # a link not referencing a sha should not be cached url = "git+https://g.c/o/r@master#egg=mypkg" req = ReqMock(link=Link(url), source_dir=repo_path) - assert not wheel_builder._should_cache( - req, check_binary_allowed=lambda r: True, - ) + assert not wheel_builder._should_cache(req) def test_format_command_result__INFO(caplog):