Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abort immediately on metadata generation failure instead of backtracking #10722

Merged
merged 3 commits into from
Jan 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions news/10655.bugfix.rst
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 1 addition & 1 deletion src/pip/_internal/cli/cmdoptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."),
)

Expand Down
27 changes: 27 additions & 0 deletions src/pip/_internal/cli/req_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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

Expand Down
19 changes: 17 additions & 2 deletions src/pip/_internal/resolution/resolvelib/factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,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
Expand All @@ -107,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] = {}
Expand Down Expand Up @@ -190,7 +192,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,
Expand All @@ -199,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:
Expand All @@ -210,7 +219,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,
Expand All @@ -219,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:
Expand Down
2 changes: 2 additions & 0 deletions src/pip/_internal/resolution/resolvelib/resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__()
Expand All @@ -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
Expand Down
62 changes: 62 additions & 0 deletions tests/functional/test_new_resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -2308,3 +2308,65 @@ 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


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:
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")
20 changes: 17 additions & 3 deletions tests/lib/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions tests/unit/resolution_resolvelib/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)

Expand Down
1 change: 1 addition & 0 deletions tests/unit/resolution_resolvelib/test_resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down