diff --git a/news/7527.trivial b/news/7527.trivial new file mode 100644 index 000000000..e69de29bb diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py index 0f39b90d8..82f94213e 100644 --- a/src/pip/_internal/commands/install.py +++ b/src/pip/_internal/commands/install.py @@ -44,7 +44,7 @@ from pip._internal.utils.misc import ( from pip._internal.utils.temp_dir import TempDirectory from pip._internal.utils.typing import MYPY_CHECK_RUNNING from pip._internal.utils.virtualenv import virtualenv_no_global -from pip._internal.wheel_builder import WheelBuilder +from pip._internal.wheel_builder import build, should_build_for_install_command if MYPY_CHECK_RUNNING: from optparse import Values @@ -58,60 +58,6 @@ if MYPY_CHECK_RUNNING: logger = logging.getLogger(__name__) -def is_wheel_installed(): - """ - Return whether the wheel package is installed. - """ - try: - import wheel # noqa: F401 - except ImportError: - return False - - return True - - -def build_wheels( - builder, # type: WheelBuilder - pep517_requirements, # type: List[InstallRequirement] - legacy_requirements, # type: List[InstallRequirement] - wheel_cache, # type: WheelCache - build_options, # type: List[str] - global_options, # type: List[str] - check_binary_allowed, # type: BinaryAllowedPredicate -): - # type: (...) -> List[InstallRequirement] - """ - Build wheels for requirements, depending on whether wheel is installed. - """ - # We don't build wheels for legacy requirements if wheel is not installed. - should_build_legacy = is_wheel_installed() - - # Always build PEP 517 requirements - _, build_failures = builder.build( - pep517_requirements, - should_unpack=True, - wheel_cache=wheel_cache, - build_options=build_options, - global_options=global_options, - check_binary_allowed=check_binary_allowed, - ) - - if should_build_legacy: - # We don't care about failures building legacy - # requirements, as we'll fall through to a direct - # install for those. - builder.build( - legacy_requirements, - should_unpack=True, - wheel_cache=wheel_cache, - build_options=build_options, - global_options=global_options, - check_binary_allowed=check_binary_allowed, - ) - - return build_failures - - def get_check_binary_allowed(format_control): # type: (FormatControl) -> BinaryAllowedPredicate def check_binary_allowed(req): @@ -399,20 +345,16 @@ class InstallCommand(RequirementCommand): check_binary_allowed = get_check_binary_allowed( finder.format_control ) - # Consider legacy and PEP517-using requirements separately - legacy_requirements = [] - pep517_requirements = [] - for req in requirement_set.requirements.values(): - if req.use_pep517: - pep517_requirements.append(req) - else: - legacy_requirements.append(req) - wheel_builder = WheelBuilder(preparer) - build_failures = build_wheels( - builder=wheel_builder, - pep517_requirements=pep517_requirements, - legacy_requirements=legacy_requirements, + reqs_to_build = [ + r for r in requirement_set.requirements.values() + if should_build_for_install_command( + r, check_binary_allowed + ) + ] + + _, build_failures = build( + reqs_to_build, wheel_cache=wheel_cache, build_options=[], global_options=[], @@ -421,11 +363,17 @@ class InstallCommand(RequirementCommand): # If we're using PEP 517, we cannot do a direct install # so we fail here. - if build_failures: + # We don't care about failures building legacy + # requirements, as we'll fall through to a direct + # install for those. + pep517_build_failures = [ + r for r in build_failures if r.use_pep517 + ] + if pep517_build_failures: raise InstallationError( "Could not build wheels for {} which use" " PEP 517 and cannot be installed directly".format( - ", ".join(r.name for r in build_failures))) + ", ".join(r.name for r in pep517_build_failures))) to_install = resolver.get_installation_order( requirement_set diff --git a/src/pip/_internal/commands/wheel.py b/src/pip/_internal/commands/wheel.py index e376440b7..eb44bcee4 100644 --- a/src/pip/_internal/commands/wheel.py +++ b/src/pip/_internal/commands/wheel.py @@ -18,7 +18,7 @@ from pip._internal.req.req_tracker import get_requirement_tracker from pip._internal.utils.misc import ensure_dir, normalize_path from pip._internal.utils.temp_dir import TempDirectory from pip._internal.utils.typing import MYPY_CHECK_RUNNING -from pip._internal.wheel_builder import WheelBuilder +from pip._internal.wheel_builder import build, should_build_for_wheel_command if MYPY_CHECK_RUNNING: from optparse import Values @@ -160,11 +160,14 @@ class WheelCommand(RequirementCommand): resolver.resolve(requirement_set) + reqs_to_build = [ + r for r in requirement_set.requirements.values() + if should_build_for_wheel_command(r) + ] + # build wheels - wb = WheelBuilder(preparer) - build_successes, build_failures = wb.build( - requirement_set.requirements.values(), - should_unpack=False, + build_successes, build_failures = build( + reqs_to_build, wheel_cache=wheel_cache, build_options=options.build_options or [], global_options=options.global_options or [], diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index a53ce0dc9..19a96290e 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -878,3 +878,15 @@ def hash_file(path, blocksize=1 << 20): length += len(block) h.update(block) return h, length + + +def is_wheel_installed(): + """ + Return whether the wheel package is installed. + """ + try: + import wheel # noqa: F401 + except ImportError: + return False + + return True diff --git a/src/pip/_internal/wheel_builder.py b/src/pip/_internal/wheel_builder.py index 9015795b9..5940e4ad9 100644 --- a/src/pip/_internal/wheel_builder.py +++ b/src/pip/_internal/wheel_builder.py @@ -13,7 +13,7 @@ from pip._internal.models.link import Link from pip._internal.operations.build.wheel import build_wheel_pep517 from pip._internal.operations.build.wheel_legacy import build_wheel_legacy from pip._internal.utils.logging import indent_log -from pip._internal.utils.misc import ensure_dir, hash_file +from pip._internal.utils.misc import ensure_dir, hash_file, is_wheel_installed from pip._internal.utils.setuptools_build import make_setuptools_clean_args from pip._internal.utils.subprocess import call_subprocess from pip._internal.utils.temp_dir import TempDirectory @@ -27,9 +27,6 @@ if MYPY_CHECK_RUNNING: ) from pip._internal.cache import WheelCache - from pip._internal.operations.prepare import ( - RequirementPreparer - ) from pip._internal.req.req_install import InstallRequirement BinaryAllowedPredicate = Callable[[InstallRequirement], bool] @@ -48,12 +45,12 @@ def _contains_egg_info( return bool(_egg_info_re.search(s)) -def should_build( +def _should_build( req, # type: InstallRequirement need_wheel, # type: bool check_binary_allowed, # type: BinaryAllowedPredicate ): - # type: (...) -> Optional[bool] + # type: (...) -> bool """Return whether an InstallRequirement should be built into a wheel.""" if req.constraint: # never build requirements that are merely constraints @@ -69,6 +66,13 @@ def should_build( # i.e. pip wheel, not pip install return True + # From this point, this concerns the pip install command only + # (need_wheel=False). + + if not req.use_pep517 and not is_wheel_installed(): + # we don't build legacy requirements if wheel is not installed + return False + if req.editable or not req.source_dir: return False @@ -82,27 +86,45 @@ def should_build( return True -def should_cache( +def should_build_for_wheel_command( + req, # type: InstallRequirement +): + # type: (...) -> bool + return _should_build( + req, need_wheel=True, check_binary_allowed=_always_true + ) + + +def should_build_for_install_command( + req, # type: InstallRequirement + check_binary_allowed, # type: BinaryAllowedPredicate +): + # type: (...) -> bool + return _should_build( + req, need_wheel=False, check_binary_allowed=check_binary_allowed + ) + + +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() + 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 + if not should_build_for_install_command( + req, check_binary_allowed=check_binary_allowed ): - # never cache if pip install (need_wheel=False) would not have built + # never cache if pip install 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 - # unless it points to an immutable commit hash in which - # case it can be cached. + # VCS checkout. Do not cache + # unless it points to an immutable commit hash. assert not req.editable assert req.source_dir vcs_backend = vcs.get_backend_for_scheme(req.link.scheme) @@ -111,46 +133,32 @@ def should_cache( return True return False - link = req.link - base, ext = link.splitext() + base, ext = req.link.splitext() 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. + # Otherwise, do not cache. return False -def _collect_buildset( - requirements, # type: Iterable[InstallRequirement] +def _get_cache_dir( + req, # type: InstallRequirement wheel_cache, # type: WheelCache check_binary_allowed, # type: BinaryAllowedPredicate - need_wheel, # type: bool ): - # type: (...) -> List[Tuple[InstallRequirement, str]] - """Return the list of InstallRequirement that need to be built, - with the persistent or temporary cache directory where the built - wheel needs to be stored. + # type: (...) -> str + """Return the persistent or temporary cache directory where the built + wheel need to be stored. """ - buildset = [] cache_available = bool(wheel_cache.cache_dir) - for req in requirements: - if not should_build( - req, - need_wheel=need_wheel, - check_binary_allowed=check_binary_allowed, - ): - continue - if ( - cache_available and - should_cache(req, check_binary_allowed) - ): - cache_dir = wheel_cache.get_path_for_link(req.link) - else: - cache_dir = wheel_cache.get_ephem_path_for_link(req.link) - buildset.append((req, cache_dir)) - return buildset + if ( + cache_available and + _should_cache(req, check_binary_allowed) + ): + cache_dir = wheel_cache.get_path_for_link(req.link) + else: + cache_dir = wheel_cache.get_ephem_path_for_link(req.link) + return cache_dir def _always_true(_): @@ -250,81 +258,60 @@ def _clean_one_legacy(req, global_options): return False -class WheelBuilder(object): - """Build wheels from a RequirementSet.""" +def build( + requirements, # type: Iterable[InstallRequirement] + 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. - def __init__( - self, - preparer, # type: RequirementPreparer - ): - # type: (...) -> None - self.preparer = preparer + :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 - def build( - self, - requirements, # type: Iterable[InstallRequirement] - should_unpack, # type: bool - 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. + if not requirements: + return [], [] - :param should_unpack: If True, after building the wheel, unpack it - and replace the sdist with the unpacked version in preparation - for installation. - :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 + # Build the wheels. + logger.info( + 'Building wheels for collected packages: %s', + ', '.join(req.name for req in requirements), + ) - buildset = _collect_buildset( - requirements, - wheel_cache=wheel_cache, - check_binary_allowed=check_binary_allowed, - need_wheel=not should_unpack, - ) - if not buildset: - return [], [] + with indent_log(): + build_successes, build_failures = [], [] + for req in requirements: + cache_dir = _get_cache_dir( + req, wheel_cache, check_binary_allowed + ) + wheel_file = _build_one( + req, cache_dir, build_options, global_options + ) + if wheel_file: + # Update the link for this. + req.link = Link(path_to_url(wheel_file)) + req.local_file_path = req.link.file_path + assert req.link.is_wheel + build_successes.append(req) + else: + build_failures.append(req) - # TODO by @pradyunsg - # Should break up this method into 2 separate methods. - - # Build the wheels. + # notify success/failure + if build_successes: logger.info( - 'Building wheels for collected packages: %s', - ', '.join([req.name for (req, _) in buildset]), + 'Successfully built %s', + ' '.join([req.name for req in build_successes]), ) - - with indent_log(): - build_successes, build_failures = [], [] - for req, cache_dir in buildset: - wheel_file = _build_one( - req, cache_dir, build_options, global_options - ) - if wheel_file: - # Update the link for this. - req.link = Link(path_to_url(wheel_file)) - req.local_file_path = req.link.file_path - assert req.link.is_wheel - build_successes.append(req) - else: - build_failures.append(req) - - # notify success/failure - if build_successes: - logger.info( - 'Successfully built %s', - ' '.join([req.name for req in build_successes]), - ) - if build_failures: - logger.info( - 'Failed to build %s', - ' '.join([req.name for req in build_failures]), - ) - # Return a list of requirements that failed to build - return build_successes, build_failures + if build_failures: + logger.info( + 'Failed to build %s', + ' '.join([req.name for req in build_failures]), + ) + # Return a list of requirements that failed to build + return build_successes, build_failures diff --git a/tests/unit/test_command_install.py b/tests/unit/test_command_install.py index 36b3ca73f..aee03b77a 100644 --- a/tests/unit/test_command_install.py +++ b/tests/unit/test_command_install.py @@ -1,11 +1,10 @@ import errno import pytest -from mock import Mock, patch +from mock import patch from pip._vendor.packaging.requirements import Requirement from pip._internal.commands.install import ( - build_wheels, create_env_error_message, decide_user_install, warn_deprecated_install_options, @@ -14,68 +13,6 @@ from pip._internal.req.req_install import InstallRequirement from pip._internal.req.req_set import RequirementSet -class TestWheelCache: - - def check_build_wheels( - self, - pep517_requirements, - legacy_requirements, - ): - """ - Return: (mock_calls, return_value). - """ - built_reqs = [] - - def build(reqs, **kwargs): - # Fail the first requirement. - built_reqs.append(reqs) - return ([], [reqs[0]]) - - builder = Mock() - builder.build.side_effect = build - - build_failures = build_wheels( - builder=builder, - pep517_requirements=pep517_requirements, - legacy_requirements=legacy_requirements, - wheel_cache=Mock(cache_dir=None), - build_options=[], - global_options=[], - check_binary_allowed=None, - ) - - return (built_reqs, build_failures) - - @patch('pip._internal.commands.install.is_wheel_installed') - def test_build_wheels__wheel_installed(self, is_wheel_installed): - is_wheel_installed.return_value = True - - built_reqs, build_failures = self.check_build_wheels( - pep517_requirements=['a', 'b'], - legacy_requirements=['c', 'd'], - ) - - # Legacy requirements were built. - assert built_reqs == [['a', 'b'], ['c', 'd']] - - # Legacy build failures are not included in the return value. - assert build_failures == ['a'] - - @patch('pip._internal.commands.install.is_wheel_installed') - def test_build_wheels__wheel_not_installed(self, is_wheel_installed): - is_wheel_installed.return_value = False - - built_reqs, build_failures = self.check_build_wheels( - pep517_requirements=['a', 'b'], - legacy_requirements=['c', 'd'], - ) - - # Legacy requirements were not built. - assert built_reqs == [['a', 'b']] - - assert build_failures == ['a'] - - class TestDecideUserInstall: @patch('site.ENABLE_USER_SITE', True) @patch('pip._internal.commands.install.site_packages_writable') diff --git a/tests/unit/test_wheel_builder.py b/tests/unit/test_wheel_builder.py index 9d2c80303..7d70e8dcb 100644 --- a/tests/unit/test_wheel_builder.py +++ b/tests/unit/test_wheel_builder.py @@ -1,7 +1,7 @@ import logging import pytest -from mock import Mock +from mock import patch from pip._internal import wheel_builder from pip._internal.models.link import Link @@ -39,6 +39,7 @@ class ReqMock: link=None, constraint=False, source_dir="/tmp/pip-install-123/pendulum", + use_pep517=True, ): self.name = name self.is_wheel = is_wheel @@ -46,43 +47,71 @@ class ReqMock: self.link = link self.constraint = constraint self.source_dir = source_dir + self.use_pep517 = use_pep517 @pytest.mark.parametrize( - "req, need_wheel, disallow_binaries, expected", + "req, 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), + (ReqMock(), False, True), + (ReqMock(), True, False), + (ReqMock(constraint=True), False, False), + (ReqMock(is_wheel=True), False, False), + (ReqMock(editable=True), False, False), + (ReqMock(source_dir=None), 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), + (ReqMock(link=Link("git+https://g.c/org/repo")), False, True), # Disallowing binaries, however, should cause them not to be built. - (ReqMock(link=Link("git+https://g.c/org/repo")), False, True, False), + (ReqMock(link=Link("git+https://g.c/org/repo")), True, False), ], ) -def test_should_build(req, need_wheel, disallow_binaries, expected): - should_build = wheel_builder.should_build( +def test_should_build_for_install_command(req, disallow_binaries, expected): + should_build = wheel_builder.should_build_for_install_command( req, - need_wheel, check_binary_allowed=lambda req: not disallow_binaries, ) assert should_build is expected +@pytest.mark.parametrize( + "req, expected", + [ + (ReqMock(), True), + (ReqMock(constraint=True), False), + (ReqMock(is_wheel=True), False), + (ReqMock(editable=True), True), + (ReqMock(source_dir=None), True), + (ReqMock(link=Link("git+https://g.c/org/repo")), True), + ], +) +def test_should_build_for_wheel_command(req, expected): + should_build = wheel_builder.should_build_for_wheel_command(req) + assert should_build is expected + + +@patch("pip._internal.wheel_builder.is_wheel_installed") +def test_should_build_legacy_wheel_not_installed(is_wheel_installed): + is_wheel_installed.return_value = False + legacy_req = ReqMock(use_pep517=False) + should_build = wheel_builder.should_build_for_install_command( + legacy_req, + check_binary_allowed=lambda req: True, + ) + assert not should_build + + +@patch("pip._internal.wheel_builder.is_wheel_installed") +def test_should_build_legacy_wheel_installed(is_wheel_installed): + is_wheel_installed.return_value = True + legacy_req = ReqMock(use_pep517=False) + should_build = wheel_builder.should_build_for_install_command( + legacy_req, + check_binary_allowed=lambda req: True, + ) + assert should_build + + @pytest.mark.parametrize( "req, disallow_binaries, expected", [ @@ -104,13 +133,13 @@ def test_should_cache( def check_binary_allowed(req): return not disallow_binaries - should_cache = wheel_builder.should_cache( + should_cache = wheel_builder._should_cache( req, check_binary_allowed ) - if not wheel_builder.should_build( - req, need_wheel=False, check_binary_allowed=check_binary_allowed + if not wheel_builder.should_build_for_install_command( + req, check_binary_allowed=check_binary_allowed ): - # never cache if pip install (need_wheel=False) would not have built) + # never cache if pip install would not have built) assert not should_cache assert should_cache is expected @@ -124,14 +153,14 @@ 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( + assert wheel_builder._should_cache( req, check_binary_allowed=lambda r: True, ) # 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( + assert not wheel_builder._should_cache( req, check_binary_allowed=lambda r: True, ) @@ -181,23 +210,3 @@ def test_format_command_result__empty_output(caplog, log_level): "Command arguments: arg1 arg2", 'Command output: None', ] - - -class TestWheelBuilder(object): - - def test_skip_building_wheels(self, caplog): - wb = wheel_builder.WheelBuilder(preparer=Mock()) - wb._build_one = mock_build_one = Mock() - - wheel_req = Mock(is_wheel=True, editable=False, constraint=False) - with caplog.at_level(logging.INFO): - wb.build( - [wheel_req], - should_unpack=False, - wheel_cache=Mock(cache_dir=None), - build_options=[], - global_options=[], - ) - - assert "due to already being wheel" in caplog.text - assert mock_build_one.mock_calls == []