diff --git a/src/pip/_internal/cli/base_command.py b/src/pip/_internal/cli/base_command.py index 0afe7e75d46..5c41488ce3c 100644 --- a/src/pip/_internal/cli/base_command.py +++ b/src/pip/_internal/cli/base_command.py @@ -22,6 +22,7 @@ from pip._internal.exceptions import ( BadCommand, CommandError, + DiagnosticPipError, InstallationError, NetworkConnectionError, PreviousBuildDirError, @@ -169,6 +170,11 @@ def exc_logging_wrapper(*args: Any) -> int: logger.debug("Exception information:", exc_info=True) return PREVIOUS_BUILD_DIR_ERROR + except DiagnosticPipError as exc: + logger.critical(str(exc)) + logger.debug("Exception information:", exc_info=True) + + return ERROR except ( InstallationError, UninstallationError, diff --git a/src/pip/_internal/exceptions.py b/src/pip/_internal/exceptions.py index ef5bc75118b..ba8c1f32b4a 100644 --- a/src/pip/_internal/exceptions.py +++ b/src/pip/_internal/exceptions.py @@ -1,23 +1,102 @@ """Exceptions used throughout package""" import configparser +import re from itertools import chain, groupby, repeat -from typing import TYPE_CHECKING, Dict, List, Optional, Union +from typing import TYPE_CHECKING, Dict, Iterator, List, Optional, Union from pip._vendor.pkg_resources import Distribution from pip._vendor.requests.models import Request, Response if TYPE_CHECKING: from hashlib import _Hash + from typing import Literal from pip._internal.metadata import BaseDistribution from pip._internal.req.req_install import InstallRequirement +# +# Scaffolding +# +def _is_kebab_case(s: str) -> bool: + return re.match(r"^[a-z]+(-[a-z]+)*$", s) is not None + + +def _prefix_with_indent(prefix: str, s: str, indent: Optional[str] = None) -> str: + if indent is None: + indent = " " * len(prefix) + else: + assert len(indent) == len(prefix) + message = s.replace("\n", "\n" + indent) + return f"{prefix}{message}\n" + + class PipError(Exception): - """Base pip exception""" + """The base pip error.""" + +class DiagnosticPipError(PipError): + """A pip error, that presents diagnostic information to the user. + This contains a bunch of logic, to enable pretty presentation of our error + messages. Each error gets a unique reference. Each error can also include + additional context, a hint and/or a note -- which are presented with the + main error message in a consistent style. + """ + + reference: str + + def __init__( + self, + *, + message: str, + context: Optional[str], + hint_stmt: Optional[str], + attention_stmt: Optional[str] = None, + reference: Optional[str] = None, + kind: 'Literal["error", "warning"]' = "error", + ) -> None: + + # Ensure a proper reference is provided. + if reference is None: + assert hasattr(self, "reference"), "error reference not provided!" + reference = self.reference + assert _is_kebab_case(reference), "error reference must be kebab-case!" + + super().__init__(f"{reference}: {message}") + + self.kind = kind + self.message = message + self.context = context + + self.reference = reference + self.attention_stmt = attention_stmt + self.hint_stmt = hint_stmt + + def __str__(self) -> str: + return "".join(self._string_parts()) + + def _string_parts(self) -> Iterator[str]: + # Present the main message, with relevant context indented. + yield f"{self.message}\n" + if self.context is not None: + yield f"\n{self.context}\n" + + # Space out the note/hint messages. + if self.attention_stmt is not None or self.hint_stmt is not None: + yield "\n" + + if self.attention_stmt is not None: + yield _prefix_with_indent("Note: ", self.attention_stmt) + + if self.hint_stmt is not None: + yield _prefix_with_indent("Hint: ", self.hint_stmt) + + +# +# Actual Errors +# class ConfigurationError(PipError): """General exception in configuration""" @@ -30,6 +109,45 @@ class UninstallationError(PipError): """General exception during uninstallation""" +class MissingPyProjectBuildRequires(DiagnosticPipError): + """Raised when pyproject.toml has `build-system`, but no `build-system.requires`.""" + + reference = "missing-pyproject-build-system-requires" + + def __init__(self, *, package: str) -> None: + super().__init__( + message=f"Can not process {package}", + context=( + "This package has an invalid pyproject.toml file.\n" + "The [build-system] table is missing the mandatory `requires` key." + ), + attention_stmt=( + "This is an issue with the package mentioned above, not pip." + ), + hint_stmt="See PEP 518 for the detailed specification.", + ) + + +class InvalidPyProjectBuildRequires(DiagnosticPipError): + """Raised when pyproject.toml an invalid `build-system.requires`.""" + + reference = "invalid-pyproject-build-system-requires" + + def __init__(self, *, package: str, reason: str) -> None: + super().__init__( + message=f"Can not process {package}", + context=( + "This package has an invalid `build-system.requires` key in " + "pyproject.toml.\n" + f"{reason}" + ), + hint_stmt="See PEP 518 for the detailed specification.", + attention_stmt=( + "This is an issue with the package mentioned above, not pip." + ), + ) + + class NoneMetadataError(PipError): """ Raised when accessing "METADATA" or "PKG-INFO" metadata for a diff --git a/src/pip/_internal/pyproject.py b/src/pip/_internal/pyproject.py index 31534a3a9d3..0607b048bae 100644 --- a/src/pip/_internal/pyproject.py +++ b/src/pip/_internal/pyproject.py @@ -5,7 +5,11 @@ from pip._vendor import tomli from pip._vendor.packaging.requirements import InvalidRequirement, Requirement -from pip._internal.exceptions import InstallationError +from pip._internal.exceptions import ( + InstallationError, + InvalidPyProjectBuildRequires, + MissingPyProjectBuildRequires, +) def _is_list_of_str(obj: Any) -> bool: @@ -119,47 +123,28 @@ def load_pyproject_toml( # Ensure that the build-system section in pyproject.toml conforms # to PEP 518. - error_template = ( - "{package} has a pyproject.toml file that does not comply " - "with PEP 518: {reason}" - ) # Specifying the build-system table but not the requires key is invalid if "requires" not in build_system: - raise InstallationError( - error_template.format( - package=req_name, - reason=( - "it has a 'build-system' table but not " - "'build-system.requires' which is mandatory in the table" - ), - ) - ) + raise MissingPyProjectBuildRequires(package=req_name) # Error out if requires is not a list of strings requires = build_system["requires"] if not _is_list_of_str(requires): - raise InstallationError( - error_template.format( - package=req_name, - reason="'build-system.requires' is not a list of strings.", - ) + raise InvalidPyProjectBuildRequires( + package=req_name, + reason="It is not a list of strings.", ) # Each requirement must be valid as per PEP 508 for requirement in requires: try: Requirement(requirement) - except InvalidRequirement: - raise InstallationError( - error_template.format( - package=req_name, - reason=( - "'build-system.requires' contains an invalid " - "requirement: {!r}".format(requirement) - ), - ) - ) + except InvalidRequirement as error: + raise InvalidPyProjectBuildRequires( + package=req_name, + reason=f"It contains an invalid requirement: {requirement!r}", + ) from error backend = build_system.get("build-backend") backend_path = build_system.get("backend-path", []) diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index ac96b1f940e..a0b92be223f 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -108,7 +108,12 @@ def test_pep518_refuses_invalid_requires(script, data, common_wheels): expect_error=True, ) assert result.returncode == 1 - assert "does not comply with PEP 518" in result.stderr + + # Ensure the relevant things are mentioned. + assert "PEP 518" in result.stderr + assert "not a list of strings" in result.stderr + assert "build-system.requires" in result.stderr + assert "pyproject.toml" in result.stderr def test_pep518_refuses_invalid_build_system(script, data, common_wheels): @@ -120,7 +125,12 @@ def test_pep518_refuses_invalid_build_system(script, data, common_wheels): expect_error=True, ) assert result.returncode == 1 - assert "does not comply with PEP 518" in result.stderr + + # Ensure the relevant things are mentioned. + assert "PEP 518" in result.stderr + assert "mandatory `requires` key" in result.stderr + assert "[build-system] table" in result.stderr + assert "pyproject.toml" in result.stderr def test_pep518_allows_missing_requires(script, data, common_wheels): @@ -132,7 +142,7 @@ def test_pep518_allows_missing_requires(script, data, common_wheels): expect_stderr=True, ) # Make sure we don't warn when this occurs. - assert "does not comply with PEP 518" not in result.stderr + assert "PEP 518" not in result.stderr # We want it to go through isolation for now. assert "Installing build dependencies" in result.stdout, result.stdout diff --git a/tests/unit/test_exceptions.py b/tests/unit/test_exceptions.py new file mode 100644 index 00000000000..45e6bee10da --- /dev/null +++ b/tests/unit/test_exceptions.py @@ -0,0 +1,213 @@ +"""Tests the presentation style of exceptions.""" + +import textwrap + +import pytest + +from pip._internal.exceptions import DiagnosticPipError + + +class TestDiagnosticPipErrorCreation: + def test_fails_without_reference(self) -> None: + class DerivedError(DiagnosticPipError): + pass + + with pytest.raises(AssertionError) as exc_info: + DerivedError(message="", context=None, hint_stmt=None) + + assert str(exc_info.value) == "error reference not provided!" + + def test_can_fetch_reference_from_subclass(self) -> None: + class DerivedError(DiagnosticPipError): + reference = "subclass-reference" + + obj = DerivedError(message="", context=None, hint_stmt=None) + assert obj.reference == "subclass-reference" + + def test_can_fetch_reference_from_arguments(self) -> None: + class DerivedError(DiagnosticPipError): + pass + + obj = DerivedError( + message="", context=None, hint_stmt=None, reference="subclass-reference" + ) + assert obj.reference == "subclass-reference" + + @pytest.mark.parametrize( + "name", + [ + "BADNAME", + "BadName", + "bad_name", + "BAD_NAME", + "_bad", + "bad-name-", + "bad--name", + "-bad-name", + "bad-name-due-to-1-number", + ], + ) + def test_rejects_non_kebab_case_names(self, name: str) -> None: + class DerivedError(DiagnosticPipError): + reference = name + + with pytest.raises(AssertionError) as exc_info: + DerivedError(message="", context=None, hint_stmt=None) + + assert str(exc_info.value) == "error reference must be kebab-case!" + + +class TestDiagnosticPipErrorPresentation_ASCII: + def test_complete(self) -> None: + err = DiagnosticPipError( + reference="test-diagnostic", + message="Oh no!\nIt broke. :(", + context="Something went wrong\nvery wrong.", + attention_stmt="You did something wrong, which is what caused this error.", + hint_stmt="Do it better next time, by trying harder.", + ) + + assert str(err) == textwrap.dedent( + """\ + Oh no! + It broke. :( + + Something went wrong + very wrong. + + Note: You did something wrong, which is what caused this error. + Hint: Do it better next time, by trying harder. + """ + ) + + def test_no_context(self) -> None: + err = DiagnosticPipError( + reference="test-diagnostic", + message="Oh no!\nIt broke. :(", + context=None, + attention_stmt="You did something wrong, which is what caused this error.", + hint_stmt="Do it better next time, by trying harder.", + ) + + assert str(err) == textwrap.dedent( + """\ + Oh no! + It broke. :( + + Note: You did something wrong, which is what caused this error. + Hint: Do it better next time, by trying harder. + """ + ) + + def test_no_note(self) -> None: + err = DiagnosticPipError( + reference="test-diagnostic", + message="Oh no!\nIt broke. :(", + context="Something went wrong\nvery wrong.", + attention_stmt=None, + hint_stmt="Do it better next time, by trying harder.", + ) + + assert str(err) == textwrap.dedent( + """\ + Oh no! + It broke. :( + + Something went wrong + very wrong. + + Hint: Do it better next time, by trying harder. + """ + ) + + def test_no_hint(self) -> None: + err = DiagnosticPipError( + reference="test-diagnostic", + message="Oh no!\nIt broke. :(", + context="Something went wrong\nvery wrong.", + attention_stmt="You did something wrong, which is what caused this error.", + hint_stmt=None, + ) + + assert str(err) == textwrap.dedent( + """\ + Oh no! + It broke. :( + + Something went wrong + very wrong. + + Note: You did something wrong, which is what caused this error. + """ + ) + + def test_no_context_no_hint(self) -> None: + err = DiagnosticPipError( + reference="test-diagnostic", + message="Oh no!\nIt broke. :(", + context=None, + attention_stmt="You did something wrong, which is what caused this error.", + hint_stmt=None, + ) + + assert str(err) == textwrap.dedent( + """\ + Oh no! + It broke. :( + + Note: You did something wrong, which is what caused this error. + """ + ) + + def test_no_context_no_note(self) -> None: + err = DiagnosticPipError( + reference="test-diagnostic", + message="Oh no!\nIt broke. :(", + context=None, + attention_stmt=None, + hint_stmt="Do it better next time, by trying harder.", + ) + + assert str(err) == textwrap.dedent( + """\ + Oh no! + It broke. :( + + Hint: Do it better next time, by trying harder. + """ + ) + + def test_no_hint_no_note(self) -> None: + err = DiagnosticPipError( + reference="test-diagnostic", + message="Oh no!\nIt broke. :(", + context="Something went wrong\nvery wrong.", + attention_stmt=None, + hint_stmt=None, + ) + + assert str(err) == textwrap.dedent( + """\ + Oh no! + It broke. :( + + Something went wrong + very wrong. + """ + ) + + def test_no_hint_no_note_no_context(self) -> None: + err = DiagnosticPipError( + reference="test-diagnostic", + message="Oh no!\nIt broke. :(", + context=None, + hint_stmt=None, + attention_stmt=None, + ) + + assert str(err) == textwrap.dedent( + """\ + Oh no! + It broke. :( + """ + ) diff --git a/tests/unit/test_pep517.py b/tests/unit/test_pep517.py index 6c11ab625ec..9305cf2a19b 100644 --- a/tests/unit/test_pep517.py +++ b/tests/unit/test_pep517.py @@ -2,7 +2,7 @@ import pytest -from pip._internal.exceptions import InstallationError +from pip._internal.exceptions import InstallationError, InvalidPyProjectBuildRequires from pip._internal.req import InstallRequirement from tests.lib import TestData from tests.lib.path import Path @@ -75,20 +75,24 @@ def test_disabling_pep517_invalid(shared_data: TestData, source: str, msg: str) def test_pep517_parsing_checks_requirements(tmpdir: Path, spec: str) -> None: tmpdir.joinpath("pyproject.toml").write_text( dedent( + f""" + [build-system] + requires = [{spec!r}] + build-backend = "foo" """ - [build-system] - requires = [{!r}] - build-backend = "foo" - """.format( - spec - ) ) ) req = InstallRequirement(None, None) req.source_dir = tmpdir # make req believe it has been unpacked - with pytest.raises(InstallationError) as e: + with pytest.raises(InvalidPyProjectBuildRequires) as e: req.load_pyproject_toml() - err_msg = e.value.args[0] - assert "contains an invalid requirement" in err_msg + error = e.value + + assert str(req) in error.message + assert error.context + assert "build-system.requires" in error.context + assert "contains an invalid requirement" in error.context + assert error.hint_stmt + assert "PEP 518" in error.hint_stmt