From 2a3e950c1b9d17f1363e13e5d3e17e76dc3f5729 Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Thu, 26 Aug 2021 17:14:27 -0700 Subject: [PATCH] [internal] Improve the error message for stale/invalid lockfiles (#12618) Refactors the `build_pex` rule to only have the code for erroring/warning on invalid lockfiles in the one place, and updates the corresponding exception and log messages. I'm not 100% sold on the contents of the log message, but I'm happy enough for now. Here's a sample warning: > ```13:09:40.67 [WARN] Invalid lockfile for PEX request `black.pex`. If your requirements or interpreter constraints have changed, follow the instructions in the header of the lockfile to regenerate it. Otherwise, ensure your interpreter constraints are compatible with the constraints specified in the lockfile.``` Closes #12541 [ci skip-rust] [ci skip-build-wheels] --- .../experimental/python/lockfile_metadata.py | 43 +++++++--- .../python/lockfile_metadata_test.py | 10 ++- .../pants/backend/python/util_rules/pex.py | 79 ++++++++++++++----- .../backend/python/util_rules/pex_test.py | 78 ++++++++++++++---- 4 files changed, 161 insertions(+), 49 deletions(-) diff --git a/src/python/pants/backend/experimental/python/lockfile_metadata.py b/src/python/pants/backend/experimental/python/lockfile_metadata.py index d101afe3368..c08f9446f4d 100644 --- a/src/python/pants/backend/experimental/python/lockfile_metadata.py +++ b/src/python/pants/backend/experimental/python/lockfile_metadata.py @@ -6,6 +6,7 @@ import hashlib import json from dataclasses import dataclass +from enum import Enum from typing import Any, Iterable from pants.backend.python.util_rules.interpreter_constraints import InterpreterConstraints @@ -92,16 +93,23 @@ def is_valid_for( expected_invalidation_digest: str | None, user_interpreter_constraints: InterpreterConstraints, interpreter_universe: Iterable[str], - ) -> bool: - """Returns True if the lockfile can be used in the current execution context.""" + ) -> LockfileMetadataValidation: + """Returns Truthy if this `LockfileMetadata` can be used in the current execution + context.""" + failure_reasons: set[InvalidLockfileReason] = set() + if expected_invalidation_digest is None: - return True - return ( - self.requirements_invalidation_digest == expected_invalidation_digest - and self.valid_for_interpreter_constraints.contains( - user_interpreter_constraints, interpreter_universe - ) - ) + return LockfileMetadataValidation(failure_reasons) + + if self.requirements_invalidation_digest != expected_invalidation_digest: + failure_reasons.add(InvalidLockfileReason.INVALIDATION_DIGEST_MISMATCH) + + if not self.valid_for_interpreter_constraints.contains( + user_interpreter_constraints, interpreter_universe + ): + failure_reasons.add(InvalidLockfileReason.INTERPRETER_CONSTRAINTS_MISMATCH) + + return LockfileMetadataValidation(failure_reasons) def calculate_invalidation_digest(requirements: Iterable[str]) -> str: @@ -114,3 +122,20 @@ def calculate_invalidation_digest(requirements: Iterable[str]) -> str: } m.update(json.dumps(inputs).encode("utf-8")) return m.hexdigest() + + +class InvalidLockfileReason(Enum): + INVALIDATION_DIGEST_MISMATCH = "invalidation_digest_mismatch" + INTERPRETER_CONSTRAINTS_MISMATCH = "interpreter_constraints_mismatch" + + +class LockfileMetadataValidation: + """Boolean-like value which additionally carries reasons why a validation failed.""" + + failure_reasons: set[InvalidLockfileReason] + + def __init__(self, failure_reasons: Iterable[InvalidLockfileReason] = ()): + self.failure_reasons = set(failure_reasons) + + def __bool__(self): + return not self.failure_reasons diff --git a/src/python/pants/backend/experimental/python/lockfile_metadata_test.py b/src/python/pants/backend/experimental/python/lockfile_metadata_test.py index 4d1a8f74c49..c3404165e7e 100644 --- a/src/python/pants/backend/experimental/python/lockfile_metadata_test.py +++ b/src/python/pants/backend/experimental/python/lockfile_metadata_test.py @@ -124,10 +124,12 @@ def assert_neq(left: Iterable[str], right: Iterable[str]) -> None: def test_is_valid_for(user_digest, expected_digest, user_ic, expected_ic, matches) -> None: m = LockfileMetadata(expected_digest, InterpreterConstraints(expected_ic)) assert ( - m.is_valid_for( - user_digest, - InterpreterConstraints(user_ic), - ["2.7", "3.5", "3.6", "3.7", "3.8", "3.9", "3.10"], + bool( + m.is_valid_for( + user_digest, + InterpreterConstraints(user_ic), + ["2.7", "3.5", "3.6", "3.7", "3.8", "3.9", "3.10"], + ) ) == matches ) diff --git a/src/python/pants/backend/python/util_rules/pex.py b/src/python/pants/backend/python/util_rules/pex.py index 1987747193c..48d9e12f055 100644 --- a/src/python/pants/backend/python/util_rules/pex.py +++ b/src/python/pants/backend/python/util_rules/pex.py @@ -17,7 +17,10 @@ import packaging.version from pkg_resources import Requirement -from pants.backend.experimental.python.lockfile_metadata import LockfileMetadata +from pants.backend.experimental.python.lockfile_metadata import ( + InvalidLockfileReason, + LockfileMetadata, +) from pants.backend.python.target_types import MainSpecification from pants.backend.python.target_types import PexPlatformsField as PythonPlatformsField from pants.backend.python.target_types import PythonRequirementsField @@ -413,15 +416,7 @@ async def build_pex( requirements_file_digest_contents = await Get(DigestContents, PathGlobs, globs) metadata = LockfileMetadata.from_lockfile(requirements_file_digest_contents[0].content) - if not metadata.is_valid_for( - request.requirements.lockfile_hex_digest, - request.interpreter_constraints, - python_setup.interpreter_universe, - ): - if python_setup.invalid_lockfile_behavior == InvalidLockfileBehavior.error: - raise ValueError("Invalid lockfile provided. [TODO(#12314): Improve message]") - elif python_setup.invalid_lockfile_behavior == InvalidLockfileBehavior.warn: - logger.warning("%s", "Invalid lockfile provided. [TODO(#12314): Improve message]") + _validate_metadata(metadata, request, request.requirements, python_setup) requirements_file_digest = await Get(Digest, PathGlobs, globs) @@ -430,15 +425,7 @@ async def build_pex( argv.extend(["--requirement", file_content.path]) metadata = LockfileMetadata.from_lockfile(file_content.content) - if not metadata.is_valid_for( - request.requirements.lockfile_hex_digest, - request.interpreter_constraints, - python_setup.interpreter_universe, - ): - if python_setup.invalid_lockfile_behavior == InvalidLockfileBehavior.error: - raise ValueError("Invalid lockfile provided. [TODO(#12314): Improve message]") - elif python_setup.invalid_lockfile_behavior == InvalidLockfileBehavior.warn: - logger.warning("%s", "Invalid lockfile provided. [TODO(#12314): Improve message]") + _validate_metadata(metadata, request, request.requirements, python_setup) requirements_file_digest = await Get(Digest, CreateDigest([file_content])) else: @@ -492,6 +479,60 @@ async def build_pex( ) +def _validate_metadata( + metadata: LockfileMetadata, + request: PexRequest, + requirements: (Lockfile | LockfileContent), + python_setup: PythonSetup, +) -> None: + + validation = metadata.is_valid_for( + requirements.lockfile_hex_digest, + request.interpreter_constraints, + python_setup.interpreter_universe, + ) + + if validation: + return + + message_parts = [ + f"Invalid lockfile for PEX request `{request.output_filename}`.", + "\n", + ] + + if InvalidLockfileReason.INVALIDATION_DIGEST_MISMATCH in validation.failure_reasons: + message_parts.append( + "The requirements set for this PEX request are different to the requirements set when " + "the lockfile was generated. To fix this, you will need to regenerate the lockfile. " + ) + message_parts.append( + f"(Expected requirements digest: {requirements.lockfile_hex_digest}, " + "actual: {metadata.requirements_invalidation_digest})" + ) + message_parts.append("\n") + + if InvalidLockfileReason.INTERPRETER_CONSTRAINTS_MISMATCH in validation.failure_reasons: + message_parts.append( + "The lockfile was generated under interpreter constraints " + f"({ metadata.valid_for_interpreter_constraints }) that are incompatible with the " + f"constraints set in the project ({request.interpreter_constraints }). If you have " + "overridden your project's interpreter constraints, you can update them to specify a " + "subset of the interpreters specified in the lockfile. If not, you will need to " + "regenerate your lockfile. " + ) + + message_parts.append( + "To regenerate the lockfile, follow the instructions in the header of the lockfile." + ) + + message = "\n".join(message_parts).strip() + + if python_setup.invalid_lockfile_behavior == InvalidLockfileBehavior.error: + raise ValueError(message) + elif python_setup.invalid_lockfile_behavior == InvalidLockfileBehavior.warn: + logger.warning("%s", message) + + def _build_pex_description(request: PexRequest) -> str: if request.description: return request.description diff --git a/src/python/pants/backend/python/util_rules/pex_test.py b/src/python/pants/backend/python/util_rules/pex_test.py index 3270164b29a..c6e3cef224e 100644 --- a/src/python/pants/backend/python/util_rules/pex_test.py +++ b/src/python/pants/backend/python/util_rules/pex_test.py @@ -533,69 +533,113 @@ def assert_description( def test_error_on_invalid_lockfile_with_path(rule_runner: RuleRunner) -> None: with pytest.raises(ExecutionError): _run_pex_for_lockfile_test( - rule_runner, True, actual="1bad", expected="900d", behavior="error" + rule_runner, + use_file=True, + behavior="error", + invalid_reqs=True, ) def test_warn_on_invalid_lockfile_with_path(rule_runner: RuleRunner, caplog) -> None: - _run_pex_for_lockfile_test(rule_runner, True, actual="1bad", expected="900d", behavior="warn") - assert "Invalid lockfile provided." in caplog.text + _run_pex_for_lockfile_test(rule_runner, use_file=True, behavior="warn", invalid_reqs=True) + assert "Invalid lockfile for PEX request" in caplog.text + + +def test_warn_on_requirements_mismatch(rule_runner: RuleRunner, caplog) -> None: + _run_pex_for_lockfile_test(rule_runner, use_file=True, behavior="warn", invalid_reqs=True) + assert "requirements set for this" in caplog.text + assert "generated under interpreter constraints" not in caplog.text + + +def test_warn_on_interpreter_constraints_mismatch(rule_runner: RuleRunner, caplog) -> None: + _run_pex_for_lockfile_test( + rule_runner, use_file=True, behavior="warn", invalid_constraints=True + ) + assert "requirements set for this" not in caplog.text + assert "generated under interpreter constraints" in caplog.text + + +def test_warn_on_mismatched_requirements_and_interpreter_constraints( + rule_runner: RuleRunner, caplog +) -> None: + _run_pex_for_lockfile_test( + rule_runner, use_file=True, behavior="warn", invalid_reqs=True, invalid_constraints=True + ) + assert "requirements set for this" in caplog.text + assert "generated under interpreter constraints" in caplog.text def test_ignore_on_invalid_lockfile_with_path(rule_runner: RuleRunner, caplog) -> None: - _run_pex_for_lockfile_test(rule_runner, True, actual="1bad", expected="900d", behavior="ignore") + _run_pex_for_lockfile_test(rule_runner, use_file=True, behavior="ignore", invalid_reqs=True) assert not caplog.text.strip() def test_no_warning_on_valid_lockfile_with_path(rule_runner: RuleRunner, caplog) -> None: - _run_pex_for_lockfile_test(rule_runner, True, actual="900d", expected="900d", behavior="warn") + _run_pex_for_lockfile_test(rule_runner, use_file=True, behavior="warn") assert not caplog.text.strip() def test_error_on_invalid_lockfile_with_content(rule_runner: RuleRunner) -> None: with pytest.raises(ExecutionError): - _run_pex_for_lockfile_test( - rule_runner, False, actual="1bad", expected="900d", behavior="error" - ) + _run_pex_for_lockfile_test(rule_runner, use_file=False, behavior="error", invalid_reqs=True) def test_warn_on_invalid_lockfile_with_content(rule_runner: RuleRunner, caplog) -> None: - _run_pex_for_lockfile_test(rule_runner, False, actual="1bad", expected="900d", behavior="warn") - assert "Invalid lockfile provided." in caplog.text + _run_pex_for_lockfile_test(rule_runner, use_file=False, behavior="warn", invalid_reqs=True) + assert "Invalid lockfile for PEX request" in caplog.text def test_no_warning_on_valid_lockfile_with_content(rule_runner: RuleRunner, caplog) -> None: - _run_pex_for_lockfile_test(rule_runner, False, actual="900d", expected="900d", behavior="warn") + _run_pex_for_lockfile_test(rule_runner, use_file=False, behavior="warn") assert not caplog.text.strip() -def _run_pex_for_lockfile_test(rule_runner, use_file, actual, expected, behavior): - constraints = "CPython>=3.9" +def _run_pex_for_lockfile_test( + rule_runner, + *, + use_file, + behavior, + invalid_reqs=False, + invalid_constraints=False, +) -> None: + actual_digest = "900d" + expected_digest = actual_digest + if invalid_reqs: + expected_digest = "baad" + + actual_constraints = "CPython>=3.6,<3.10" + expected_constraints = actual_constraints + if invalid_constraints: + expected_constraints = "CPython>=3.9" + lockfile = f""" # --- BEGIN PANTS LOCKFILE METADATA: DO NOT EDIT OR REMOVE --- # {{ -# "requirements_invalidation_digest": "{actual}", +# "requirements_invalidation_digest": "{actual_digest}", # "valid_for_interpreter_constraints": [ -# "{ constraints }" +# "{ actual_constraints }" # ] # }} # --- END PANTS LOCKFILE METADATA --- ansicolors==1.1.8 """ + requirements: Lockfile | LockfileContent + if use_file: file_path = "lockfile.txt" rule_runner.write_files({file_path: lockfile}) requirements = Lockfile( file_path=file_path, file_path_description_of_origin="iceland", - lockfile_hex_digest=expected, + lockfile_hex_digest=expected_digest, ) else: content = FileContent("lockfile.txt", lockfile.encode("utf-8")) - requirements = LockfileContent(file_content=content, lockfile_hex_digest=expected) + requirements = LockfileContent(file_content=content, lockfile_hex_digest=expected_digest) create_pex_and_get_all_data( rule_runner, + interpreter_constraints=InterpreterConstraints([expected_constraints]), requirements=requirements, additional_pants_args=( "--python-setup-experimental-lockfile=lockfile.txt",