_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.

Author: sbidoul

src/pip/_internal/commands/install.py

@@ -358,7 +358,6 @@ def run(self, options, args):
                     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

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,18 +258,13 @@ 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.
 
     :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
             )

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):