Skip to content

Commit

Permalink
feat: split stderr for better error presentation (#897)
Browse files Browse the repository at this point in the history
  • Loading branch information
bepri authored Nov 27, 2024
1 parent 6e687a9 commit 5bbd983
Show file tree
Hide file tree
Showing 10 changed files with 398 additions and 22 deletions.
65 changes: 58 additions & 7 deletions craft_parts/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,18 @@

"""Craft parts errors."""

import dataclasses
import contextlib
import pathlib
from collections.abc import Iterable
from io import StringIO
from typing import TYPE_CHECKING

from overrides import override

if TYPE_CHECKING:
from pydantic.error_wrappers import ErrorDict, Loc


@dataclasses.dataclass(repr=True)
class PartsError(Exception):
"""Unexpected error.
Expand All @@ -37,10 +39,17 @@ class PartsError(Exception):
the Craft Parts documentation.
"""

brief: str
details: str | None = None
resolution: str | None = None
doc_slug: str | None = None
def __init__(
self,
brief: str,
details: str | None = None,
resolution: str | None = None,
doc_slug: str | None = None,
) -> None:
self.brief = brief
self._details = details
self.resolution = resolution
self.doc_slug = doc_slug

def __str__(self) -> str:
components = [self.brief]
Expand All @@ -53,6 +62,14 @@ def __str__(self) -> str:

return "\n".join(components)

def __repr__(self) -> str:
return f"{self.__class__.__name__}(brief={self.brief!r}, details={self.details!r}, resolution={self.resolution!r}, doc_slug={self.doc_slug!r})"

@property
def details(self) -> str | None:
"""Further details on the error."""
return self._details


class FeatureError(PartsError):
"""A feature is not configured as expected."""
Expand Down Expand Up @@ -482,15 +499,49 @@ class PluginBuildError(PartsError):
:param plugin_name: The name of the plugin being processed.
"""

def __init__(self, *, part_name: str, plugin_name: str) -> None:
def __init__(
self, *, part_name: str, plugin_name: str, stderr: bytes | None = None
) -> None:
self.part_name = part_name
self.plugin_name = plugin_name
self.stderr = stderr
brief = f"Failed to run the build script for part {part_name!r}."
resolution = f"Check the build output and verify the project can work with the {plugin_name!r} plugin."
super().__init__(
brief=brief, resolution=resolution, doc_slug="/reference/plugins.html"
)

@property
@override
def details(self) -> str | None:
"""Further details on the error.
Discards all trace lines that come before the last-executed script line
"""
with contextlib.closing(StringIO()) as details_io:
if self.stderr is None:
return None

stderr = self.stderr.decode("utf-8", errors="replace")
details_io.write("\nCaptured standard error:")

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
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}")

return details_io.getvalue()


class PluginCleanError(PartsError):
"""Script to clean strict build preparation failed at runtime.
Expand Down
22 changes: 9 additions & 13 deletions craft_parts/executor/step_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
from craft_parts.plugins import Plugin
from craft_parts.sources.local_source import SourceHandler
from craft_parts.steps import Step
from craft_parts.utils import file_utils
from craft_parts.utils import file_utils, process

from . import filesets
from .filesets import Fileset
Expand Down Expand Up @@ -122,9 +122,9 @@ def _builtin_pull(self) -> StepContents:
stdout=self._stdout,
stderr=self._stderr,
)
except subprocess.CalledProcessError as process_error:
raise errors.PluginPullError(
part_name=self._part.name
except process.ProcessError as process_error:
raise (
errors.PluginPullError(part_name=self._part.name)
) from process_error

return StepContents()
Expand Down Expand Up @@ -153,9 +153,11 @@ def _builtin_build(self) -> StepContents:
stdout=self._stdout,
stderr=self._stderr,
)
except subprocess.CalledProcessError as process_error:
except process.ProcessError as process_error:
raise errors.PluginBuildError(
part_name=self._part.name, plugin_name=self._part.plugin_name
part_name=self._part.name,
plugin_name=self._part.plugin_name,
stderr=process_error.result.stderr,
) from process_error

return StepContents()
Expand Down Expand Up @@ -458,10 +460,4 @@ def _create_and_run_script(
script_path.chmod(0o755)
logger.debug("Executing %r", script_path)

subprocess.run(
[script_path],
cwd=cwd,
check=True,
stdout=stdout,
stderr=stderr,
)
process.run([script_path], cwd=cwd, stdout=stdout, stderr=stderr, check=True)
221 changes: 221 additions & 0 deletions craft_parts/utils/process.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*-
#
# Copyright 2024 Canonical Ltd.
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU Lesser General Public
# License version 3 as published by the Free Software Foundation.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
# Lesser General Public License for more details.
#
# You should have received a copy of the GNU Lesser General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.

"""Utilities for executing subprocesses and handling their stdout and stderr streams."""

import os
import selectors
import subprocess
import sys
from collections.abc import Generator, Sequence
from contextlib import closing, contextmanager
from dataclasses import dataclass
from io import BytesIO
from pathlib import Path
from typing import IO, TextIO

Command = str | Path | Sequence[str | Path]
Stream = TextIO | int | None

_BUF_SIZE = 4096

# Compatibility with subprocess.DEVNULL
DEVNULL = subprocess.DEVNULL


@dataclass
class ProcessResult:
"""Describes the outcome of a process."""

returncode: int
stdout: bytes
stderr: bytes
combined: bytes
command: Command

def check_returncode(self) -> None:
"""Raise an exception if the process returned non-zero."""
if self.returncode != 0:
raise ProcessError(self)


class _ProcessStream:
def __init__(self, read_fd: int, write_fd: int) -> None:
self.read_fd = read_fd
self.write_fd = write_fd

self._linebuf = bytearray()
self._streambuf = b""

# (Mostly) optimize away the process function if the read handle is empty
if self.read_fd == DEVNULL:
self.process = self._process_nothing # type: ignore[method-assign]

@property
def singular(self) -> bytes:
return self._streambuf

def process(self) -> bytes:
"""Forward any data from ``self.read_fd`` to ``self.write_fd`` and return a copy of it.
Does nothing if ``read_fd`` is DEVNULL.
"""
data = os.read(self.read_fd, _BUF_SIZE)
i = data.rfind(b"\n")
if i >= 0:
self._linebuf.extend(data[: i + 1])
self._streambuf += self._linebuf
os.write(self.write_fd, self._linebuf)
self._linebuf.clear()
self._linebuf.extend(data[i + 1 :])
return data
self._linebuf.extend(data)
return b""

def _process_nothing(self) -> bytes:
"""Do nothing."""
return b""


def run(
command: Command,
*,
cwd: Path | None = None,
stdout: Stream = None,
stderr: Stream = None,
check: bool = True,
) -> ProcessResult:
"""Execute a subprocess and collect its output.
This function collects the stdout and stderr streams as separate
accounts and a singular, combined account.
:param command: Command to execute.
:param cwd: Path to execute in.
:param stdout: Handle to a fd or I/O stream to treat as stdout. None defaults
to ``sys.stdout``, and process.DEVNULL can be passed for no printing or
stream capturing.
:param stderr: Handle to a fd or I/O stream to treat as stderr. None defaults
to ``sys.stderr``, and process.DEVNULL can be passed for no printing or
stream capturing.
:param check: If True, a ProcessError exception will be raised if ``command``
returns a non-zero return code.
:raises ProcessError: If process exits with a non-zero return code.
:raises OSError: If the specified executable is not found.
:return: A description of the process' outcome.
:rtype: ProcessResult
"""
# Optimized base case - no redirection at all
if stdout == DEVNULL and stderr == DEVNULL:
result_sp = subprocess.run(
command, stdout=DEVNULL, stderr=DEVNULL, cwd=cwd, check=False
)
result = ProcessResult(result_sp.returncode, b"", b"", b"", command)
if check:
result.check_returncode()
return result

proc = subprocess.Popen(
command,
stdout=DEVNULL if stdout == DEVNULL else subprocess.PIPE,
stderr=DEVNULL if stderr == DEVNULL else subprocess.PIPE,
cwd=cwd,
)

with (
_select_stream(stdout, sys.stdout) as out_fd,
_select_stream(stderr, sys.stderr) as err_fd,
):
# Set up select library with any streams that need monitoring
selector = selectors.DefaultSelector()
out_handler = _get_stream_handler(proc.stdout, out_fd, selector)
err_handler = _get_stream_handler(proc.stderr, err_fd, selector)

with closing(BytesIO()) as combined_io:
while True:
try:
for event, _ in selector.select():
combined_io.write(event.data.process())

except BlockingIOError:
pass

if proc.poll() is not None:
combined = combined_io.getvalue()
break

stdout_res = out_handler.singular if out_handler else b""
stderr_res = err_handler.singular if err_handler else b""

result = ProcessResult(
proc.returncode,
stdout_res,
stderr_res,
combined,
command,
)

if check:
result.check_returncode()

return result


@contextmanager
def _select_stream(stream: Stream, default_stream: TextIO) -> Generator[int]:
"""Select and return an appropriate raw file descriptor.
Based on the input, this function returns a raw integer file descriptor according
to what is expected from the ``run()`` function in this same module.
If determining the file handle involves opening our own, this generator handles
closing it afterwards.
"""
if stream == DEVNULL:
with open(os.devnull, "wb") as s:
yield s.fileno()
elif isinstance(stream, int):
yield stream
elif stream is None:
yield default_stream.fileno()
else:
yield stream.fileno()


def _get_stream_handler(
proc_std: IO[bytes] | None, write_fd: int, selector: selectors.BaseSelector
) -> _ProcessStream | None:
"""Create a stream handle if necessary and register it."""
if not proc_std:
return None

proc_fd = proc_std.fileno()
os.set_blocking(proc_fd, False)
handler = _ProcessStream(proc_fd, write_fd)
selector.register(proc_std, selectors.EVENT_READ, handler)
return handler


@dataclass
class ProcessError(Exception):
"""Simple error for failed processes.
Generally raised if the return code of a process is non-zero.
"""

result: ProcessResult
Loading

0 comments on commit 5bbd983

Please sign in to comment.