Merge pull request #7262 from sbidoul/refactor-should_use_ephem_cache-sbi

Refactor should_use_ephemeral_cache

Author: chrahunt

src/pip/_internal/wheel.py

@@ -772,61 +772,99 @@ def _contains_egg_info(
     return bool(_egg_info_re.search(s))
 
 
-def should_use_ephemeral_cache(
+def should_build(
     req,  # type: InstallRequirement
-    should_unpack,  # type: bool
-    cache_available,  # type: bool
+    need_wheel,  # type: bool
     check_binary_allowed,  # type: BinaryAllowedPredicate
 ):
     # type: (...) -> Optional[bool]
-    """Return whether to build an InstallRequirement object using the
-    ephemeral cache.
-
-    :param cache_available: whether a cache directory is available for the
-        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.
-    """
+    """Return whether an InstallRequirement should be built into a wheel."""
     if req.constraint:
         # never build requirements that are merely constraints
-        return None
+        return False
     if req.is_wheel:
-        if not should_unpack:
+        if need_wheel:
             logger.info(
                 'Skipping %s, due to already being wheel.', req.name,
             )
-        return None
-    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 need_wheel:
+        # i.e. pip wheel, not pip install
+        return True
+
     if req.editable or not req.source_dir:
-        return None
+        return False
 
     if not check_binary_allowed(req):
         logger.info(
             "Skipping wheel build for %s, due to binaries "
             "being disabled for it.", req.name,
         )
-        return None
+        return False
+
+    return True
+
+
+def should_cache(
+    req,  # type: InstallRequirement
+    check_binary_allowed,  # type: BinaryAllowedPredicate
+):
+    # type: (...) -> Optional[bool]
+    """
+    Return whether a built InstallRequirement can be stored in the persistent
+    wheel cache, assuming the wheel cache is available, and should_build()
+    has determined a wheel needs to be built.
+    """
+    if not should_build(
+        req, need_wheel=False, check_binary_allowed=check_binary_allowed
+    ):
+        # never cache if pip install (need_wheel=False) would not have built
+        # (editable mode, etc)
+        return False
 
     if req.link and req.link.is_vcs:
         # VCS checkout. Build wheel just for this run.
-        return True
+        return False
 
     link = req.link
     base, ext = link.splitext()
-    if cache_available and _contains_egg_info(base):
-        return False
+    if _contains_egg_info(base):
+        return True
 
     # Otherwise, build the wheel just for this run using the ephemeral
     # cache since we are either in the case of e.g. a local directory, or
     # no cache directory is available to use.
-    return True
+    return False
+
+
+def should_use_ephemeral_cache(
+    req,  # type: InstallRequirement
+    should_unpack,  # type: bool
+    cache_available,  # type: bool
+    check_binary_allowed,  # type: BinaryAllowedPredicate
+):
+    # type: (...) -> Optional[bool]
+    """Return whether to build an InstallRequirement object using the
+    ephemeral cache.
+
+    :param cache_available: whether a cache directory is available for the
+        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 not should_build(req, not should_unpack, check_binary_allowed):
+        return None
+    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 not cache_available:
+        return True
+    return not should_cache(req, check_binary_allowed)
 
 
 def format_command_result(

tests/unit/test_wheel.py

@@ -23,6 +23,25 @@
 from tests.lib import DATA_DIR, assert_paths_equal
 
 
+class ReqMock:
+
+    def __init__(
+        self,
+        name="pendulum",
+        is_wheel=False,
+        editable=False,
+        link=None,
+        constraint=False,
+        source_dir="/tmp/pip-install-123/pendulum",
+    ):
+        self.name = name
+        self.is_wheel = is_wheel
+        self.editable = editable
+        self.link = link
+        self.constraint = constraint
+        self.source_dir = source_dir
+
+
 @pytest.mark.parametrize(
     "s, expected",
     [
@@ -82,6 +101,71 @@ def test_format_tag(file_tag, expected):
     assert actual == expected
 
 
+@pytest.mark.parametrize(
+    "req, need_wheel, disallow_binaries, expected",
+    [
+        # pip wheel (need_wheel=True)
+        (ReqMock(), True, False, True),
+        (ReqMock(), True, True, True),
+        (ReqMock(constraint=True), True, False, False),
+        (ReqMock(is_wheel=True), True, False, False),
+        (ReqMock(editable=True), True, False, True),
+        (ReqMock(source_dir=None), True, False, True),
+        (ReqMock(link=Link("git+https://g.c/org/repo")), True, False, True),
+        (ReqMock(link=Link("git+https://g.c/org/repo")), True, True, True),
+        # pip install (need_wheel=False)
+        (ReqMock(), False, False, True),
+        (ReqMock(), False, True, False),
+        (ReqMock(constraint=True), False, False, False),
+        (ReqMock(is_wheel=True), False, False, False),
+        (ReqMock(editable=True), False, False, False),
+        (ReqMock(source_dir=None), False, False, False),
+        # By default (i.e. when binaries are allowed), VCS requirements
+        # should be built in install mode.
+        (ReqMock(link=Link("git+https://g.c/org/repo")), False, False, True),
+        # Disallowing binaries, however, should cause them not to be built.
+        (ReqMock(link=Link("git+https://g.c/org/repo")), False, True, False),
+    ],
+)
+def test_should_build(req, need_wheel, disallow_binaries, expected):
+    should_build = wheel.should_build(
+        req,
+        need_wheel,
+        check_binary_allowed=lambda req: not disallow_binaries,
+    )
+    assert should_build is expected
+
+
+@pytest.mark.parametrize(
+    "req, disallow_binaries, 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),
+    ],
+)
+def test_should_cache(
+    req, disallow_binaries, expected
+):
+    def check_binary_allowed(req):
+        return not disallow_binaries
+
+    should_cache = wheel.should_cache(req, check_binary_allowed)
+    if not wheel.should_build(
+        req, need_wheel=False, check_binary_allowed=check_binary_allowed
+    ):
+        # never cache if pip install (need_wheel=False) would not have built)
+        assert not should_cache
+    assert should_cache is expected
+
+
 @pytest.mark.parametrize(
     "base_name, should_unpack, cache_available, expected",
     [