From aaa4237adfcbab90373e4fdb1cf5597d7fe1f405 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Sun, 27 Oct 2019 17:52:19 +0100 Subject: [PATCH 1/2] refactor should_use_ephemeral_cache --- src/pip/_internal/wheel.py | 89 +++++++++++++++++++++++++++----------- tests/unit/test_wheel.py | 84 +++++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+), 26 deletions(-) diff --git a/src/pip/_internal/wheel.py b/src/pip/_internal/wheel.py index 72168e5d831..79091e54b92 100644 --- a/src/pip/_internal/wheel.py +++ b/src/pip/_internal/wheel.py @@ -772,61 +772,98 @@ 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 req.editable or not req.source_dir: + return False + + if not check_binary_allowed(req): + 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( diff --git a/tests/unit/test_wheel.py b/tests/unit/test_wheel.py index 707451d92d2..b530bcb6920 100644 --- a/tests/unit/test_wheel.py +++ b/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", [ From e638e24e10e53a1d30d5ab89dd0b226edc2aacaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Sun, 27 Oct 2019 18:16:47 +0100 Subject: [PATCH 2/2] do not repeat tests in should_cache --- src/pip/_internal/wheel.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/pip/_internal/wheel.py b/src/pip/_internal/wheel.py index 79091e54b92..451ca57e8b2 100644 --- a/src/pip/_internal/wheel.py +++ b/src/pip/_internal/wheel.py @@ -816,10 +816,11 @@ def should_cache( wheel cache, assuming the wheel cache is available, and should_build() has determined a wheel needs to be built. """ - if req.editable or not req.source_dir: - return False - - if not check_binary_allowed(req): + 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: