From f47837adfc2f12e1fb37c5bbc378ad06a0fbcb11 Mon Sep 17 00:00:00 2001 From: Claudio Matsuoka Date: Fri, 14 Feb 2025 08:43:15 -0300 Subject: [PATCH] fix(errors): reformat plugin build error output (#1012) In case of Python plugin build errors, print error messages to stderr instead of stdout so they can be captured and properly displayed to the user. Don't start printing from the last script execution trace line because messages printed to stderr appear before the traced exit line. Fixes #1000 Signed-off-by: Claudio Matsuoka claudio.matsuoka@canonical.com --- craft_parts/errors.py | 37 ++++++++++++----------- craft_parts/plugins/base.py | 8 ++--- docs/reference/changelog.rst | 8 ++++- pyproject.toml | 2 +- tests/integration/executor/test_errors.py | 1 + tests/integration/plugins/test_poetry.py | 26 +++++++++++++--- tests/integration/plugins/test_python.py | 26 +++++++++++++--- tests/unit/plugins/test_base.py | 14 ++++----- tests/unit/plugins/test_base_python.py | 8 ++--- tests/unit/utils/test_process.py | 4 +-- 10 files changed, 89 insertions(+), 45 deletions(-) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index 03c8b083b..781b04d2a 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -513,27 +513,28 @@ def __init__( def details(self) -> str | None: """Further details on the error. - Discards all trace lines that come before the last-executed script line + Displays the last three trace lines from the error output. """ - with contextlib.closing(StringIO()) as details_io: - if self.stderr is None: - return None - - stderr = self.stderr.decode("utf-8", errors="replace") - stderr_lines = stderr.split("\n") - # Find the final command captured in the logs - last_command = None - for idx, line in enumerate(reversed(stderr_lines)): - if line.startswith("+"): - last_command = len(stderr_lines) - idx - 1 + if self.stderr is None: + return None + + stderr = self.stderr.decode("utf-8", errors="replace") + stderr_lines = list(filter(lambda x: x, stderr.split("\n"))) + + # Find the third trace output line + anchor_line = 0 + traced_lines_to_display = 3 + count = 0 + for idx, line in enumerate(reversed(stderr_lines)): + if line.startswith("+"): + count += 1 + if count > traced_lines_to_display: + anchor_line = -idx break - else: - # Fallback to printing the whole log - last_command = 0 - for line in stderr_lines[last_command:]: - if line: - details_io.write(f"\n:: {line}") + with contextlib.closing(StringIO()) as details_io: + for line in stderr_lines[anchor_line:]: + details_io.write(f"\n:: {line}") return details_io.getvalue() diff --git a/craft_parts/plugins/base.py b/craft_parts/plugins/base.py index 02ab54913..948e0e8cd 100644 --- a/craft_parts/plugins/base.py +++ b/craft_parts/plugins/base.py @@ -1,6 +1,6 @@ # -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*- # -# Copyright 2020-2024 Canonical Ltd. +# Copyright 2020-2025 Canonical Ltd. # # This program is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -164,7 +164,7 @@ def _get_find_python_interpreter_commands(self) -> list[str]: # look for python3.10 basename=$(basename $(readlink -f ${{PARTS_PYTHON_VENV_INTERP_PATH}})) echo Looking for a Python interpreter called \\"${{basename}}\\" in the payload... - payload_python=$(find "$install_dir" "$stage_dir" -type f -executable -name "${{basename}}" -print -quit 2>/dev/null) + payload_python=$(find "$install_dir" "$stage_dir" -type f -executable -name "${{basename}}" -print -quit 2>/dev/null || true) if [ -n "$payload_python" ]; then # We found a provisioned interpreter, use it. @@ -179,12 +179,12 @@ def _get_find_python_interpreter_commands(self) -> list[str]: fi else # Otherwise use what _get_system_python_interpreter() told us. - echo "Python interpreter not found in payload." + echo "Python interpreter not found in payload." >&2 symlink_target="{python_interpreter}" fi if [ -z "$symlink_target" ]; then - echo "No suitable Python interpreter found, giving up." + echo "No suitable Python interpreter found, giving up." >&2 exit 1 fi diff --git a/docs/reference/changelog.rst b/docs/reference/changelog.rst index fc2c62f6f..bbe13f56c 100644 --- a/docs/reference/changelog.rst +++ b/docs/reference/changelog.rst @@ -2,6 +2,13 @@ Changelog ********* +X.Y.Z () +------------------ + +Bug fixes: + +- Fix handling and propagation of Python plugin error messages. + 2.6.1 (2025-02-12) ------------------ @@ -11,7 +18,6 @@ Bug fixes: - Fix Jdeps parameter ordering in the :ref:`jlink plugin`. - 2.3.1 (2025-02-07) ------------------ diff --git a/pyproject.toml b/pyproject.toml index 26f75cbfa..c0b3b0a23 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -75,7 +75,7 @@ dev = [ # TODO: Remove this once we've switched CI to uv # Testing "hypothesis", "jsonschema", - "pytest-check", + "pytest-check<2.5.0", "pytest-subprocess", "requests-mock", diff --git a/tests/integration/executor/test_errors.py b/tests/integration/executor/test_errors.py index 7f69e2235..29896c699 100644 --- a/tests/integration/executor/test_errors.py +++ b/tests/integration/executor/test_errors.py @@ -74,6 +74,7 @@ def test_plugin_build_errors(new_dir, partitions): """\ Failed to run the build script for part 'foo'. + :: + go mod download all :: + go install -p 1 ./... :: # example.com/hello :: ./hello.go:9:9: undefined: fmt.Printfs diff --git a/tests/integration/plugins/test_poetry.py b/tests/integration/plugins/test_poetry.py index 49998b515..310eddcde 100644 --- a/tests/integration/plugins/test_poetry.py +++ b/tests/integration/plugins/test_poetry.py @@ -241,21 +241,39 @@ def _get_system_python_interpreter(self) -> str | None: ) actions = lf.plan(Step.PRIME) + expected_error_text = textwrap.dedent( + """\ + + '[' -z '' ']' + + echo 'No suitable Python interpreter found, giving up.' + No suitable Python interpreter found, giving up. + + exit 1 + """ + ) + out = pathlib.Path("out.txt") - with out.open(mode="w") as outfile, pytest.raises(errors.PluginBuildError): + err = pathlib.Path("err.txt") + with ( + out.open(mode="w") as outfile, + err.open(mode="w") as errfile, + pytest.raises(errors.PluginBuildError) as exc_info, + ): with lf.action_executor() as ctx: - ctx.execute(actions, stdout=outfile) + ctx.execute(actions, stdout=outfile, stderr=errfile) + + assert exc_info.value.stderr is not None + assert expected_error_text in exc_info.value.stderr.decode() output = out.read_text() expected_text = textwrap.dedent( f"""\ Looking for a Python interpreter called "{real_basename}" in the payload... - Python interpreter not found in payload. - No suitable Python interpreter found, giving up. """ ) assert expected_text in output + output = err.read_text() + assert expected_error_text in output + def test_find_payload_python_good_version(new_dir, partitions, parts_dict, poetry_part): """Test that the build succeeds if a payload interpreter is needed, and it's diff --git a/tests/integration/plugins/test_python.py b/tests/integration/plugins/test_python.py index 48e17af34..705f41efb 100644 --- a/tests/integration/plugins/test_python.py +++ b/tests/integration/plugins/test_python.py @@ -321,21 +321,39 @@ def _get_system_python_interpreter(self) -> str | None: ) actions = lf.plan(Step.PRIME) + expected_error_text = textwrap.dedent( + """\ + + '[' -z '' ']' + + echo 'No suitable Python interpreter found, giving up.' + No suitable Python interpreter found, giving up. + + exit 1 + """ + ) + out = Path("out.txt") - with out.open(mode="w") as outfile, pytest.raises(errors.PluginBuildError): + err = Path("err.txt") + with ( + out.open(mode="w") as outfile, + err.open(mode="w") as errfile, + pytest.raises(errors.PluginBuildError) as exc_info, + ): with lf.action_executor() as ctx: - ctx.execute(actions, stdout=outfile) + ctx.execute(actions, stdout=outfile, stderr=errfile) + + assert exc_info.value.stderr is not None + assert expected_error_text in exc_info.value.stderr.decode() output = out.read_text() expected_text = textwrap.dedent( f"""\ Looking for a Python interpreter called "{real_basename}" in the payload... - Python interpreter not found in payload. - No suitable Python interpreter found, giving up. """ ) assert expected_text in output + output = err.read_text() + assert expected_error_text in output + def test_find_payload_python_good_version(new_dir, partitions): """Test that the build succeeds if a payload interpreter is needed, and it's diff --git a/tests/unit/plugins/test_base.py b/tests/unit/plugins/test_base.py index 3dacf4445..3c458c4e8 100644 --- a/tests/unit/plugins/test_base.py +++ b/tests/unit/plugins/test_base.py @@ -1,6 +1,6 @@ # -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*- # -# Copyright 2021-2023 Canonical Ltd. +# Copyright 2021-2025 Canonical Ltd. # # This program is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -146,7 +146,7 @@ def test_python_get_find_python_interpreter_commands( # look for python3.10 basename=$(basename $(readlink -f ${{PARTS_PYTHON_VENV_INTERP_PATH}})) echo Looking for a Python interpreter called \\"${{basename}}\\" in the payload... - payload_python=$(find "$install_dir" "$stage_dir" -type f -executable -name "${{basename}}" -print -quit 2>/dev/null) + payload_python=$(find "$install_dir" "$stage_dir" -type f -executable -name "${{basename}}" -print -quit 2>/dev/null || true) if [ -n "$payload_python" ]; then # We found a provisioned interpreter, use it. @@ -161,12 +161,12 @@ def test_python_get_find_python_interpreter_commands( fi else # Otherwise use what _get_system_python_interpreter() told us. - echo "Python interpreter not found in payload." + echo "Python interpreter not found in payload." >&2 symlink_target="$(readlink -f "$(which "${{PARTS_PYTHON_INTERPRETER}}")")" fi if [ -z "$symlink_target" ]; then - echo "No suitable Python interpreter found, giving up." + echo "No suitable Python interpreter found, giving up." >&2 exit 1 fi @@ -238,7 +238,7 @@ def test_python_get_build_commands(new_dir, python_plugin: FooPythonPlugin): # look for python3.10 basename=$(basename $(readlink -f ${{PARTS_PYTHON_VENV_INTERP_PATH}})) echo Looking for a Python interpreter called \\"${{basename}}\\" in the payload... - payload_python=$(find "$install_dir" "$stage_dir" -type f -executable -name "${{basename}}" -print -quit 2>/dev/null) + payload_python=$(find "$install_dir" "$stage_dir" -type f -executable -name "${{basename}}" -print -quit 2>/dev/null || true) if [ -n "$payload_python" ]; then # We found a provisioned interpreter, use it. @@ -253,12 +253,12 @@ def test_python_get_build_commands(new_dir, python_plugin: FooPythonPlugin): fi else # Otherwise use what _get_system_python_interpreter() told us. - echo "Python interpreter not found in payload." + echo "Python interpreter not found in payload." >&2 symlink_target="$(readlink -f "$(which "${{PARTS_PYTHON_INTERPRETER}}")")" fi if [ -z "$symlink_target" ]; then - echo "No suitable Python interpreter found, giving up." + echo "No suitable Python interpreter found, giving up." >&2 exit 1 fi diff --git a/tests/unit/plugins/test_base_python.py b/tests/unit/plugins/test_base_python.py index 8bc34e36c..3cae85673 100644 --- a/tests/unit/plugins/test_base_python.py +++ b/tests/unit/plugins/test_base_python.py @@ -1,6 +1,6 @@ # -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*- # -# Copyright 2024 Canonical Ltd. +# Copyright 2025 Canonical Ltd. # # This program is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -72,7 +72,7 @@ def get_python_build_commands( # look for python3.10 basename=$(basename $(readlink -f ${{PARTS_PYTHON_VENV_INTERP_PATH}})) echo Looking for a Python interpreter called \\"${{basename}}\\" in the payload... - payload_python=$(find "$install_dir" "$stage_dir" -type f -executable -name "${{basename}}" -print -quit 2>/dev/null) + payload_python=$(find "$install_dir" "$stage_dir" -type f -executable -name "${{basename}}" -print -quit 2>/dev/null || true) if [ -n "$payload_python" ]; then # We found a provisioned interpreter, use it. @@ -87,12 +87,12 @@ def get_python_build_commands( fi else # Otherwise use what _get_system_python_interpreter() told us. - echo "Python interpreter not found in payload." + echo "Python interpreter not found in payload." >&2 symlink_target="$(readlink -f "$(which "${{PARTS_PYTHON_INTERPRETER}}")")" fi if [ -z "$symlink_target" ]; then - echo "No suitable Python interpreter found, giving up." + echo "No suitable Python interpreter found, giving up." >&2 exit 1 fi diff --git a/tests/unit/utils/test_process.py b/tests/unit/utils/test_process.py index 84b29ad69..a02549403 100644 --- a/tests/unit/utils/test_process.py +++ b/tests/unit/utils/test_process.py @@ -33,7 +33,7 @@ @pytest.mark.parametrize(("out", "err"), _RUN_TEST_CASES) def test_run(out, err): - result = process.run(["/usr/bin/sh", "-c", f"echo {out};sleep 0;echo {err} >&2"]) + result = process.run(["/usr/bin/sh", "-c", f"echo {out};sleep 0.1;echo {err} >&2"]) assert result.returncode == 0 assert result.stdout == (out + "\n").encode() assert result.stderr == (err + "\n").encode() @@ -82,7 +82,7 @@ def read(conn: socket.socket, _mask: int) -> None: [ "/usr/bin/sh", "-c", - f"echo {out};sleep 0;echo {err} >&2; echo -n {out}|socat - UNIX-CONNECT:{new_dir}/test.socket", + f"echo {out};sleep 0.1;echo {err} >&2; echo -n {out}|socat - UNIX-CONNECT:{new_dir}/test.socket", ], selector=selector, )