From 8eeee223041a973642bb646d37ccfe0e6ff0a8f5 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Fri, 10 Dec 2021 15:54:09 +0000 Subject: [PATCH 1/3] Abort immediately on metadata generation failure instead of backtracking This behaviour is more forgiving when a source distribution cannot be installed (eg: due to missing build dependencies or platform incompatibility) and favours early eager failures instead of trying to ensure that a package is installed regardless of the amount of effort it takes. --- news/10655.bugfix.rst | 4 ++++ .../resolution/resolvelib/factory.py | 5 ++--- tests/functional/test_new_resolver.py | 19 ++++++++++++++++++ tests/lib/__init__.py | 20 ++++++++++++++++--- 4 files changed, 42 insertions(+), 6 deletions(-) create mode 100644 news/10655.bugfix.rst diff --git a/news/10655.bugfix.rst b/news/10655.bugfix.rst new file mode 100644 index 00000000000..5e867d8246a --- /dev/null +++ b/news/10655.bugfix.rst @@ -0,0 +1,4 @@ +Stop backtracking on build failures, by instead surfacing them to the +user and failing immediately. This behaviour is more forgiving when +a package cannot be built due to missing build dependencies or platform +incompatibility. diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 8c28d0014dc..20d779e880f 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -27,7 +27,6 @@ from pip._internal.exceptions import ( DistributionNotFound, InstallationError, - InstallationSubprocessError, MetadataInconsistent, UnsupportedPythonVersion, UnsupportedWheel, @@ -190,7 +189,7 @@ def _make_candidate_from_link( name=name, version=version, ) - except (InstallationSubprocessError, MetadataInconsistent) as e: + except MetadataInconsistent as e: logger.info( "Discarding [blue underline]%s[/]: [yellow]%s[reset]", link, @@ -210,7 +209,7 @@ def _make_candidate_from_link( name=name, version=version, ) - except (InstallationSubprocessError, MetadataInconsistent) as e: + except MetadataInconsistent as e: logger.info( "Discarding [blue underline]%s[/]: [yellow]%s[reset]", link, diff --git a/tests/functional/test_new_resolver.py b/tests/functional/test_new_resolver.py index b3f523788ea..5e64f1665c8 100644 --- a/tests/functional/test_new_resolver.py +++ b/tests/functional/test_new_resolver.py @@ -2308,3 +2308,22 @@ def test_new_resolver_respect_user_requested_if_extra_is_installed( "pkg2", ) script.assert_installed(pkg3="1.0", pkg2="2.0", pkg1="1.0") + + +def test_new_resolver_do_not_backtrack_on_build_failure( + script: PipTestEnvironment, +) -> None: + create_basic_sdist_for_package(script, "pkg1", "2.0", fails_egg_info=True) + create_basic_wheel_for_package(script, "pkg1", "1.0") + + result = script.pip( + "install", + "--no-cache-dir", + "--no-index", + "--find-links", + script.scratch_path, + "pkg1", + expect_error=True, + ) + + assert "egg_info" in result.stderr diff --git a/tests/lib/__init__.py b/tests/lib/__init__.py index 9c1b4f442a9..06849d2d705 100644 --- a/tests/lib/__init__.py +++ b/tests/lib/__init__.py @@ -1183,22 +1183,36 @@ def create_basic_sdist_for_package( name: str, version: str, extra_files: Optional[Dict[str, str]] = None, + *, + fails_egg_info: bool = False, + fails_bdist_wheel: bool = False, ) -> Path: files = { - "setup.py": """ + "setup.py": f"""\ + import sys from setuptools import find_packages, setup + + fails_bdist_wheel = {fails_bdist_wheel!r} + fails_egg_info = {fails_egg_info!r} + + if fails_egg_info and "egg_info" in sys.argv: + raise Exception("Simulated failure for generating metadata.") + + if fails_bdist_wheel and "bdist_wheel" in sys.argv: + raise Exception("Simulated failure for building a wheel.") + setup(name={name!r}, version={version!r}) """, } # Some useful shorthands - archive_name = "{name}-{version}.tar.gz".format(name=name, version=version) + archive_name = f"{name}-{version}.tar.gz" # Replace key-values with formatted values for key, value in list(files.items()): del files[key] key = key.format(name=name) - files[key] = textwrap.dedent(value).format(name=name, version=version).strip() + files[key] = textwrap.dedent(value) # Add new files after formatting if extra_files: From ca78aba4565d95b1f70039da205ea2f8ddfe9134 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Sun, 12 Dec 2021 08:18:28 +0000 Subject: [PATCH 2/3] Test that the resolver skips packages, as instructed by constraints --- tests/functional/test_new_resolver.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/functional/test_new_resolver.py b/tests/functional/test_new_resolver.py index 5e64f1665c8..1f04e4ab253 100644 --- a/tests/functional/test_new_resolver.py +++ b/tests/functional/test_new_resolver.py @@ -2327,3 +2327,26 @@ def test_new_resolver_do_not_backtrack_on_build_failure( ) assert "egg_info" in result.stderr + + +def test_new_resolver_works_when_failing_package_builds_are_disallowed( + script: PipTestEnvironment, +) -> None: + create_basic_wheel_for_package(script, "pkg2", "1.0", depends=["pkg1"]) + create_basic_sdist_for_package(script, "pkg1", "2.0", fails_egg_info=True) + create_basic_wheel_for_package(script, "pkg1", "1.0") + constraints_file = script.scratch_path / "constraints.txt" + constraints_file.write_text("pkg1 != 2.0") + + script.pip( + "install", + "--no-cache-dir", + "--no-index", + "--find-links", + script.scratch_path, + "-c", + constraints_file, + "pkg2", + ) + + script.assert_installed(pkg2="1.0", pkg1="1.0") From 9d0db8839ffc3c7750410f22a85bfc424a7a58f5 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Sun, 12 Dec 2021 13:00:51 +0000 Subject: [PATCH 3/3] Add `--use-deprecated=backtrack-on-build-failures` This serves as an opt-out from build failures causing the entire installation to abort. --- src/pip/_internal/cli/cmdoptions.py | 2 +- src/pip/_internal/cli/req_command.py | 27 +++++++++++++++++++ .../resolution/resolvelib/factory.py | 16 +++++++++++ .../resolution/resolvelib/resolver.py | 2 ++ tests/functional/test_new_resolver.py | 20 ++++++++++++++ tests/unit/resolution_resolvelib/conftest.py | 1 + .../resolution_resolvelib/test_resolver.py | 1 + 7 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/cli/cmdoptions.py b/src/pip/_internal/cli/cmdoptions.py index 6e43928f6ee..e9806fd79d0 100644 --- a/src/pip/_internal/cli/cmdoptions.py +++ b/src/pip/_internal/cli/cmdoptions.py @@ -964,7 +964,7 @@ def check_list_path_option(options: Values) -> None: metavar="feature", action="append", default=[], - choices=["legacy-resolver", "out-of-tree-build"], + choices=["legacy-resolver", "out-of-tree-build", "backtrack-on-build-failures"], help=("Enable deprecated functionality, that will be removed in the future."), ) diff --git a/src/pip/_internal/cli/req_command.py b/src/pip/_internal/cli/req_command.py index 177b3dbc24d..8dc00e32826 100644 --- a/src/pip/_internal/cli/req_command.py +++ b/src/pip/_internal/cli/req_command.py @@ -227,6 +227,31 @@ def determine_resolver_variant(options: Values) -> str: return "2020-resolver" + @staticmethod + def determine_build_failure_suppression(options: Values) -> bool: + """Determines whether build failures should be suppressed and backtracked on.""" + if "backtrack-on-build-failures" not in options.deprecated_features_enabled: + return False + + if "legacy-resolver" in options.deprecated_features_enabled: + raise CommandError("Cannot backtrack with legacy resolver.") + + deprecated( + reason=( + "Backtracking on build failures can mask issues related to how " + "a package generates metadata or builds a wheel. This flag will " + "be removed in pip 22.2." + ), + gone_in=None, + replacement=( + "avoiding known-bad versions by explicitly telling pip to ignore them " + "(either directly as requirements, or via a constraints file)" + ), + feature_flag=None, + issue=10655, + ) + return True + @classmethod def make_requirement_preparer( cls, @@ -323,6 +348,7 @@ def make_resolver( isolated=options.isolated_mode, use_pep517=use_pep517, ) + suppress_build_failures = cls.determine_build_failure_suppression(options) resolver_variant = cls.determine_resolver_variant(options) # The long import name and duplicated invocation is needed to convince # Mypy into correctly typechecking. Otherwise it would complain the @@ -342,6 +368,7 @@ def make_resolver( force_reinstall=force_reinstall, upgrade_strategy=upgrade_strategy, py_version_info=py_version_info, + suppress_build_failures=suppress_build_failures, ) import pip._internal.resolution.legacy.resolver diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py index 20d779e880f..5c4d3f7cd81 100644 --- a/src/pip/_internal/resolution/resolvelib/factory.py +++ b/src/pip/_internal/resolution/resolvelib/factory.py @@ -27,6 +27,7 @@ from pip._internal.exceptions import ( DistributionNotFound, InstallationError, + InstallationSubprocessError, MetadataInconsistent, UnsupportedPythonVersion, UnsupportedWheel, @@ -96,6 +97,7 @@ def __init__( force_reinstall: bool, ignore_installed: bool, ignore_requires_python: bool, + suppress_build_failures: bool, py_version_info: Optional[Tuple[int, ...]] = None, ) -> None: self._finder = finder @@ -106,6 +108,7 @@ def __init__( self._use_user_site = use_user_site self._force_reinstall = force_reinstall self._ignore_requires_python = ignore_requires_python + self._suppress_build_failures = suppress_build_failures self._build_failures: Cache[InstallationError] = {} self._link_candidate_cache: Cache[LinkCandidate] = {} @@ -198,6 +201,13 @@ def _make_candidate_from_link( ) self._build_failures[link] = e return None + except InstallationSubprocessError as e: + if not self._suppress_build_failures: + raise + logger.warning("Discarding %s due to build failure: %s", link, e) + self._build_failures[link] = e + return None + base: BaseCandidate = self._editable_candidate_cache[link] else: if link not in self._link_candidate_cache: @@ -218,6 +228,12 @@ def _make_candidate_from_link( ) self._build_failures[link] = e return None + except InstallationSubprocessError as e: + if not self._suppress_build_failures: + raise + logger.warning("Discarding %s due to build failure: %s", link, e) + self._build_failures[link] = e + return None base = self._link_candidate_cache[link] if not extras: diff --git a/src/pip/_internal/resolution/resolvelib/resolver.py b/src/pip/_internal/resolution/resolvelib/resolver.py index 8ee36d377d8..618f1e1aead 100644 --- a/src/pip/_internal/resolution/resolvelib/resolver.py +++ b/src/pip/_internal/resolution/resolvelib/resolver.py @@ -47,6 +47,7 @@ def __init__( ignore_requires_python: bool, force_reinstall: bool, upgrade_strategy: str, + suppress_build_failures: bool, py_version_info: Optional[Tuple[int, ...]] = None, ): super().__init__() @@ -61,6 +62,7 @@ def __init__( force_reinstall=force_reinstall, ignore_installed=ignore_installed, ignore_requires_python=ignore_requires_python, + suppress_build_failures=suppress_build_failures, py_version_info=py_version_info, ) self.ignore_dependencies = ignore_dependencies diff --git a/tests/functional/test_new_resolver.py b/tests/functional/test_new_resolver.py index 1f04e4ab253..21b9e0b7fa9 100644 --- a/tests/functional/test_new_resolver.py +++ b/tests/functional/test_new_resolver.py @@ -2329,6 +2329,26 @@ def test_new_resolver_do_not_backtrack_on_build_failure( assert "egg_info" in result.stderr +def test_new_resolver_flag_permits_backtracking_on_build_failure( + script: PipTestEnvironment, +) -> None: + create_basic_sdist_for_package(script, "pkg1", "2.0", fails_egg_info=True) + create_basic_wheel_for_package(script, "pkg1", "1.0") + + script.pip( + "install", + "--use-deprecated=backtrack-on-build-failures", + "--no-cache-dir", + "--no-index", + "--find-links", + script.scratch_path, + "pkg1", + allow_stderr_warning=True, + ) + + script.assert_installed(pkg1="1.0") + + def test_new_resolver_works_when_failing_package_builds_are_disallowed( script: PipTestEnvironment, ) -> None: diff --git a/tests/unit/resolution_resolvelib/conftest.py b/tests/unit/resolution_resolvelib/conftest.py index 8f3bba5016e..cfd440570e6 100644 --- a/tests/unit/resolution_resolvelib/conftest.py +++ b/tests/unit/resolution_resolvelib/conftest.py @@ -63,6 +63,7 @@ def factory(finder: PackageFinder, preparer: RequirementPreparer) -> Iterator[Fa force_reinstall=False, ignore_installed=False, ignore_requires_python=False, + suppress_build_failures=False, py_version_info=None, ) diff --git a/tests/unit/resolution_resolvelib/test_resolver.py b/tests/unit/resolution_resolvelib/test_resolver.py index 579195b55ea..d1af696b7a2 100644 --- a/tests/unit/resolution_resolvelib/test_resolver.py +++ b/tests/unit/resolution_resolvelib/test_resolver.py @@ -29,6 +29,7 @@ def resolver(preparer: RequirementPreparer, finder: PackageFinder) -> Resolver: ignore_requires_python=False, force_reinstall=False, upgrade_strategy="to-satisfy-only", + suppress_build_failures=False, ) return resolver