From 806a013bc009dc48c3e50b4007072abc7ce362de Mon Sep 17 00:00:00 2001 From: Chris Jerdonek Date: Fri, 1 Feb 2019 02:23:58 -0800 Subject: [PATCH 1/4] Add failing test for the have_directory_for_build AssertionError. --- src/pip/_internal/wheel.py | 89 +++++++++++++++++++++++++------------- tests/unit/test_wheel.py | 37 ++++++++++++++++ 2 files changed, 96 insertions(+), 30 deletions(-) diff --git a/src/pip/_internal/wheel.py b/src/pip/_internal/wheel.py index 03bff0bf9a2..e6c738ad242 100644 --- a/src/pip/_internal/wheel.py +++ b/src/pip/_internal/wheel.py @@ -48,7 +48,7 @@ from pip._vendor.packaging.requirements import Requirement # noqa: F401 from pip._internal.req.req_install import InstallRequirement # noqa: F401 from pip._internal.download import PipSession # noqa: F401 - from pip._internal.index import PackageFinder # noqa: F401 + from pip._internal.index import FormatControl, PackageFinder # noqa: F401 from pip._internal.operations.prepare import ( # noqa: F401 RequirementPreparer ) @@ -725,6 +725,58 @@ def _contains_egg_info( return bool(_egg_info_re.search(s)) +def should_use_ephemeral_cache( + req, # type: InstallRequirement + format_control, # type: FormatControl + autobuilding # type: bool +): + # type: (...) -> Optional[bool] + """ + Return whether to build an InstallRequirement object using the + ephemeral cache. + + The function returns True, False, or None, as follows: + True / False: build the requirement with ephem_cache True or + False, respectively. + None: don't build the requirement. + """ + if req.constraint: + return None + if req.is_wheel: + if not autobuilding: + logger.info( + 'Skipping %s, due to already being wheel.', req.name, + ) + return None + if autobuilding and req.editable: + return None + if autobuilding and not req.source_dir: + return None + + if autobuilding and req.link and not req.link.is_artifact: + # VCS checkout. Build wheel just for this run. + return True + + if not autobuilding: + return False + + if "binary" not in format_control.get_allowed_formats( + canonicalize_name(req.name)): + logger.info( + "Skipping bdist_wheel for %s, due to binaries " + "being disabled for it.", req.name, + ) + return None + + link = req.link + base, ext = link.splitext() + if _contains_egg_info(base): + return False + + # Otherwise, e.g. local directory. Build wheel just for this run. + return True + + class WheelBuilder(object): """Build wheels from a RequirementSet.""" @@ -862,36 +914,13 @@ def build( buildset = [] format_control = self.finder.format_control for req in requirements: - if req.constraint: + ephem_cache = should_use_ephemeral_cache( + req, format_control=format_control, autobuilding=autobuilding, + ) + if ephem_cache is None: continue - if req.is_wheel: - if not autobuilding: - logger.info( - 'Skipping %s, due to already being wheel.', req.name, - ) - elif autobuilding and req.editable: - pass - elif autobuilding and not req.source_dir: - pass - elif autobuilding and req.link and not req.link.is_artifact: - # VCS checkout. Build wheel just for this run. - buildset.append((req, True)) - else: - ephem_cache = False - if autobuilding: - link = req.link - base, ext = link.splitext() - if not _contains_egg_info(base): - # E.g. local directory. Build wheel just for this run. - ephem_cache = True - if "binary" not in format_control.get_allowed_formats( - canonicalize_name(req.name)): - logger.info( - "Skipping bdist_wheel for %s, due to binaries " - "being disabled for it.", req.name, - ) - continue - buildset.append((req, ephem_cache)) + + buildset.append((req, ephem_cache)) if not buildset: return [] diff --git a/tests/unit/test_wheel.py b/tests/unit/test_wheel.py index 6fe125da8ee..792b4e99a49 100644 --- a/tests/unit/test_wheel.py +++ b/tests/unit/test_wheel.py @@ -10,6 +10,9 @@ from pip._internal import pep425tags, wheel from pip._internal.exceptions import InvalidWheelFilename, UnsupportedWheel +from pip._internal.index import FormatControl +from pip._internal.models.link import Link +from pip._internal.req.req_install import InstallRequirement from pip._internal.utils.compat import WINDOWS from pip._internal.utils.misc import unpack_file from tests.lib import DATA_DIR @@ -35,6 +38,40 @@ def test_contains_egg_info(s, expected): assert result == expected +def test_should_use_ephemeral_cache__issue_6197(): + """ + Regression test for: https://github.com/pypa/pip/issues/6197 + """ + req = Requirement('pendulum') + link_url = ( + 'https://files.pythonhosted.org/packages/aa/pendulum-2.0.4.tar.gz' + '#sha256=cf535d36c063575d4752af36df928882b2e0e31541b4482c97d637527' + '85f9fcb' + ) + link = Link( + url=link_url, + comes_from='https://pypi.org/simple/pendulum/', + requires_python='>=2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*', + ) + req = InstallRequirement( + req=req, + comes_from=None, + constraint=False, + editable=False, + link=link, + source_dir='/tmp/pip-install-9py5m2z1/pendulum', + ) + assert not req.is_wheel + assert req.link.is_artifact + + format_control = FormatControl() + autobuilding = True + ephem_cache = wheel.should_use_ephemeral_cache( + req, format_control=format_control, autobuilding=autobuilding, + ) + assert ephem_cache is False + + @pytest.mark.parametrize("console_scripts", ["pip = pip._internal.main:pip", "pip:pip = pip._internal.main:pip"]) From 6b0892eace9a26452310f096c654f33bf2001e15 Mon Sep 17 00:00:00 2001 From: Chris Jerdonek Date: Fri, 1 Feb 2019 02:24:16 -0800 Subject: [PATCH 2/4] Use the ephemeral cache if autobuilding and no cache directory is available. --- news/6197.bugfix | 3 +++ src/pip/_internal/wheel.py | 23 +++++++++++++++-------- tests/unit/test_wheel.py | 4 +++- 3 files changed, 21 insertions(+), 9 deletions(-) create mode 100644 news/6197.bugfix diff --git a/news/6197.bugfix b/news/6197.bugfix new file mode 100644 index 00000000000..e1b24a6fedc --- /dev/null +++ b/news/6197.bugfix @@ -0,0 +1,3 @@ +Fix a crash where PEP 517-based builds using ``--no-cache-dir`` would fail in +some circumstances with an ``AssertionError`` due to not finalizing a build +directory internally. diff --git a/src/pip/_internal/wheel.py b/src/pip/_internal/wheel.py index e6c738ad242..e75855b341f 100644 --- a/src/pip/_internal/wheel.py +++ b/src/pip/_internal/wheel.py @@ -728,17 +728,19 @@ def _contains_egg_info( def should_use_ephemeral_cache( req, # type: InstallRequirement format_control, # type: FormatControl - autobuilding # type: bool + autobuilding, # type: bool + cache_available # type: bool ): # type: (...) -> Optional[bool] """ Return whether to build an InstallRequirement object using the ephemeral cache. - The function returns True, False, or None, as follows: - True / False: build the requirement with ephem_cache True or - False, respectively. - None: don't build the requirement. + :param cache_available: whether a cache directory is available for the + autobuilding=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: return None @@ -770,10 +772,12 @@ def should_use_ephemeral_cache( link = req.link base, ext = link.splitext() - if _contains_egg_info(base): + if cache_available and _contains_egg_info(base): return False - # Otherwise, e.g. local directory. Build wheel just for this run. + # 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 @@ -910,12 +914,15 @@ def build( newly built wheel, in preparation for installation. :return: True if all the wheels built correctly. """ - 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) + for req in requirements: ephem_cache = should_use_ephemeral_cache( req, format_control=format_control, autobuilding=autobuilding, + cache_available=cache_available, ) if ephem_cache is None: continue diff --git a/tests/unit/test_wheel.py b/tests/unit/test_wheel.py index 792b4e99a49..6f6498697af 100644 --- a/tests/unit/test_wheel.py +++ b/tests/unit/test_wheel.py @@ -66,10 +66,12 @@ def test_should_use_ephemeral_cache__issue_6197(): format_control = FormatControl() autobuilding = True + cache_available = False ephem_cache = wheel.should_use_ephemeral_cache( req, format_control=format_control, autobuilding=autobuilding, + cache_available=cache_available, ) - assert ephem_cache is False + assert ephem_cache is True @pytest.mark.parametrize("console_scripts", From d7b256651ccbf6064f9e9d5de65f75607515ccd8 Mon Sep 17 00:00:00 2001 From: Chris Jerdonek Date: Thu, 31 Jan 2019 01:58:38 -0800 Subject: [PATCH 3/4] Simplify if logic in should_use_ephemeral_cache(). --- src/pip/_internal/wheel.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/pip/_internal/wheel.py b/src/pip/_internal/wheel.py index e75855b341f..d9e8f2216a2 100644 --- a/src/pip/_internal/wheel.py +++ b/src/pip/_internal/wheel.py @@ -750,18 +750,16 @@ def should_use_ephemeral_cache( 'Skipping %s, due to already being wheel.', req.name, ) return None - if autobuilding and req.editable: - return None - if autobuilding and not req.source_dir: + if not autobuilding: + return False + + if req.editable or not req.source_dir: return None - if autobuilding and req.link and not req.link.is_artifact: + if req.link and not req.link.is_artifact: # VCS checkout. Build wheel just for this run. return True - if not autobuilding: - return False - if "binary" not in format_control.get_allowed_formats( canonicalize_name(req.name)): logger.info( From 2d9d3ff90be75bcb2000bd389ef8a964cc543a2f Mon Sep 17 00:00:00 2001 From: Chris Jerdonek Date: Fri, 1 Feb 2019 02:40:23 -0800 Subject: [PATCH 4/4] Add more unit test cases. --- tests/unit/test_wheel.py | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/tests/unit/test_wheel.py b/tests/unit/test_wheel.py index 6f6498697af..4a8a0bde727 100644 --- a/tests/unit/test_wheel.py +++ b/tests/unit/test_wheel.py @@ -38,16 +38,31 @@ def test_contains_egg_info(s, expected): assert result == expected -def test_should_use_ephemeral_cache__issue_6197(): +@pytest.mark.parametrize( + "base_name, autobuilding, cache_available, expected", + [ + ('pendulum-2.0.4', False, False, False), + # The following cases test autobuilding=True. + # Test _contains_egg_info() returning True. + ('pendulum-2.0.4', True, True, False), + ('pendulum-2.0.4', True, False, True), + # Test _contains_egg_info() returning False. + ('pendulum', True, True, True), + ('pendulum', True, False, True), + ], +) +def test_should_use_ephemeral_cache__issue_6197( + base_name, autobuilding, cache_available, expected, +): """ Regression test for: https://github.com/pypa/pip/issues/6197 """ req = Requirement('pendulum') link_url = ( - 'https://files.pythonhosted.org/packages/aa/pendulum-2.0.4.tar.gz' + 'https://files.pythonhosted.org/packages/aa/{base_name}.tar.gz' '#sha256=cf535d36c063575d4752af36df928882b2e0e31541b4482c97d637527' '85f9fcb' - ) + ).format(base_name=base_name) link = Link( url=link_url, comes_from='https://pypi.org/simple/pendulum/', @@ -65,13 +80,11 @@ def test_should_use_ephemeral_cache__issue_6197(): assert req.link.is_artifact format_control = FormatControl() - autobuilding = True - cache_available = False ephem_cache = wheel.should_use_ephemeral_cache( req, format_control=format_control, autobuilding=autobuilding, cache_available=cache_available, ) - assert ephem_cache is True + assert ephem_cache is expected @pytest.mark.parametrize("console_scripts",