From dd77e9d0ee7194d2d749de38efd3d2ff0756a529 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 7 Nov 2024 08:46:05 -0500 Subject: [PATCH 01/56] feat: add fork_utils module Adds a fork_utils module for capturing stdout/stderr separately, while also keeping a (best-effort) temporally-accurate combined account of the two. --- craft_parts/errors.py | 9 ++- craft_parts/executor/step_handler.py | 17 +++--- craft_parts/utils/fork_utils.py | 83 ++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 9 deletions(-) create mode 100644 craft_parts/utils/fork_utils.py diff --git a/craft_parts/errors.py b/craft_parts/errors.py index 39008b1a4..7bd0110e7 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -482,10 +482,17 @@ 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 brief = f"Failed to run the build script for part {part_name!r}." + + if stderr is not None: + brief += "\nCaptured standard error:" + for line in stderr.split(b"\n"): + if line: + brief += f"\n:: {line.decode()}" + 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" diff --git a/craft_parts/executor/step_handler.py b/craft_parts/executor/step_handler.py index aee896592..464fe77e8 100644 --- a/craft_parts/executor/step_handler.py +++ b/craft_parts/executor/step_handler.py @@ -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, fork_utils from . import filesets from .filesets import Fileset @@ -441,6 +441,8 @@ def _create_and_run_script( stdout: Stream, stderr: Stream, build_environment_script_path: Path | None = None, + part_name: str = "", + plugin_name: str = "", ) -> None: """Create a script with step-specific commands and execute it.""" with script_path.open("w") as run_file: @@ -458,10 +460,9 @@ 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, - ) + fork = fork_utils.run([script_path], cwd) + + if fork.returncode != 0: + raise errors.PluginBuildError( + part_name=part_name, plugin_name=plugin_name, stderr=fork.stderr + ) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py new file mode 100644 index 000000000..58df9a23b --- /dev/null +++ b/craft_parts/utils/fork_utils.py @@ -0,0 +1,83 @@ +# -*- 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 . + +from dataclasses import dataclass +import os +from pathlib import Path +import select +import subprocess +from typing import cast, Union, Sequence + +Command = Union[str, Path, Sequence[Union[str, Path]]] + +BUF_SIZE = 4096 + +@dataclass +class ForkResult: + returncode: int + stdout: bytes + stderr: bytes + combined: bytes + +def run(command: Command, cwd: Path) -> ForkResult: + proc = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd) + + stdout = stderr = comb = b"" + + fdout = proc.stdout.fileno() # type: ignore # stdout (and stderr below) are guaranteed not `None` because we called with `subprocess.PIPE` + fderr = proc.stderr.fileno() # type: ignore + + os.set_blocking(fdout, False) + os.set_blocking(fderr, False) + + line_out = bytearray() + line_err = bytearray() + + while True: + r, _, _ = select.select([fdout, fderr], [], []) + + try: + if fdout in r: + data = os.read(fdout, BUF_SIZE) + i = data.rfind(b'\n') + if i >= 0: + line_out.extend(data[:i+1]) + comb += line_out + stdout += line_out + line_out.clear() + line_out.extend(data[i+1:]) + else: + line_out.extend(data) + + if fderr in r: + data = os.read(fderr, BUF_SIZE) + i = data.rfind(b'\n') + if i >= 0: + line_err.extend(data[:i+1]) + comb += line_err + stderr += line_err + line_err.clear() + line_err.extend(data[i+1:]) + else: + line_err.extend(data) + + except BlockingIOError: + pass + + if proc.poll() is not None: + break + + return ForkResult(proc.returncode, stdout, stderr, comb) From 3fea4e371594d5c346f584a51c4c339ddf478f81 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 7 Nov 2024 09:33:24 -0500 Subject: [PATCH 02/56] feat: respect the stdout and stderr arguments to ForkProcess ForkProcess now prints to the provided stdout/stderr, or nowhere at all if fileno -1 is given --- craft_parts/executor/step_handler.py | 2 +- craft_parts/utils/fork_utils.py | 68 ++++++++++++++++++++++++---- 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/craft_parts/executor/step_handler.py b/craft_parts/executor/step_handler.py index 464fe77e8..2cb3555b0 100644 --- a/craft_parts/executor/step_handler.py +++ b/craft_parts/executor/step_handler.py @@ -460,7 +460,7 @@ def _create_and_run_script( script_path.chmod(0o755) logger.debug("Executing %r", script_path) - fork = fork_utils.run([script_path], cwd) + fork = fork_utils.run([script_path], cwd=cwd, stdout=stdout, stderr=stderr) if fork.returncode != 0: raise errors.PluginBuildError( diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index 58df9a23b..2ac7ccb4b 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -19,11 +19,13 @@ from pathlib import Path import select import subprocess -from typing import cast, Union, Sequence +import threading +from typing import Union, Sequence, TextIO, Any Command = Union[str, Path, Sequence[Union[str, Path]]] +Stream = int | TextIO | None -BUF_SIZE = 4096 +_BUF_SIZE = 4096 @dataclass class ForkResult: @@ -32,10 +34,52 @@ class ForkResult: stderr: bytes combined: bytes -def run(command: Command, cwd: Path) -> ForkResult: +class StreamHandler(threading.Thread): + def __init__(self, true_fd: Stream) -> None: + super().__init__() + if isinstance(true_fd, int): + self._true_fd = true_fd + elif isinstance(true_fd, TextIO): + self._true_fd = true_fd.fileno() + else: + self._true_fd = -1 + + self.collected = b"" + self._read_pipe, self._write_pipe = os.pipe() + self._stop_flag = False + + def run(self) -> None: + while True: + r, _, _ = select.select([self._read_pipe], [], []) + + try: + if self._read_pipe in r: + data = os.read(self._read_pipe, _BUF_SIZE) + self.collected += data + if self._true_fd != -1: + os.write(self._true_fd, data) + + except BlockingIOError: + pass + + if self._stop_flag: + break + + def stop(self) -> None: + if self._stop_flag: + return + self._stop_flag = True + os.close(self._read_pipe) + os.close(self._write_pipe) + + def write(self, data: bytearray) -> None: + os.write(self._write_pipe, data) + + +def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream) -> ForkResult: proc = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd) - stdout = stderr = comb = b"" + comb = b"" fdout = proc.stdout.fileno() # type: ignore # stdout (and stderr below) are guaranteed not `None` because we called with `subprocess.PIPE` fderr = proc.stderr.fileno() # type: ignore @@ -46,29 +90,33 @@ def run(command: Command, cwd: Path) -> ForkResult: line_out = bytearray() line_err = bytearray() + out = StreamHandler(stdout) + err = StreamHandler(stderr) + out.start() + err.start() while True: r, _, _ = select.select([fdout, fderr], [], []) try: if fdout in r: - data = os.read(fdout, BUF_SIZE) + data = os.read(fdout, _BUF_SIZE) i = data.rfind(b'\n') if i >= 0: line_out.extend(data[:i+1]) comb += line_out - stdout += line_out + out.write(line_out) line_out.clear() line_out.extend(data[i+1:]) else: line_out.extend(data) if fderr in r: - data = os.read(fderr, BUF_SIZE) + data = os.read(fderr, _BUF_SIZE) i = data.rfind(b'\n') if i >= 0: line_err.extend(data[:i+1]) comb += line_err - stderr += line_err + err.write(line_err) line_err.clear() line_err.extend(data[i+1:]) else: @@ -78,6 +126,8 @@ def run(command: Command, cwd: Path) -> ForkResult: pass if proc.poll() is not None: + out.stop() + err.stop() break - return ForkResult(proc.returncode, stdout, stderr, comb) + return ForkResult(proc.returncode, out.collected, err.collected, comb) From 9e81c177ab8be6b17867edc6c036725926b89f5d Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 7 Nov 2024 14:48:28 -0500 Subject: [PATCH 03/56] fix: properly join threads before closing pipes StreamHandler objects were closing their pipes before guaranteeing that their associated thread wouldn't attempt to read from them again. This commit should fix that issue. --- craft_parts/utils/fork_utils.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index 2ac7ccb4b..a973fb276 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -20,7 +20,7 @@ import select import subprocess import threading -from typing import Union, Sequence, TextIO, Any +from typing import Union, Sequence, TextIO Command = Union[str, Path, Sequence[Union[str, Path]]] Stream = int | TextIO | None @@ -69,6 +69,7 @@ def stop(self) -> None: if self._stop_flag: return self._stop_flag = True + self.join() os.close(self._read_pipe) os.close(self._write_pipe) From ba14122081b45dea7ab21f78f0e3d445f7398d63 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 7 Nov 2024 15:00:54 -0500 Subject: [PATCH 04/56] fix: discard certain errors relating to managed pipes --- craft_parts/utils/fork_utils.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index a973fb276..da5ad9fe6 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -62,6 +62,16 @@ def run(self) -> None: except BlockingIOError: pass + except OSError as e: + # Happens in cases where the thread ends but this function is still trying to read + # Since this function manages the read/write pipes, we can assume that this function + # should simply stop running at this point. + # + # FIXME: Needs refactoring. This could possibly ignore an error when trying to write + # to self._true_fd, which is not a file descriptor we manage. + if e.errno == 9: + break + if self._stop_flag: break From b2008eaf9736c364291b6acfc5321a48a6938eb4 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 7 Nov 2024 15:07:00 -0500 Subject: [PATCH 05/56] fix: refactor StreamHandler to override super join method --- craft_parts/utils/fork_utils.py | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index da5ad9fe6..f7abcfe69 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -62,24 +62,14 @@ def run(self) -> None: except BlockingIOError: pass - except OSError as e: - # Happens in cases where the thread ends but this function is still trying to read - # Since this function manages the read/write pipes, we can assume that this function - # should simply stop running at this point. - # - # FIXME: Needs refactoring. This could possibly ignore an error when trying to write - # to self._true_fd, which is not a file descriptor we manage. - if e.errno == 9: - break - if self._stop_flag: break - def stop(self) -> None: + def join(self, timeout: float | None = None) -> None: if self._stop_flag: return self._stop_flag = True - self.join() + super().join(timeout) os.close(self._read_pipe) os.close(self._write_pipe) @@ -137,8 +127,8 @@ def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream) -> ForkResu pass if proc.poll() is not None: - out.stop() - err.stop() + out.join() + err.join() break return ForkResult(proc.returncode, out.collected, err.collected, comb) From 6111ca388a62c91207c7a35cf10b4f5cb84771d4 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 7 Nov 2024 15:37:26 -0500 Subject: [PATCH 06/56] fix: use non-blocking pipes for reading/writing in StreamHandler --- craft_parts/utils/fork_utils.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index f7abcfe69..1c542e91d 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -46,10 +46,12 @@ def __init__(self, true_fd: Stream) -> None: self.collected = b"" self._read_pipe, self._write_pipe = os.pipe() + os.set_blocking(self._read_pipe, False) + os.set_blocking(self._write_pipe, False) self._stop_flag = False def run(self) -> None: - while True: + while not self._stop_flag: r, _, _ = select.select([self._read_pipe], [], []) try: @@ -62,9 +64,6 @@ def run(self) -> None: except BlockingIOError: pass - if self._stop_flag: - break - def join(self, timeout: float | None = None) -> None: if self._stop_flag: return From b7d7480740241931e3a57f7811ecdcb352828a15 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 7 Nov 2024 15:58:45 -0500 Subject: [PATCH 07/56] fix: handle pipes being closed unexpectedly --- craft_parts/utils/fork_utils.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index 1c542e91d..1c0fd63d4 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -59,16 +59,27 @@ def run(self) -> None: data = os.read(self._read_pipe, _BUF_SIZE) self.collected += data if self._true_fd != -1: - os.write(self._true_fd, data) + try: + os.write(self._true_fd, data) + except OSError: + raise RuntimeError("Stream handle given to StreamHandler object was unreachable. Was it closed early?") except BlockingIOError: pass - def join(self, timeout: float | None = None) -> None: + except OSError as e: + # Occurs when the pipe closes while trying to read from it. This generally happens if the program + # responsible for the pipe is stopped. Since that makes it expected behavior for the pipe to be + # closed, we can discard this specific error + if e.errno == 9: + return + else: + raise e + + def stop(self) -> None: if self._stop_flag: return self._stop_flag = True - super().join(timeout) os.close(self._read_pipe) os.close(self._write_pipe) @@ -126,8 +137,8 @@ def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream) -> ForkResu pass if proc.poll() is not None: - out.join() - err.join() + out.stop() + err.stop() break return ForkResult(proc.returncode, out.collected, err.collected, comb) From f7def79dd8a2d2130a013b2e77dc2c7d419efa4b Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Fri, 8 Nov 2024 09:16:36 -0500 Subject: [PATCH 08/56] feat: propogate subprocess errors up Raise a unique exception when scripts fail, that way different caller functions can decide how to handle the failure themselves --- craft_parts/executor/step_handler.py | 16 +++++++--------- craft_parts/utils/fork_utils.py | 6 ++++++ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/craft_parts/executor/step_handler.py b/craft_parts/executor/step_handler.py index 2cb3555b0..b4cafa4de 100644 --- a/craft_parts/executor/step_handler.py +++ b/craft_parts/executor/step_handler.py @@ -122,10 +122,10 @@ def _builtin_pull(self) -> StepContents: stdout=self._stdout, stderr=self._stderr, ) - except subprocess.CalledProcessError as process_error: + except fork_utils.ForkError: raise errors.PluginPullError( part_name=self._part.name - ) from process_error + ) return StepContents() @@ -153,10 +153,10 @@ def _builtin_build(self) -> StepContents: stdout=self._stdout, stderr=self._stderr, ) - except subprocess.CalledProcessError as process_error: + except fork_utils.ForkError as forkerror: raise errors.PluginBuildError( - part_name=self._part.name, plugin_name=self._part.plugin_name - ) from process_error + part_name=self._part.name, plugin_name=self._part.plugin_name, stderr=forkerror.result.stderr + ) return StepContents() @@ -441,8 +441,6 @@ def _create_and_run_script( stdout: Stream, stderr: Stream, build_environment_script_path: Path | None = None, - part_name: str = "", - plugin_name: str = "", ) -> None: """Create a script with step-specific commands and execute it.""" with script_path.open("w") as run_file: @@ -463,6 +461,6 @@ def _create_and_run_script( fork = fork_utils.run([script_path], cwd=cwd, stdout=stdout, stderr=stderr) if fork.returncode != 0: - raise errors.PluginBuildError( - part_name=part_name, plugin_name=plugin_name, stderr=fork.stderr + raise fork_utils.ForkError( + result=fork, cwd=cwd, command=script_path ) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index 1c0fd63d4..c6d703926 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -142,3 +142,9 @@ def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream) -> ForkResu break return ForkResult(proc.returncode, out.collected, err.collected, comb) + +@dataclass +class ForkError(Exception): + result: ForkResult + cwd: Path + command: Command From b564a94003ce2d0ea0b2118bc7ea5f580ece5770 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Fri, 8 Nov 2024 09:42:50 -0500 Subject: [PATCH 09/56] doc: document new module --- craft_parts/utils/fork_utils.py | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index c6d703926..25cc97e95 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -20,9 +20,9 @@ import select import subprocess import threading -from typing import Union, Sequence, TextIO +from typing import Sequence, TextIO -Command = Union[str, Path, Sequence[Union[str, Path]]] +Command = str | Path | Sequence[str | Path] Stream = int | TextIO | None _BUF_SIZE = 4096 @@ -35,12 +35,14 @@ class ForkResult: combined: bytes class StreamHandler(threading.Thread): - def __init__(self, true_fd: Stream) -> None: + """Helper class for splitting a stream into two destinations: the stream handed to it via `fd` and the `self.collected` field.""" + def __init__(self, fd: Stream) -> None: + """`true_fd` should be the file descriptor that this instance writes to""" super().__init__() - if isinstance(true_fd, int): - self._true_fd = true_fd - elif isinstance(true_fd, TextIO): - self._true_fd = true_fd.fileno() + if isinstance(fd, int): + self._true_fd = fd + elif isinstance(fd, TextIO): + self._true_fd = fd.fileno() else: self._true_fd = -1 @@ -51,6 +53,7 @@ def __init__(self, true_fd: Stream) -> None: self._stop_flag = False def run(self) -> None: + """Constantly check if `self._read_pipe` has any data ready to be read, then duplicate it if so""" while not self._stop_flag: r, _, _ = select.select([self._read_pipe], [], []) @@ -77,6 +80,7 @@ def run(self) -> None: raise e def stop(self) -> None: + """Stops monitoring the stream and closes all associated pipes""" if self._stop_flag: return self._stop_flag = True @@ -84,10 +88,18 @@ def stop(self) -> None: os.close(self._write_pipe) def write(self, data: bytearray) -> None: + """Sends a message to write to the channels managed by this instance""" os.write(self._write_pipe, data) def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream) -> ForkResult: + """Executes a subprocess and collects its stdout and stderr streams as separate accounts and a singular, combined account. + Args: + command: Command to execute. + cwd: Path to execute in. + stdout: Handle to a fd or I/O stream to treat as stdout + stderr: Handle to a fd or I/O stream to treat as stderr + """ proc = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd) comb = b"" @@ -145,6 +157,7 @@ def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream) -> ForkResu @dataclass class ForkError(Exception): + """Simple error for failed forked processes. Generally raised if the return code of a forked process is non-zero.""" result: ForkResult cwd: Path command: Command From 5a6dca4e3b3066bab443a82e2ac4869cf4b35503 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Fri, 8 Nov 2024 09:45:25 -0500 Subject: [PATCH 10/56] chore: refactor ForkError to be raised within the module Moves the raising of ForkError from the caller to the `run()` function itself, additionally adding a `check` parameter for skipping this exception if it should be ignored. --- craft_parts/executor/step_handler.py | 7 +------ craft_parts/utils/fork_utils.py | 12 ++++++++++-- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/craft_parts/executor/step_handler.py b/craft_parts/executor/step_handler.py index b4cafa4de..8479c7b56 100644 --- a/craft_parts/executor/step_handler.py +++ b/craft_parts/executor/step_handler.py @@ -458,9 +458,4 @@ def _create_and_run_script( script_path.chmod(0o755) logger.debug("Executing %r", script_path) - fork = fork_utils.run([script_path], cwd=cwd, stdout=stdout, stderr=stderr) - - if fork.returncode != 0: - raise fork_utils.ForkError( - result=fork, cwd=cwd, command=script_path - ) + fork_utils.run([script_path], cwd=cwd, stdout=stdout, stderr=stderr) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index 25cc97e95..c512e8a1c 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -92,13 +92,16 @@ def write(self, data: bytearray) -> None: os.write(self._write_pipe, data) -def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream) -> ForkResult: +def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream, check: bool = False) -> ForkResult: """Executes a subprocess and collects its stdout and stderr streams as separate accounts and a singular, combined account. Args: command: Command to execute. cwd: Path to execute in. stdout: Handle to a fd or I/O stream to treat as stdout stderr: Handle to a fd or I/O stream to treat as stderr + + Raises: + ForkError when forked process exits with a non-zero return code """ proc = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd) @@ -153,7 +156,12 @@ def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream) -> ForkResu err.stop() break - return ForkResult(proc.returncode, out.collected, err.collected, comb) + result = ForkResult(proc.returncode, out.collected, err.collected, comb) + + if check and result.returncode != 0: + raise ForkError(result=result, cwd=cwd, command=command) + + return result @dataclass class ForkError(Exception): From a7092614f34c01386413595060a65b1954e40db8 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Fri, 8 Nov 2024 11:50:44 -0500 Subject: [PATCH 11/56] fix: correctly check for an error return code in scripts --- craft_parts/executor/step_handler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/craft_parts/executor/step_handler.py b/craft_parts/executor/step_handler.py index 8479c7b56..13b59e586 100644 --- a/craft_parts/executor/step_handler.py +++ b/craft_parts/executor/step_handler.py @@ -458,4 +458,4 @@ def _create_and_run_script( script_path.chmod(0o755) logger.debug("Executing %r", script_path) - fork_utils.run([script_path], cwd=cwd, stdout=stdout, stderr=stderr) + fork_utils.run([script_path], cwd=cwd, stdout=stdout, stderr=stderr, check=True) From 172f3d345982b18067d204f3c77df67456b2027b Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Fri, 8 Nov 2024 14:00:04 -0500 Subject: [PATCH 12/56] style: fix all linter errors --- craft_parts/errors.py | 4 +- craft_parts/utils/fork_utils.py | 65 +++++++++++++++++++-------------- 2 files changed, 40 insertions(+), 29 deletions(-) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index 7bd0110e7..f8105ac4b 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -486,13 +486,13 @@ def __init__(self, *, part_name: str, plugin_name: str, stderr: bytes | None = N self.part_name = part_name self.plugin_name = plugin_name brief = f"Failed to run the build script for part {part_name!r}." - + if stderr is not None: brief += "\nCaptured standard error:" for line in stderr.split(b"\n"): if line: brief += f"\n:: {line.decode()}" - + 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" diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index c512e8a1c..0e1ffa40c 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -14,13 +14,17 @@ # You should have received a copy of the GNU Lesser General Public License # along with this program. If not, see . -from dataclasses import dataclass +"""Utilities for executing subprocesses and handling their stdout and stderr streams.""" + +import errno import os -from pathlib import Path import select import subprocess import threading -from typing import Sequence, TextIO +from collections.abc import Sequence +from dataclasses import dataclass +from pathlib import Path +from typing import IO, TextIO, cast Command = str | Path | Sequence[str | Path] Stream = int | TextIO | None @@ -29,6 +33,8 @@ @dataclass class ForkResult: + """Describes the outcome of a forked process.""" + returncode: int stdout: bytes stderr: bytes @@ -36,8 +42,9 @@ class ForkResult: class StreamHandler(threading.Thread): """Helper class for splitting a stream into two destinations: the stream handed to it via `fd` and the `self.collected` field.""" + def __init__(self, fd: Stream) -> None: - """`true_fd` should be the file descriptor that this instance writes to""" + """`fd` should be the file descriptor that this instance writes to.""" super().__init__() if isinstance(fd, int): self._true_fd = fd @@ -53,7 +60,7 @@ def __init__(self, fd: Stream) -> None: self._stop_flag = False def run(self) -> None: - """Constantly check if `self._read_pipe` has any data ready to be read, then duplicate it if so""" + """Constantly check if `self._read_pipe` has any data ready to be read, then duplicate it if so.""" while not self._stop_flag: r, _, _ = select.select([self._read_pipe], [], []) @@ -66,21 +73,20 @@ def run(self) -> None: os.write(self._true_fd, data) except OSError: raise RuntimeError("Stream handle given to StreamHandler object was unreachable. Was it closed early?") - + except BlockingIOError: pass except OSError as e: # Occurs when the pipe closes while trying to read from it. This generally happens if the program - # responsible for the pipe is stopped. Since that makes it expected behavior for the pipe to be + # responsible for the pipe is stopped. Since that makes it expected behavior for the pipe to be # closed, we can discard this specific error - if e.errno == 9: + if e.errno == errno.EBADF: return - else: - raise e + raise def stop(self) -> None: - """Stops monitoring the stream and closes all associated pipes""" + """Stop monitoring the stream and close all associated pipes.""" if self._stop_flag: return self._stop_flag = True @@ -88,27 +94,31 @@ def stop(self) -> None: os.close(self._write_pipe) def write(self, data: bytearray) -> None: - """Sends a message to write to the channels managed by this instance""" + """Send a message to write to the channels managed by this instance.""" os.write(self._write_pipe, data) -def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream, check: bool = False) -> ForkResult: - """Executes a subprocess and collects its stdout and stderr streams as separate accounts and a singular, combined account. - Args: - command: Command to execute. - cwd: Path to execute in. - stdout: Handle to a fd or I/O stream to treat as stdout - stderr: Handle to a fd or I/O stream to treat as stderr +def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream, *, check: bool = False) -> ForkResult: + """Execute a subprocess and collects its stdout and stderr streams as separate accounts and a singular, combined account. + + Args: + command: Command to execute. + cwd: Path to execute in. + stdout: Handle to a fd or I/O stream to treat as stdout + stderr: Handle to a fd or I/O stream to treat as stderr + check: If True, a ForkError exception will be raised if `command` returns a non-zero return code. + + Raises: + ForkError when forked process exits with a non-zero return code - Raises: - ForkError when forked process exits with a non-zero return code """ proc = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd) comb = b"" - - fdout = proc.stdout.fileno() # type: ignore # stdout (and stderr below) are guaranteed not `None` because we called with `subprocess.PIPE` - fderr = proc.stderr.fileno() # type: ignore + + # stdout and stderr are guaranteed not `None` because we called with `subprocess.PIPE` + fdout = cast(IO[bytes], proc.stdout).fileno() + fderr = cast(IO[bytes], proc.stderr).fileno() os.set_blocking(fdout, False) os.set_blocking(fderr, False) @@ -126,7 +136,7 @@ def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream, check: bool try: if fdout in r: data = os.read(fdout, _BUF_SIZE) - i = data.rfind(b'\n') + i = data.rfind(b"\n") if i >= 0: line_out.extend(data[:i+1]) comb += line_out @@ -138,7 +148,7 @@ def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream, check: bool if fderr in r: data = os.read(fderr, _BUF_SIZE) - i = data.rfind(b'\n') + i = data.rfind(b"\n") if i >= 0: line_err.extend(data[:i+1]) comb += line_err @@ -157,7 +167,7 @@ def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream, check: bool break result = ForkResult(proc.returncode, out.collected, err.collected, comb) - + if check and result.returncode != 0: raise ForkError(result=result, cwd=cwd, command=command) @@ -166,6 +176,7 @@ def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream, check: bool @dataclass class ForkError(Exception): """Simple error for failed forked processes. Generally raised if the return code of a forked process is non-zero.""" + result: ForkResult cwd: Path command: Command From ec8757649521d37b4adcf40e8f23d1a6f066105f Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Fri, 8 Nov 2024 14:00:52 -0500 Subject: [PATCH 13/56] test: update tests to work with new fork_utils module --- tests/unit/executor/test_step_handler.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/executor/test_step_handler.py b/tests/unit/executor/test_step_handler.py index 8a55a99b1..8a3bd33a6 100644 --- a/tests/unit/executor/test_step_handler.py +++ b/tests/unit/executor/test_step_handler.py @@ -137,7 +137,7 @@ def test_run_builtin_overlay(self, new_dir, mocker): assert result == StepContents() def test_run_builtin_build(self, new_dir, partitions, mocker): - mock_run = mocker.patch("subprocess.run") + mock_run = mocker.patch("craft_parts.utils.fork_utils.run") Path("parts/p1/run").mkdir(parents=True) sh = _step_handler_for_step( @@ -283,7 +283,7 @@ def test_run_builtin_invalid(self, new_dir): def test_run_builtin_pull_strict(self, new_dir, mocker): """Test the Pull step in strict mode calls get_pull_commands()""" Path("parts/p1/run").mkdir(parents=True) - mock_run = mocker.patch("subprocess.run") + mock_run = mocker.patch("craft_parts.utils.fork_utils.run") self._project_info._strict_mode = True sh = _step_handler_for_step( Step.PULL, From 413c18983fb54ad8422c3fe3afba502a8fea8f26 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Fri, 8 Nov 2024 14:09:20 -0500 Subject: [PATCH 14/56] style: fix MORE linter errors --- craft_parts/errors.py | 4 +++- craft_parts/executor/step_handler.py | 8 ++++---- craft_parts/utils/fork_utils.py | 25 +++++++++++++++++-------- 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index f8105ac4b..a273d0455 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -482,7 +482,9 @@ class PluginBuildError(PartsError): :param plugin_name: The name of the plugin being processed. """ - def __init__(self, *, part_name: str, plugin_name: str, stderr: bytes | None = None) -> 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 brief = f"Failed to run the build script for part {part_name!r}." diff --git a/craft_parts/executor/step_handler.py b/craft_parts/executor/step_handler.py index 13b59e586..a6b5cba4f 100644 --- a/craft_parts/executor/step_handler.py +++ b/craft_parts/executor/step_handler.py @@ -123,9 +123,7 @@ def _builtin_pull(self) -> StepContents: stderr=self._stderr, ) except fork_utils.ForkError: - raise errors.PluginPullError( - part_name=self._part.name - ) + raise errors.PluginPullError(part_name=self._part.name) return StepContents() @@ -155,7 +153,9 @@ def _builtin_build(self) -> StepContents: ) except fork_utils.ForkError as forkerror: raise errors.PluginBuildError( - part_name=self._part.name, plugin_name=self._part.plugin_name, stderr=forkerror.result.stderr + part_name=self._part.name, + plugin_name=self._part.plugin_name, + stderr=forkerror.result.stderr, ) return StepContents() diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index 0e1ffa40c..ba5dcb814 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -31,6 +31,7 @@ _BUF_SIZE = 4096 + @dataclass class ForkResult: """Describes the outcome of a forked process.""" @@ -40,6 +41,7 @@ class ForkResult: stderr: bytes combined: bytes + class StreamHandler(threading.Thread): """Helper class for splitting a stream into two destinations: the stream handed to it via `fd` and the `self.collected` field.""" @@ -72,7 +74,9 @@ def run(self) -> None: try: os.write(self._true_fd, data) except OSError: - raise RuntimeError("Stream handle given to StreamHandler object was unreachable. Was it closed early?") + raise RuntimeError( + "Stream handle given to StreamHandler object was unreachable. Was it closed early?" + ) except BlockingIOError: pass @@ -98,7 +102,9 @@ def write(self, data: bytearray) -> None: os.write(self._write_pipe, data) -def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream, *, check: bool = False) -> ForkResult: +def run( + command: Command, cwd: Path, stdout: Stream, stderr: Stream, *, check: bool = False +) -> ForkResult: """Execute a subprocess and collects its stdout and stderr streams as separate accounts and a singular, combined account. Args: @@ -112,12 +118,14 @@ def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream, *, check: b ForkError when forked process exits with a non-zero return code """ - proc = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd) + proc = subprocess.Popen( + command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd + ) comb = b"" # stdout and stderr are guaranteed not `None` because we called with `subprocess.PIPE` - fdout = cast(IO[bytes], proc.stdout).fileno() + fdout = cast(IO[bytes], proc.stdout).fileno() fderr = cast(IO[bytes], proc.stderr).fileno() os.set_blocking(fdout, False) @@ -138,11 +146,11 @@ def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream, *, check: b data = os.read(fdout, _BUF_SIZE) i = data.rfind(b"\n") if i >= 0: - line_out.extend(data[:i+1]) + line_out.extend(data[: i + 1]) comb += line_out out.write(line_out) line_out.clear() - line_out.extend(data[i+1:]) + line_out.extend(data[i + 1 :]) else: line_out.extend(data) @@ -150,11 +158,11 @@ def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream, *, check: b data = os.read(fderr, _BUF_SIZE) i = data.rfind(b"\n") if i >= 0: - line_err.extend(data[:i+1]) + line_err.extend(data[: i + 1]) comb += line_err err.write(line_err) line_err.clear() - line_err.extend(data[i+1:]) + line_err.extend(data[i + 1 :]) else: line_err.extend(data) @@ -173,6 +181,7 @@ def run(command: Command, cwd: Path, stdout: Stream, stderr: Stream, *, check: b return result + @dataclass class ForkError(Exception): """Simple error for failed forked processes. Generally raised if the return code of a forked process is non-zero.""" From 2f28ed4c432d1b88e5573ddbb935fd5d4d4bb8be Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Fri, 8 Nov 2024 14:34:08 -0500 Subject: [PATCH 15/56] doc: improve and reformat doc strings --- craft_parts/utils/fork_utils.py | 51 ++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index ba5dcb814..cc40ef7f2 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -43,10 +43,13 @@ class ForkResult: class StreamHandler(threading.Thread): - """Helper class for splitting a stream into two destinations: the stream handed to it via `fd` and the `self.collected` field.""" + """Helper class for splitting a stream into two destinations: the stream handed to it via ``fd`` and the ``self.collected`` field.""" def __init__(self, fd: Stream) -> None: - """`fd` should be the file descriptor that this instance writes to.""" + """Initialize a StreamHandler + + :param fd: The "real" file descriptor to print to. + """ super().__init__() if isinstance(fd, int): self._true_fd = fd @@ -55,21 +58,29 @@ def __init__(self, fd: Stream) -> None: else: self._true_fd = -1 - self.collected = b"" + self._collected = b"" self._read_pipe, self._write_pipe = os.pipe() os.set_blocking(self._read_pipe, False) os.set_blocking(self._write_pipe, False) self._stop_flag = False + @property + def collected(self) -> bytes: + return self._collected + def run(self) -> None: - """Constantly check if `self._read_pipe` has any data ready to be read, then duplicate it if so.""" + """Constantly check if any data has been sent, then duplicate it if so. + + :raises RuntimeError: If the file descriptor passed at initialization is closed before `.stop()` is called. + :raises OSError: If an internal error occurs preventing this function from reading or writing from pipes. + """ while not self._stop_flag: r, _, _ = select.select([self._read_pipe], [], []) try: if self._read_pipe in r: data = os.read(self._read_pipe, _BUF_SIZE) - self.collected += data + self._collected += data if self._true_fd != -1: try: os.write(self._true_fd, data) @@ -98,7 +109,10 @@ def stop(self) -> None: os.close(self._write_pipe) def write(self, data: bytearray) -> None: - """Send a message to write to the channels managed by this instance.""" + """Send a message to write to the channels managed by this instance. + + :param data: Byte data to write + """ os.write(self._write_pipe, data) @@ -107,16 +121,21 @@ def run( ) -> ForkResult: """Execute a subprocess and collects its stdout and stderr streams as separate accounts and a singular, combined account. - Args: - command: Command to execute. - cwd: Path to execute in. - stdout: Handle to a fd or I/O stream to treat as stdout - stderr: Handle to a fd or I/O stream to treat as stderr - check: If True, a ForkError exception will be raised if `command` returns a non-zero return code. - - Raises: - ForkError when forked process exits with a non-zero return code - + :param command: Command to execute. + :type Command: + :param cwd: Path to execute in. + :type Path: + :param stdout: Handle to a fd or I/O stream to treat as stdout + :type Stream: + :param stderr: Handle to a fd or I/O stream to treat as stderr + :type Stream: + :param check: If True, a ForkError exception will be raised if ``command`` returns a non-zero return code. + :type bool: + + :raises ForkError: If forked process exits with a non-zero return code + + :return: A description of the forked process' outcome + :rtype: ForkResult """ proc = subprocess.Popen( command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd From d1563f3f32b78f0c3c332f7e6ff05e05a7c16ad7 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Fri, 8 Nov 2024 14:36:12 -0500 Subject: [PATCH 16/56] style: fix docstring linter errors --- craft_parts/utils/fork_utils.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index cc40ef7f2..ffcb1672b 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -47,7 +47,7 @@ class StreamHandler(threading.Thread): def __init__(self, fd: Stream) -> None: """Initialize a StreamHandler - + :param fd: The "real" file descriptor to print to. """ super().__init__() @@ -70,7 +70,7 @@ def collected(self) -> bytes: def run(self) -> None: """Constantly check if any data has been sent, then duplicate it if so. - + :raises RuntimeError: If the file descriptor passed at initialization is closed before `.stop()` is called. :raises OSError: If an internal error occurs preventing this function from reading or writing from pipes. """ @@ -110,7 +110,7 @@ def stop(self) -> None: def write(self, data: bytearray) -> None: """Send a message to write to the channels managed by this instance. - + :param data: Byte data to write """ os.write(self._write_pipe, data) From a2fcc496b866a22363128f6017f6879a5d99f405 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Fri, 8 Nov 2024 14:38:57 -0500 Subject: [PATCH 17/56] style: fix more docstring linter errors --- craft_parts/utils/fork_utils.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index ffcb1672b..bf8d0a551 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -46,7 +46,7 @@ class StreamHandler(threading.Thread): """Helper class for splitting a stream into two destinations: the stream handed to it via ``fd`` and the ``self.collected`` field.""" def __init__(self, fd: Stream) -> None: - """Initialize a StreamHandler + """Initialize a StreamHandler. :param fd: The "real" file descriptor to print to. """ @@ -66,6 +66,7 @@ def __init__(self, fd: Stream) -> None: @property def collected(self) -> bytes: + """Data collected from stream over the lifetime of this handler.""" return self._collected def run(self) -> None: From f0502edb286a03d87fb443a8aa94931a5a560383 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Tue, 12 Nov 2024 09:06:51 -0500 Subject: [PATCH 18/56] fix: more closely replicate the subprocess module for backwards compatibility Most tests and other modules expect `subprocess.run()` to be called, so the kwargs passed into it are given with the expectation that they'll be handled that way. Instead of changing each call one-by-one, this commit changes `fork_utils.run()` to default to the system's stdout/stderr when given `None` instead of not redirecting output at all. --- craft_parts/utils/fork_utils.py | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index bf8d0a551..c58f31187 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -20,6 +20,7 @@ import os import select import subprocess +import sys import threading from collections.abc import Sequence from dataclasses import dataclass @@ -53,10 +54,10 @@ def __init__(self, fd: Stream) -> None: super().__init__() if isinstance(fd, int): self._true_fd = fd - elif isinstance(fd, TextIO): - self._true_fd = fd.fileno() - else: + elif fd is None: self._true_fd = -1 + else: + self._true_fd = fd.fileno() self._collected = b"" self._read_pipe, self._write_pipe = os.pipe() @@ -126,11 +127,11 @@ def run( :type Command: :param cwd: Path to execute in. :type Path: - :param stdout: Handle to a fd or I/O stream to treat as stdout + :param stdout: Handle to a fd or I/O stream to treat as stdout. None defaults to `sys.stdout`, and any negative number results in no printing at all. :type Stream: - :param stderr: Handle to a fd or I/O stream to treat as stderr + :param stderr: Handle to a fd or I/O stream to treat as stderr. None defaults to `sys.stderr`, and any negative number results in no printing at all. :type Stream: - :param check: If True, a ForkError exception will be raised if ``command`` returns a non-zero return code. + :param check: If True, a ForkError exception will be raised if `command` returns a non-zero return code. :type bool: :raises ForkError: If forked process exits with a non-zero return code @@ -154,8 +155,15 @@ def run( line_out = bytearray() line_err = bytearray() - out = StreamHandler(stdout) - err = StreamHandler(stderr) + if stdout is None: + out = StreamHandler(sys.stdout) + else: + out = StreamHandler(stdout) + if stderr is None: + err = StreamHandler(sys.stderr) + else: + err = StreamHandler(stderr) + out.start() err.start() while True: From 4fd7ce4fa3abe14daed92522f53198c83eae850d Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Tue, 12 Nov 2024 09:07:31 -0500 Subject: [PATCH 19/56] fix(test): change tests to expect new stderr capturing upon failure --- tests/integration/executor/test_errors.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/integration/executor/test_errors.py b/tests/integration/executor/test_errors.py index baffa71e0..0f3172d8c 100644 --- a/tests/integration/executor/test_errors.py +++ b/tests/integration/executor/test_errors.py @@ -73,6 +73,11 @@ def test_plugin_build_errors(new_dir, partitions): assert str(raised.value) == textwrap.dedent( """\ Failed to run the build script for part 'foo'. + Captured standard error: + :: + go mod download all + :: + go install -p 1 ./... + :: # example.com/hello + :: ./hello.go:9:9: undefined: fmt.Printfs Check the build output and verify the project can work with the 'go' plugin.""" ) assert raised.value.doc_slug == "/reference/plugins.html" From b3c2c2feedb22e04bb84eb1eaee831b009640e3a Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Tue, 12 Nov 2024 09:12:52 -0500 Subject: [PATCH 20/56] doc: expand wordlist and correct to EN-GB spellings --- craft_parts/utils/fork_utils.py | 4 ++-- docs/common/craft-parts/craft-parts.wordlist.txt | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index c58f31187..c60f350fd 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -47,7 +47,7 @@ class StreamHandler(threading.Thread): """Helper class for splitting a stream into two destinations: the stream handed to it via ``fd`` and the ``self.collected`` field.""" def __init__(self, fd: Stream) -> None: - """Initialize a StreamHandler. + """Initialise a StreamHandler. :param fd: The "real" file descriptor to print to. """ @@ -73,7 +73,7 @@ def collected(self) -> bytes: def run(self) -> None: """Constantly check if any data has been sent, then duplicate it if so. - :raises RuntimeError: If the file descriptor passed at initialization is closed before `.stop()` is called. + :raises RuntimeError: If the file descriptor passed at initialisation is closed before `.stop()` is called. :raises OSError: If an internal error occurs preventing this function from reading or writing from pipes. """ while not self._stop_flag: diff --git a/docs/common/craft-parts/craft-parts.wordlist.txt b/docs/common/craft-parts/craft-parts.wordlist.txt index 4e251b5d2..ef82134c1 100644 --- a/docs/common/craft-parts/craft-parts.wordlist.txt +++ b/docs/common/craft-parts/craft-parts.wordlist.txt @@ -73,6 +73,8 @@ Fileset FilesetConflict FilesetError Filesets +ForkError +ForkResult GenerateJsonSchema GiB GitSource @@ -142,6 +144,7 @@ NpmPlugin NpmPluginEnvironmentValidator NpmPluginProperties OCI +OSError OsRelease OsReleaseCodenameError OsReleaseIdError @@ -256,6 +259,7 @@ StepContents StepHandler StepInfo StepState +StreamHandler Submodules Subpackages Subtractive @@ -317,6 +321,7 @@ craftctl crafter ctl customization +cwd dataset defs deps @@ -333,6 +338,7 @@ dst emacs env executables +fd filepath filepaths fileset @@ -411,6 +417,7 @@ readthedocs recognized reorganize reorganized +returncode runtime rustc rustup @@ -438,6 +445,7 @@ subdirectory subkey submodules subprocess +subprocesses subtree summarized svn From 807880d3cc51d87d96314620f63f692f8436a5e9 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Tue, 12 Nov 2024 10:03:03 -0500 Subject: [PATCH 21/56] style: fix ruff warnings --- craft_parts/utils/fork_utils.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index c60f350fd..8b6fa7778 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -155,14 +155,8 @@ def run( line_out = bytearray() line_err = bytearray() - if stdout is None: - out = StreamHandler(sys.stdout) - else: - out = StreamHandler(stdout) - if stderr is None: - err = StreamHandler(sys.stderr) - else: - err = StreamHandler(stderr) + out = StreamHandler(sys.stdout if stdout is None else stdout) + err = StreamHandler(sys.stderr if stderr is None else stderr) out.start() err.start() From 1a8baef50b3b6240a1ea4e764b7cdf38ab380287 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Tue, 12 Nov 2024 10:17:03 -0500 Subject: [PATCH 22/56] doc: expand wordlist --- docs/common/craft-parts/craft-parts.wordlist.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/common/craft-parts/craft-parts.wordlist.txt b/docs/common/craft-parts/craft-parts.wordlist.txt index ef82134c1..578bb003c 100644 --- a/docs/common/craft-parts/craft-parts.wordlist.txt +++ b/docs/common/craft-parts/craft-parts.wordlist.txt @@ -451,6 +451,7 @@ summarized svn symlink symlinks +sys texinfo tgz toml From 0e8f96152fbac7700c4528e86ccd8f842e761a8f Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Tue, 12 Nov 2024 16:30:36 -0500 Subject: [PATCH 23/56] test: add tests for new module --- craft_parts/utils/fork_utils.py | 20 ++++--- tests/integration/utils/test_fork_utils.py | 48 ++++++++++++++++ .../utils/test_fork_utils/complex.sh | 16 ++++++ .../utils/test_fork_utils/fails.sh | 2 + .../utils/test_fork_utils/simple.sh | 2 + tests/unit/utils/test_fork_utils.py | 56 +++++++++++++++++++ 6 files changed, 137 insertions(+), 7 deletions(-) create mode 100644 tests/integration/utils/test_fork_utils.py create mode 100755 tests/integration/utils/test_fork_utils/complex.sh create mode 100755 tests/integration/utils/test_fork_utils/fails.sh create mode 100755 tests/integration/utils/test_fork_utils/simple.sh create mode 100644 tests/unit/utils/test_fork_utils.py diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index 8b6fa7778..8a7e78843 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -77,7 +77,7 @@ def run(self) -> None: :raises OSError: If an internal error occurs preventing this function from reading or writing from pipes. """ while not self._stop_flag: - r, _, _ = select.select([self._read_pipe], [], []) + r, _, _ = select.select([self._read_pipe], [], [], 0.25) try: if self._read_pipe in r: @@ -102,11 +102,12 @@ def run(self) -> None: return raise - def stop(self) -> None: - """Stop monitoring the stream and close all associated pipes.""" + def join(self, timeout: float | None = None) -> None: + """Stop monitoring the stream and close all associated pipes. Blocks until done reading.""" if self._stop_flag: return self._stop_flag = True + super().join(timeout) os.close(self._read_pipe) os.close(self._write_pipe) @@ -119,7 +120,12 @@ def write(self, data: bytearray) -> None: def run( - command: Command, cwd: Path, stdout: Stream, stderr: Stream, *, check: bool = False + command: Command, + *, + cwd: Path | None = None, + stdout: Stream = None, + stderr: Stream = None, + check: bool = False, ) -> ForkResult: """Execute a subprocess and collects its stdout and stderr streams as separate accounts and a singular, combined account. @@ -192,8 +198,8 @@ def run( pass if proc.poll() is not None: - out.stop() - err.stop() + out.join() + err.join() break result = ForkResult(proc.returncode, out.collected, err.collected, comb) @@ -209,5 +215,5 @@ class ForkError(Exception): """Simple error for failed forked processes. Generally raised if the return code of a forked process is non-zero.""" result: ForkResult - cwd: Path + cwd: Path | None command: Command diff --git a/tests/integration/utils/test_fork_utils.py b/tests/integration/utils/test_fork_utils.py new file mode 100644 index 000000000..7c02c724b --- /dev/null +++ b/tests/integration/utils/test_fork_utils.py @@ -0,0 +1,48 @@ +# -*- 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 . + +from collections.abc import Iterable +from pathlib import Path + +import pytest +from craft_parts.utils import fork_utils + + +@pytest.fixture +def case_dir() -> Path: + return Path(__file__).parent / "test_fork_utils" + + +def test_simple_script(case_dir, capfd) -> None: + fork_utils.run(["/bin/bash", case_dir / "simple.sh"]) + assert capfd.readouterr().out == "foo\n" + + +def test_complex_script(case_dir, capfd) -> None: + def build_string_from_iter(it: Iterable[int]) -> str: + return "\n".join({str(n) for n in it}) + "\n" + + result = fork_utils.run(["/bin/bash", case_dir / "complex.sh"]) + assert build_string_from_iter(range(0, 400)) == result.combined.decode("utf-8") + + assert build_string_from_iter(range(0, 400, 4)) == capfd.readouterr().out + + +def test_fails_on_check(case_dir) -> None: + with pytest.raises(fork_utils.ForkError) as raises: + fork_utils.run(["/bin/bash", case_dir / "fails.sh"], check=True) + + assert raises.value.result.returncode == 1 diff --git a/tests/integration/utils/test_fork_utils/complex.sh b/tests/integration/utils/test_fork_utils/complex.sh new file mode 100755 index 000000000..7ba22f3df --- /dev/null +++ b/tests/integration/utils/test_fork_utils/complex.sh @@ -0,0 +1,16 @@ +#!/usr/bin/env bash + +n=0 + +for i in $(seq 1 100); do + echo "$n" + let "n++" + sleep 0 + echo "$n" >&2 + let "n++" + echo "$n" >&2 + let "n++" + echo "$n" >&2 + let "n++" + sleep 0 +done \ No newline at end of file diff --git a/tests/integration/utils/test_fork_utils/fails.sh b/tests/integration/utils/test_fork_utils/fails.sh new file mode 100755 index 000000000..987600155 --- /dev/null +++ b/tests/integration/utils/test_fork_utils/fails.sh @@ -0,0 +1,2 @@ +#!/usr/bin/env bash +exit 1 \ No newline at end of file diff --git a/tests/integration/utils/test_fork_utils/simple.sh b/tests/integration/utils/test_fork_utils/simple.sh new file mode 100755 index 000000000..892d6167a --- /dev/null +++ b/tests/integration/utils/test_fork_utils/simple.sh @@ -0,0 +1,2 @@ +#!/usr/bin/env bash +echo "foo" \ No newline at end of file diff --git a/tests/unit/utils/test_fork_utils.py b/tests/unit/utils/test_fork_utils.py new file mode 100644 index 000000000..ef0871711 --- /dev/null +++ b/tests/unit/utils/test_fork_utils.py @@ -0,0 +1,56 @@ +# -*- 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 . + +import sys + +import pytest +from craft_parts.utils import fork_utils + + +@pytest.mark.parametrize(("stdout", "expected"), [(None, -1), (5, 5)]) +def test_stream_selection( + stdout: fork_utils.Stream, expected: fork_utils.Stream +) -> None: + handler = fork_utils.StreamHandler(stdout) + assert handler._true_fd == expected + + +# Not one of the parametrize arguments above because pytest does not seem to allocate a sys.stdout in the decorator stage +def test_stream_fileno() -> None: + handler = fork_utils.StreamHandler(sys.stdout) + assert handler._true_fd == sys.stdout.fileno() + + +def test_pipe_write(capfd) -> None: + handler = fork_utils.StreamHandler(sys.stdout) + handler.start() + handler.write(bytearray("is anybody listening?", "utf-8")) + handler.join() + + assert capfd.readouterr().out == "is anybody listening?" + assert handler.collected == b"is anybody listening?" + + +def test_file_write(tmpdir) -> None: + with open(tmpdir / "foo", "w") as fout: + handler = fork_utils.StreamHandler(fout) + handler.start() + handler.write(bytearray("is anybody listening now?", "utf-8")) + handler.join() + + assert handler.collected == b"is anybody listening now?" + with open(tmpdir / "foo") as fin: + assert fin.read() == "is anybody listening now?" From 9bf2bb2dd1fa6c064bf9dd2d699fe5536933549c Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Wed, 13 Nov 2024 08:41:22 -0500 Subject: [PATCH 24/56] doc: correct docstrings for reST --- craft_parts/utils/fork_utils.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index 8a7e78843..215d2e55a 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -47,7 +47,7 @@ class StreamHandler(threading.Thread): """Helper class for splitting a stream into two destinations: the stream handed to it via ``fd`` and the ``self.collected`` field.""" def __init__(self, fd: Stream) -> None: - """Initialise a StreamHandler. + """Initialise a ``StreamHandler``. :param fd: The "real" file descriptor to print to. """ @@ -73,7 +73,7 @@ def collected(self) -> bytes: def run(self) -> None: """Constantly check if any data has been sent, then duplicate it if so. - :raises RuntimeError: If the file descriptor passed at initialisation is closed before `.stop()` is called. + :raises RuntimeError: If the file descriptor passed at initialisation is closed before ``.stop()`` is called. :raises OSError: If an internal error occurs preventing this function from reading or writing from pipes. """ while not self._stop_flag: @@ -133,11 +133,11 @@ def run( :type Command: :param cwd: Path to execute in. :type Path: - :param stdout: Handle to a fd or I/O stream to treat as stdout. None defaults to `sys.stdout`, and any negative number results in no printing at all. + :param stdout: Handle to a fd or I/O stream to treat as stdout. None defaults to ``sys.stdout``, and any negative number results in no printing at all. :type Stream: - :param stderr: Handle to a fd or I/O stream to treat as stderr. None defaults to `sys.stderr`, and any negative number results in no printing at all. + :param stderr: Handle to a fd or I/O stream to treat as stderr. None defaults to ``sys.stderr``, and any negative number results in no printing at all. :type Stream: - :param check: If True, a ForkError exception will be raised if `command` returns a non-zero return code. + :param check: If True, a ForkError exception will be raised if ``command`` returns a non-zero return code. :type bool: :raises ForkError: If forked process exits with a non-zero return code From 8585b0f7b93bcbddd811df72cfb21ea07d88efc9 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Wed, 13 Nov 2024 08:42:00 -0500 Subject: [PATCH 25/56] fix(test): correct generation of expected output in fork_util tests --- tests/integration/utils/test_fork_utils.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/integration/utils/test_fork_utils.py b/tests/integration/utils/test_fork_utils.py index 7c02c724b..5166fb54b 100644 --- a/tests/integration/utils/test_fork_utils.py +++ b/tests/integration/utils/test_fork_utils.py @@ -14,7 +14,6 @@ # You should have received a copy of the GNU Lesser General Public License # along with this program. If not, see . -from collections.abc import Iterable from pathlib import Path import pytest @@ -32,13 +31,20 @@ def test_simple_script(case_dir, capfd) -> None: def test_complex_script(case_dir, capfd) -> None: - def build_string_from_iter(it: Iterable[int]) -> str: - return "\n".join({str(n) for n in it}) + "\n" - result = fork_utils.run(["/bin/bash", case_dir / "complex.sh"]) - assert build_string_from_iter(range(0, 400)) == result.combined.decode("utf-8") - assert build_string_from_iter(range(0, 400, 4)) == capfd.readouterr().out + out, err = capfd.readouterr() + out_n = [int(s) for s in out.split()] + err_n = [int(s) for s in err.split()] + + comb_n = out_n + err_n + comb_sort = sorted(comb_n) + expected = "\n".join([str(n) for n in comb_sort]) + "\n" + assert expected == result.combined.decode("utf-8") + + out_sort = sorted(out_n) + expected = "\n".join([str(n) for n in out_sort]) + "\n" + assert expected == result.stdout.decode("utf-8") def test_fails_on_check(case_dir) -> None: From 9e1cb40640a41893067d51611a5e569baca6a065 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Wed, 13 Nov 2024 15:54:17 -0500 Subject: [PATCH 26/56] wip: testing --- craft_parts/utils/fork_utils.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index 215d2e55a..1d98c4c4f 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -77,7 +77,9 @@ def run(self) -> None: :raises OSError: If an internal error occurs preventing this function from reading or writing from pipes. """ while not self._stop_flag: + print(f"FD={self._true_fd} preparing a select") r, _, _ = select.select([self._read_pipe], [], [], 0.25) + print(f"FD={self._true_fd} select completed") try: if self._read_pipe in r: @@ -107,7 +109,9 @@ def join(self, timeout: float | None = None) -> None: if self._stop_flag: return self._stop_flag = True + print(f"FD={self._true_fd} waiting on super join") super().join(timeout) + print(f"FD={self._true_fd} joined on super") os.close(self._read_pipe) os.close(self._write_pipe) @@ -164,10 +168,14 @@ def run( out = StreamHandler(sys.stdout if stdout is None else stdout) err = StreamHandler(sys.stderr if stderr is None else stderr) + print(f"out-fd = {out._true_fd}") + print(f"err-fd = {err._true_fd}") out.start() err.start() while True: - r, _, _ = select.select([fdout, fderr], [], []) + print("selecting on main thread") + r, _, _ = select.select([fdout, fderr], [], [], 0.25) + print("selected on main thread") try: if fdout in r: @@ -198,8 +206,10 @@ def run( pass if proc.poll() is not None: + print("about to join") out.join() err.join() + print("joined") break result = ForkResult(proc.returncode, out.collected, err.collected, comb) From b0fecbfd309b708767c6641820583bb9a08d73fa Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 14 Nov 2024 11:52:27 -0500 Subject: [PATCH 27/56] fix: refactor to remove unnecessary threading and code complexity --- craft_parts/utils/fork_utils.py | 151 +++++++++------------------- tests/unit/utils/test_fork_utils.py | 40 ++------ 2 files changed, 52 insertions(+), 139 deletions(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index 1d98c4c4f..5cac52a35 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -16,22 +16,23 @@ """Utilities for executing subprocesses and handling their stdout and stderr streams.""" -import errno import os import select import subprocess import sys -import threading from collections.abc import Sequence from dataclasses import dataclass from pathlib import Path from typing import IO, TextIO, cast Command = str | Path | Sequence[str | Path] -Stream = int | TextIO | None +Stream = TextIO | int | None _BUF_SIZE = 4096 +# Compatibility with subprocess.DEVNULL +DEVNULL = subprocess.DEVNULL + @dataclass class ForkResult: @@ -43,87 +44,7 @@ class ForkResult: combined: bytes -class StreamHandler(threading.Thread): - """Helper class for splitting a stream into two destinations: the stream handed to it via ``fd`` and the ``self.collected`` field.""" - - def __init__(self, fd: Stream) -> None: - """Initialise a ``StreamHandler``. - - :param fd: The "real" file descriptor to print to. - """ - super().__init__() - if isinstance(fd, int): - self._true_fd = fd - elif fd is None: - self._true_fd = -1 - else: - self._true_fd = fd.fileno() - - self._collected = b"" - self._read_pipe, self._write_pipe = os.pipe() - os.set_blocking(self._read_pipe, False) - os.set_blocking(self._write_pipe, False) - self._stop_flag = False - - @property - def collected(self) -> bytes: - """Data collected from stream over the lifetime of this handler.""" - return self._collected - - def run(self) -> None: - """Constantly check if any data has been sent, then duplicate it if so. - - :raises RuntimeError: If the file descriptor passed at initialisation is closed before ``.stop()`` is called. - :raises OSError: If an internal error occurs preventing this function from reading or writing from pipes. - """ - while not self._stop_flag: - print(f"FD={self._true_fd} preparing a select") - r, _, _ = select.select([self._read_pipe], [], [], 0.25) - print(f"FD={self._true_fd} select completed") - - try: - if self._read_pipe in r: - data = os.read(self._read_pipe, _BUF_SIZE) - self._collected += data - if self._true_fd != -1: - try: - os.write(self._true_fd, data) - except OSError: - raise RuntimeError( - "Stream handle given to StreamHandler object was unreachable. Was it closed early?" - ) - - except BlockingIOError: - pass - - except OSError as e: - # Occurs when the pipe closes while trying to read from it. This generally happens if the program - # responsible for the pipe is stopped. Since that makes it expected behavior for the pipe to be - # closed, we can discard this specific error - if e.errno == errno.EBADF: - return - raise - - def join(self, timeout: float | None = None) -> None: - """Stop monitoring the stream and close all associated pipes. Blocks until done reading.""" - if self._stop_flag: - return - self._stop_flag = True - print(f"FD={self._true_fd} waiting on super join") - super().join(timeout) - print(f"FD={self._true_fd} joined on super") - os.close(self._read_pipe) - os.close(self._write_pipe) - - def write(self, data: bytearray) -> None: - """Send a message to write to the channels managed by this instance. - - :param data: Byte data to write - """ - os.write(self._write_pipe, data) - - -def run( +def run( # noqa: PLR0915 command: Command, *, cwd: Path | None = None, @@ -136,24 +57,26 @@ def run( :param command: Command to execute. :type Command: :param cwd: Path to execute in. - :type Path: - :param stdout: Handle to a fd or I/O stream to treat as stdout. None defaults to ``sys.stdout``, and any negative number results in no printing at all. + :type Path | None: + :param stdout: Handle to a fd or I/O stream to treat as stdout. None defaults to ``sys.stdout``, and fork_utils.DEVNULL can be passed for no printing. :type Stream: - :param stderr: Handle to a fd or I/O stream to treat as stderr. None defaults to ``sys.stderr``, and any negative number results in no printing at all. + :param stderr: Handle to a fd or I/O stream to treat as stderr. None defaults to ``sys.stderr``, and fork_utils.DEVNULL can be passed for no printing. :type Stream: :param check: If True, a ForkError exception will be raised if ``command`` returns a non-zero return code. :type bool: - :raises ForkError: If forked process exits with a non-zero return code + :raises ForkError: If forked process exits with a non-zero return code. + :raises OSError: If the specified executable is not found. - :return: A description of the forked process' outcome + :return: A description of the forked process' outcome. :rtype: ForkResult """ proc = subprocess.Popen( command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd ) - comb = b"" + stdout = _select_stream(stdout, sys.stdout) + stderr = _select_stream(stderr, sys.stderr) # stdout and stderr are guaranteed not `None` because we called with `subprocess.PIPE` fdout = cast(IO[bytes], proc.stdout).fileno() @@ -165,17 +88,10 @@ def run( line_out = bytearray() line_err = bytearray() - out = StreamHandler(sys.stdout if stdout is None else stdout) - err = StreamHandler(sys.stderr if stderr is None else stderr) + out = err = comb = b"" - print(f"out-fd = {out._true_fd}") - print(f"err-fd = {err._true_fd}") - out.start() - err.start() while True: - print("selecting on main thread") - r, _, _ = select.select([fdout, fderr], [], [], 0.25) - print("selected on main thread") + r, _, _ = select.select([fdout, fderr], [], []) try: if fdout in r: @@ -184,7 +100,8 @@ def run( if i >= 0: line_out.extend(data[: i + 1]) comb += line_out - out.write(line_out) + out += line_out + print(line_out.decode("utf-8"), file=stdout, end="") line_out.clear() line_out.extend(data[i + 1 :]) else: @@ -196,7 +113,8 @@ def run( if i >= 0: line_err.extend(data[: i + 1]) comb += line_err - err.write(line_err) + err += line_err + print(line_err.decode("utf-8"), file=stderr, end="") line_err.clear() line_err.extend(data[i + 1 :]) else: @@ -205,14 +123,22 @@ def run( except BlockingIOError: pass + except Exception: + if stdout.name == os.devnull: + stdout.close() + if stderr.name == os.devnull: + stderr.close() + raise + if proc.poll() is not None: - print("about to join") - out.join() - err.join() - print("joined") break - result = ForkResult(proc.returncode, out.collected, err.collected, comb) + if stdout.name == os.devnull: + stdout.close() + if stderr.name == os.devnull: + stderr.close() + + result = ForkResult(proc.returncode, out, err, comb) if check and result.returncode != 0: raise ForkError(result=result, cwd=cwd, command=command) @@ -220,6 +146,19 @@ def run( return result +def _select_stream(stream: Stream, default: TextIO) -> TextIO: + """Translate a ``Stream`` object into a usable Python stream handle.""" + if isinstance(stream, int): + if stream != DEVNULL: + raise ValueError( + f'Invalid stream "{stream}": Raw file descriptors are not supported.' + ) + return open(os.devnull, "w") + if stream is None: + return default + return stream + + @dataclass class ForkError(Exception): """Simple error for failed forked processes. Generally raised if the return code of a forked process is non-zero.""" diff --git a/tests/unit/utils/test_fork_utils.py b/tests/unit/utils/test_fork_utils.py index ef0871711..b6fe81380 100644 --- a/tests/unit/utils/test_fork_utils.py +++ b/tests/unit/utils/test_fork_utils.py @@ -14,43 +14,17 @@ # You should have received a copy of the GNU Lesser General Public License # along with this program. If not, see . -import sys - import pytest from craft_parts.utils import fork_utils -@pytest.mark.parametrize(("stdout", "expected"), [(None, -1), (5, 5)]) -def test_stream_selection( - stdout: fork_utils.Stream, expected: fork_utils.Stream -) -> None: - handler = fork_utils.StreamHandler(stdout) - assert handler._true_fd == expected - - -# Not one of the parametrize arguments above because pytest does not seem to allocate a sys.stdout in the decorator stage -def test_stream_fileno() -> None: - handler = fork_utils.StreamHandler(sys.stdout) - assert handler._true_fd == sys.stdout.fileno() - - -def test_pipe_write(capfd) -> None: - handler = fork_utils.StreamHandler(sys.stdout) - handler.start() - handler.write(bytearray("is anybody listening?", "utf-8")) - handler.join() - - assert capfd.readouterr().out == "is anybody listening?" - assert handler.collected == b"is anybody listening?" +def test_no_raw_fd(): + with pytest.raises(ValueError, match="Raw file descriptors are not supported."): + fork_utils.run(["true"], stdout=-999) -def test_file_write(tmpdir) -> None: - with open(tmpdir / "foo", "w") as fout: - handler = fork_utils.StreamHandler(fout) - handler.start() - handler.write(bytearray("is anybody listening now?", "utf-8")) - handler.join() +def test_devnull(capfd): + result = fork_utils.run(["echo", "hello"], stdout=fork_utils.DEVNULL) - assert handler.collected == b"is anybody listening now?" - with open(tmpdir / "foo") as fin: - assert fin.read() == "is anybody listening now?" + assert capfd.readouterr().out == "" + assert result.stdout == b"hello\n" From 9939a9da2afa7e6b353ad7c64cda5ebce9003bc1 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 14 Nov 2024 12:55:30 -0500 Subject: [PATCH 28/56] doc: update wordlist --- docs/common/craft-parts/craft-parts.wordlist.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/common/craft-parts/craft-parts.wordlist.txt b/docs/common/craft-parts/craft-parts.wordlist.txt index 578bb003c..4813e6cc8 100644 --- a/docs/common/craft-parts/craft-parts.wordlist.txt +++ b/docs/common/craft-parts/craft-parts.wordlist.txt @@ -49,6 +49,7 @@ DebError DebPackage DebSource DebSourceModel +DEVNULL DirtyReport DotPluginEnvironmentValidator Dotnet @@ -259,7 +260,6 @@ StepContents StepHandler StepInfo StepState -StreamHandler Submodules Subpackages Subtractive @@ -338,7 +338,6 @@ dst emacs env executables -fd filepath filepaths fileset From c19c325323659b08b5e339ed00501f631c6174b6 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 14 Nov 2024 14:40:55 -0500 Subject: [PATCH 29/56] fix: allow raw FDs for subprocess drop-in compatibility --- craft_parts/utils/fork_utils.py | 39 ++++++++++++++--------------- tests/unit/utils/test_fork_utils.py | 6 ----- 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index 5cac52a35..106a12f4e 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -75,8 +75,8 @@ def run( # noqa: PLR0915 command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd ) - stdout = _select_stream(stdout, sys.stdout) - stderr = _select_stream(stderr, sys.stderr) + stdout_fd = _select_stream(stdout, sys.stdout.fileno()) + stderr_fd = _select_stream(stderr, sys.stderr.fileno()) # stdout and stderr are guaranteed not `None` because we called with `subprocess.PIPE` fdout = cast(IO[bytes], proc.stdout).fileno() @@ -101,7 +101,7 @@ def run( # noqa: PLR0915 line_out.extend(data[: i + 1]) comb += line_out out += line_out - print(line_out.decode("utf-8"), file=stdout, end="") + os.write(stdout_fd, line_out) line_out.clear() line_out.extend(data[i + 1 :]) else: @@ -114,7 +114,7 @@ def run( # noqa: PLR0915 line_err.extend(data[: i + 1]) comb += line_err err += line_err - print(line_err.decode("utf-8"), file=stderr, end="") + os.write(stderr_fd, line_err) line_err.clear() line_err.extend(data[i + 1 :]) else: @@ -124,19 +124,19 @@ def run( # noqa: PLR0915 pass except Exception: - if stdout.name == os.devnull: - stdout.close() - if stderr.name == os.devnull: - stderr.close() + if stdout == DEVNULL: + os.close(stdout_fd) + if stderr == DEVNULL: + os.close(stderr_fd) raise if proc.poll() is not None: break - if stdout.name == os.devnull: - stdout.close() - if stderr.name == os.devnull: - stderr.close() + if stdout == DEVNULL: + os.close(stdout_fd) + if stderr == DEVNULL: + os.close(stderr_fd) result = ForkResult(proc.returncode, out, err, comb) @@ -146,16 +146,15 @@ def run( # noqa: PLR0915 return result -def _select_stream(stream: Stream, default: TextIO) -> TextIO: - """Translate a ``Stream`` object into a usable Python stream handle.""" - if isinstance(stream, int): - if stream != DEVNULL: - raise ValueError( - f'Invalid stream "{stream}": Raw file descriptors are not supported.' - ) - return open(os.devnull, "w") +def _select_stream(stream: Stream, default: int) -> int: + """Translate a ``Stream`` object into a raw FD.""" + if stream == DEVNULL: + return os.open(os.devnull, os.O_WRONLY) + if isinstance(stream, TextIO): + return stream.fileno() if stream is None: return default + return stream diff --git a/tests/unit/utils/test_fork_utils.py b/tests/unit/utils/test_fork_utils.py index b6fe81380..9d833d0d2 100644 --- a/tests/unit/utils/test_fork_utils.py +++ b/tests/unit/utils/test_fork_utils.py @@ -14,15 +14,9 @@ # You should have received a copy of the GNU Lesser General Public License # along with this program. If not, see . -import pytest from craft_parts.utils import fork_utils -def test_no_raw_fd(): - with pytest.raises(ValueError, match="Raw file descriptors are not supported."): - fork_utils.run(["true"], stdout=-999) - - def test_devnull(capfd): result = fork_utils.run(["echo", "hello"], stdout=fork_utils.DEVNULL) From 14ffdd4fb6e7e65b922b2860cc2aa253907c16b7 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 14 Nov 2024 15:07:31 -0500 Subject: [PATCH 30/56] fix: change stream selection conditions to properly get raw FDs --- craft_parts/utils/fork_utils.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/fork_utils.py index 106a12f4e..958bb23be 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/fork_utils.py @@ -150,12 +150,12 @@ def _select_stream(stream: Stream, default: int) -> int: """Translate a ``Stream`` object into a raw FD.""" if stream == DEVNULL: return os.open(os.devnull, os.O_WRONLY) - if isinstance(stream, TextIO): - return stream.fileno() + if isinstance(stream, int): + return stream if stream is None: return default - return stream + return stream.fileno() @dataclass From e6fafca62d67a5a992a20e211c9c449ee0b0747c Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 14 Nov 2024 15:07:37 -0500 Subject: [PATCH 31/56] doc: update wordlist --- docs/common/craft-parts/craft-parts.wordlist.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/common/craft-parts/craft-parts.wordlist.txt b/docs/common/craft-parts/craft-parts.wordlist.txt index 4813e6cc8..31de270e0 100644 --- a/docs/common/craft-parts/craft-parts.wordlist.txt +++ b/docs/common/craft-parts/craft-parts.wordlist.txt @@ -338,6 +338,7 @@ dst emacs env executables +fd filepath filepaths fileset From 9d73fde1b4883a9b5d702b132344e615be5d15a4 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Wed, 20 Nov 2024 09:20:25 -0500 Subject: [PATCH 32/56] chore: pr feedback, rename `fork_utils` to simply `process` --- craft_parts/errors.py | 2 +- craft_parts/executor/step_handler.py | 10 +++---- .../utils/{fork_utils.py => process.py} | 28 +++++++++---------- .../craft-parts/craft-parts.wordlist.txt | 4 +-- tests/integration/utils/test_fork_utils.py | 10 +++---- tests/unit/executor/test_step_handler.py | 4 +-- tests/unit/utils/test_fork_utils.py | 4 +-- 7 files changed, 31 insertions(+), 31 deletions(-) rename craft_parts/utils/{fork_utils.py => process.py} (83%) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index a273d0455..112759b2c 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -493,7 +493,7 @@ def __init__( brief += "\nCaptured standard error:" for line in stderr.split(b"\n"): if line: - brief += f"\n:: {line.decode()}" + brief += f"\n:: {line.decode("utf-8", errors="replace")}" resolution = f"Check the build output and verify the project can work with the {plugin_name!r} plugin." super().__init__( diff --git a/craft_parts/executor/step_handler.py b/craft_parts/executor/step_handler.py index a6b5cba4f..4a23b8aef 100644 --- a/craft_parts/executor/step_handler.py +++ b/craft_parts/executor/step_handler.py @@ -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, fork_utils +from craft_parts.utils import file_utils, process from . import filesets from .filesets import Fileset @@ -122,7 +122,7 @@ def _builtin_pull(self) -> StepContents: stdout=self._stdout, stderr=self._stderr, ) - except fork_utils.ForkError: + except process.ProcessError: raise errors.PluginPullError(part_name=self._part.name) return StepContents() @@ -151,11 +151,11 @@ def _builtin_build(self) -> StepContents: stdout=self._stdout, stderr=self._stderr, ) - except fork_utils.ForkError as forkerror: + except process.ProcessError as procerror: raise errors.PluginBuildError( part_name=self._part.name, plugin_name=self._part.plugin_name, - stderr=forkerror.result.stderr, + stderr=procerror.result.stderr, ) return StepContents() @@ -458,4 +458,4 @@ def _create_and_run_script( script_path.chmod(0o755) logger.debug("Executing %r", script_path) - fork_utils.run([script_path], cwd=cwd, stdout=stdout, stderr=stderr, check=True) + process.run([script_path], cwd=cwd, stdout=stdout, stderr=stderr, check=True) diff --git a/craft_parts/utils/fork_utils.py b/craft_parts/utils/process.py similarity index 83% rename from craft_parts/utils/fork_utils.py rename to craft_parts/utils/process.py index 958bb23be..5700aa96d 100644 --- a/craft_parts/utils/fork_utils.py +++ b/craft_parts/utils/process.py @@ -35,8 +35,8 @@ @dataclass -class ForkResult: - """Describes the outcome of a forked process.""" +class ProcessResult: + """Describes the outcome of a process.""" returncode: int stdout: bytes @@ -51,25 +51,25 @@ def run( # noqa: PLR0915 stdout: Stream = None, stderr: Stream = None, check: bool = False, -) -> ForkResult: +) -> ProcessResult: """Execute a subprocess and collects its stdout and stderr streams as separate accounts and a singular, combined account. :param command: Command to execute. :type Command: :param cwd: Path to execute in. :type Path | None: - :param stdout: Handle to a fd or I/O stream to treat as stdout. None defaults to ``sys.stdout``, and fork_utils.DEVNULL can be passed for no printing. + :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. :type Stream: - :param stderr: Handle to a fd or I/O stream to treat as stderr. None defaults to ``sys.stderr``, and fork_utils.DEVNULL can be passed for no printing. + :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. :type Stream: - :param check: If True, a ForkError exception will be raised if ``command`` returns a non-zero return code. + :param check: If True, a ProcessError exception will be raised if ``command`` returns a non-zero return code. :type bool: - :raises ForkError: If forked process exits with 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 forked process' outcome. - :rtype: ForkResult + :return: A description of the process' outcome. + :rtype: ProcessResult """ proc = subprocess.Popen( command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd @@ -138,10 +138,10 @@ def run( # noqa: PLR0915 if stderr == DEVNULL: os.close(stderr_fd) - result = ForkResult(proc.returncode, out, err, comb) + result = ProcessResult(proc.returncode, out, err, comb) if check and result.returncode != 0: - raise ForkError(result=result, cwd=cwd, command=command) + raise ProcessError(result=result, cwd=cwd, command=command) return result @@ -159,9 +159,9 @@ def _select_stream(stream: Stream, default: int) -> int: @dataclass -class ForkError(Exception): - """Simple error for failed forked processes. Generally raised if the return code of a forked process is non-zero.""" +class ProcessError(Exception): + """Simple error for failed processes. Generally raised if the return code of a process is non-zero.""" - result: ForkResult + result: ProcessResult cwd: Path | None command: Command diff --git a/docs/common/craft-parts/craft-parts.wordlist.txt b/docs/common/craft-parts/craft-parts.wordlist.txt index 31de270e0..6b7e85d6c 100644 --- a/docs/common/craft-parts/craft-parts.wordlist.txt +++ b/docs/common/craft-parts/craft-parts.wordlist.txt @@ -74,8 +74,8 @@ Fileset FilesetConflict FilesetError Filesets -ForkError -ForkResult +ProcessError +ProcessResult GenerateJsonSchema GiB GitSource diff --git a/tests/integration/utils/test_fork_utils.py b/tests/integration/utils/test_fork_utils.py index 5166fb54b..c64a42de9 100644 --- a/tests/integration/utils/test_fork_utils.py +++ b/tests/integration/utils/test_fork_utils.py @@ -17,7 +17,7 @@ from pathlib import Path import pytest -from craft_parts.utils import fork_utils +from craft_parts.utils import process @pytest.fixture @@ -26,12 +26,12 @@ def case_dir() -> Path: def test_simple_script(case_dir, capfd) -> None: - fork_utils.run(["/bin/bash", case_dir / "simple.sh"]) + process.run(["/bin/bash", case_dir / "simple.sh"]) assert capfd.readouterr().out == "foo\n" def test_complex_script(case_dir, capfd) -> None: - result = fork_utils.run(["/bin/bash", case_dir / "complex.sh"]) + result = process.run(["/bin/bash", case_dir / "complex.sh"]) out, err = capfd.readouterr() out_n = [int(s) for s in out.split()] @@ -48,7 +48,7 @@ def test_complex_script(case_dir, capfd) -> None: def test_fails_on_check(case_dir) -> None: - with pytest.raises(fork_utils.ForkError) as raises: - fork_utils.run(["/bin/bash", case_dir / "fails.sh"], check=True) + with pytest.raises(process.ProcessError) as raises: + process.run(["/bin/bash", case_dir / "fails.sh"], check=True) assert raises.value.result.returncode == 1 diff --git a/tests/unit/executor/test_step_handler.py b/tests/unit/executor/test_step_handler.py index 8a3bd33a6..287b262ba 100644 --- a/tests/unit/executor/test_step_handler.py +++ b/tests/unit/executor/test_step_handler.py @@ -137,7 +137,7 @@ def test_run_builtin_overlay(self, new_dir, mocker): assert result == StepContents() def test_run_builtin_build(self, new_dir, partitions, mocker): - mock_run = mocker.patch("craft_parts.utils.fork_utils.run") + mock_run = mocker.patch("craft_parts.utils.process.run") Path("parts/p1/run").mkdir(parents=True) sh = _step_handler_for_step( @@ -283,7 +283,7 @@ def test_run_builtin_invalid(self, new_dir): def test_run_builtin_pull_strict(self, new_dir, mocker): """Test the Pull step in strict mode calls get_pull_commands()""" Path("parts/p1/run").mkdir(parents=True) - mock_run = mocker.patch("craft_parts.utils.fork_utils.run") + mock_run = mocker.patch("craft_parts.utils.process.run") self._project_info._strict_mode = True sh = _step_handler_for_step( Step.PULL, diff --git a/tests/unit/utils/test_fork_utils.py b/tests/unit/utils/test_fork_utils.py index 9d833d0d2..be795a7ec 100644 --- a/tests/unit/utils/test_fork_utils.py +++ b/tests/unit/utils/test_fork_utils.py @@ -14,11 +14,11 @@ # You should have received a copy of the GNU Lesser General Public License # along with this program. If not, see . -from craft_parts.utils import fork_utils +from craft_parts.utils import process def test_devnull(capfd): - result = fork_utils.run(["echo", "hello"], stdout=fork_utils.DEVNULL) + result = process.run(["echo", "hello"], stdout=process.DEVNULL) assert capfd.readouterr().out == "" assert result.stdout == b"hello\n" From d42e2cda65a739826e1ff38fed837e4cf2ec7d80 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Wed, 20 Nov 2024 10:10:41 -0500 Subject: [PATCH 33/56] chore: pr feedback, move repeated code into a function --- craft_parts/utils/process.py | 92 +++++++++++++++++++----------------- 1 file changed, 49 insertions(+), 43 deletions(-) diff --git a/craft_parts/utils/process.py b/craft_parts/utils/process.py index 5700aa96d..3bfca4aaf 100644 --- a/craft_parts/utils/process.py +++ b/craft_parts/utils/process.py @@ -44,13 +44,40 @@ class ProcessResult: combined: bytes -def run( # noqa: PLR0915 +class _ProcessStream: + def __init__(self, read_fd, write_fd): + self.read_fd = read_fd + self.write_fd = write_fd + + self._linebuf = bytearray() + self._streambuf = b"" + + @property + def singular(self) -> bytes: + return self._streambuf + + def process(self) -> bytes: + """Process any data in ``self.read_fd``, then return it.""" + 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 + else: + self._linebuf.extend(data) + return b"" + +def run( command: Command, *, cwd: Path | None = None, stdout: Stream = None, stderr: Stream = None, - check: bool = False, + check: bool = True, ) -> ProcessResult: """Execute a subprocess and collects its stdout and stderr streams as separate accounts and a singular, combined account. @@ -75,70 +102,49 @@ def run( # noqa: PLR0915 command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd ) - stdout_fd = _select_stream(stdout, sys.stdout.fileno()) - stderr_fd = _select_stream(stderr, sys.stderr.fileno()) + out_fd = _select_stream(stdout, sys.stdout.fileno()) + err_fd = _select_stream(stderr, sys.stderr.fileno()) # stdout and stderr are guaranteed not `None` because we called with `subprocess.PIPE` - fdout = cast(IO[bytes], proc.stdout).fileno() - fderr = cast(IO[bytes], proc.stderr).fileno() + proc_stdout = cast(IO[bytes], proc.stdout).fileno() + proc_stderr = cast(IO[bytes], proc.stderr).fileno() - os.set_blocking(fdout, False) - os.set_blocking(fderr, False) + os.set_blocking(proc_stdout, False) + os.set_blocking(proc_stderr, False) - line_out = bytearray() - line_err = bytearray() - - out = err = comb = b"" + out_handler = _ProcessStream(proc_stdout, out_fd) + err_handler = _ProcessStream(proc_stderr, err_fd) + combined = b"" while True: - r, _, _ = select.select([fdout, fderr], [], []) + r, _, _ = select.select([proc_stdout, proc_stderr], [], []) try: - if fdout in r: - data = os.read(fdout, _BUF_SIZE) - i = data.rfind(b"\n") - if i >= 0: - line_out.extend(data[: i + 1]) - comb += line_out - out += line_out - os.write(stdout_fd, line_out) - line_out.clear() - line_out.extend(data[i + 1 :]) - else: - line_out.extend(data) - - if fderr in r: - data = os.read(fderr, _BUF_SIZE) - i = data.rfind(b"\n") - if i >= 0: - line_err.extend(data[: i + 1]) - comb += line_err - err += line_err - os.write(stderr_fd, line_err) - line_err.clear() - line_err.extend(data[i + 1 :]) - else: - line_err.extend(data) + if proc_stdout in r: + combined += out_handler.process() + + if proc_stderr in r: + combined += err_handler.process() except BlockingIOError: pass except Exception: if stdout == DEVNULL: - os.close(stdout_fd) + os.close(out_fd) if stderr == DEVNULL: - os.close(stderr_fd) + os.close(err_fd) raise if proc.poll() is not None: break if stdout == DEVNULL: - os.close(stdout_fd) + os.close(out_fd) if stderr == DEVNULL: - os.close(stderr_fd) + os.close(err_fd) - result = ProcessResult(proc.returncode, out, err, comb) + result = ProcessResult(proc.returncode, out_handler.singular, err_handler.singular, combined) if check and result.returncode != 0: raise ProcessError(result=result, cwd=cwd, command=command) From 60446a7eff17bbd70ecfb3c9c51197d6c6c73c3e Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Wed, 20 Nov 2024 10:21:40 -0500 Subject: [PATCH 34/56] chore: pr feedback, fix tests and add some more checks to them --- .../utils/test_fork_utils/fails.sh | 2 -- .../{test_fork_utils.py => test_process.py} | 23 ++++++++++++++---- .../complex.sh | 2 +- tests/integration/utils/test_process/fails.sh | 3 +++ .../simple.sh | 2 +- tests/unit/utils/test_fork_utils.py | 24 ------------------- 6 files changed, 23 insertions(+), 33 deletions(-) delete mode 100755 tests/integration/utils/test_fork_utils/fails.sh rename tests/integration/utils/{test_fork_utils.py => test_process.py} (71%) rename tests/integration/utils/{test_fork_utils => test_process}/complex.sh (97%) create mode 100755 tests/integration/utils/test_process/fails.sh rename tests/integration/utils/{test_fork_utils => test_process}/simple.sh (64%) delete mode 100644 tests/unit/utils/test_fork_utils.py diff --git a/tests/integration/utils/test_fork_utils/fails.sh b/tests/integration/utils/test_fork_utils/fails.sh deleted file mode 100755 index 987600155..000000000 --- a/tests/integration/utils/test_fork_utils/fails.sh +++ /dev/null @@ -1,2 +0,0 @@ -#!/usr/bin/env bash -exit 1 \ No newline at end of file diff --git a/tests/integration/utils/test_fork_utils.py b/tests/integration/utils/test_process.py similarity index 71% rename from tests/integration/utils/test_fork_utils.py rename to tests/integration/utils/test_process.py index c64a42de9..25e9a5927 100644 --- a/tests/integration/utils/test_fork_utils.py +++ b/tests/integration/utils/test_process.py @@ -22,7 +22,7 @@ @pytest.fixture def case_dir() -> Path: - return Path(__file__).parent / "test_fork_utils" + return Path(__file__).parent / "test_process" def test_simple_script(case_dir, capfd) -> None: @@ -31,6 +31,10 @@ def test_simple_script(case_dir, capfd) -> None: def test_complex_script(case_dir, capfd) -> None: + def _build_expected(raw: list[int]) -> str: + sorted_output = sorted(raw) + return "\n".join([str(n) for n in sorted_output]) + "\n" + result = process.run(["/bin/bash", case_dir / "complex.sh"]) out, err = capfd.readouterr() @@ -38,17 +42,26 @@ def test_complex_script(case_dir, capfd) -> None: err_n = [int(s) for s in err.split()] comb_n = out_n + err_n - comb_sort = sorted(comb_n) - expected = "\n".join([str(n) for n in comb_sort]) + "\n" + expected = _build_expected(comb_n) assert expected == result.combined.decode("utf-8") - out_sort = sorted(out_n) - expected = "\n".join([str(n) for n in out_sort]) + "\n" + expected = _build_expected(out_n) assert expected == result.stdout.decode("utf-8") + expected = _build_expected(err_n) + assert expected == result.stderr.decode("utf-8") + def test_fails_on_check(case_dir) -> None: with pytest.raises(process.ProcessError) as raises: process.run(["/bin/bash", case_dir / "fails.sh"], check=True) assert raises.value.result.returncode == 1 + assert raises.value.result.stderr == b"Error: Not enough cows.\n" + + +def test_devnull(capfd): + result = process.run(["echo", "hello"], stdout=process.DEVNULL) + + assert capfd.readouterr().out == "" + assert result.stdout == b"hello\n" diff --git a/tests/integration/utils/test_fork_utils/complex.sh b/tests/integration/utils/test_process/complex.sh similarity index 97% rename from tests/integration/utils/test_fork_utils/complex.sh rename to tests/integration/utils/test_process/complex.sh index 7ba22f3df..cc1fa4871 100755 --- a/tests/integration/utils/test_fork_utils/complex.sh +++ b/tests/integration/utils/test_process/complex.sh @@ -13,4 +13,4 @@ for i in $(seq 1 100); do echo "$n" >&2 let "n++" sleep 0 -done \ No newline at end of file +done diff --git a/tests/integration/utils/test_process/fails.sh b/tests/integration/utils/test_process/fails.sh new file mode 100755 index 000000000..d72cfa476 --- /dev/null +++ b/tests/integration/utils/test_process/fails.sh @@ -0,0 +1,3 @@ +#!/usr/bin/env bash +echo "Error: Not enough cows." >&2 +exit 1 diff --git a/tests/integration/utils/test_fork_utils/simple.sh b/tests/integration/utils/test_process/simple.sh similarity index 64% rename from tests/integration/utils/test_fork_utils/simple.sh rename to tests/integration/utils/test_process/simple.sh index 892d6167a..af85c9549 100755 --- a/tests/integration/utils/test_fork_utils/simple.sh +++ b/tests/integration/utils/test_process/simple.sh @@ -1,2 +1,2 @@ #!/usr/bin/env bash -echo "foo" \ No newline at end of file +echo "foo" diff --git a/tests/unit/utils/test_fork_utils.py b/tests/unit/utils/test_fork_utils.py deleted file mode 100644 index be795a7ec..000000000 --- a/tests/unit/utils/test_fork_utils.py +++ /dev/null @@ -1,24 +0,0 @@ -# -*- 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 . - -from craft_parts.utils import process - - -def test_devnull(capfd): - result = process.run(["echo", "hello"], stdout=process.DEVNULL) - - assert capfd.readouterr().out == "" - assert result.stdout == b"hello\n" From 7baa7b6aac544566c00114de1a332a984c9a80a8 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Wed, 20 Nov 2024 10:23:55 -0500 Subject: [PATCH 35/56] style: run linters --- craft_parts/utils/process.py | 18 ++++++++++-------- tests/integration/utils/test_process.py | 2 +- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/craft_parts/utils/process.py b/craft_parts/utils/process.py index 3bfca4aaf..cf1e68de3 100644 --- a/craft_parts/utils/process.py +++ b/craft_parts/utils/process.py @@ -45,17 +45,17 @@ class ProcessResult: class _ProcessStream: - def __init__(self, read_fd, write_fd): + 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"" - + @property def singular(self) -> bytes: return self._streambuf - + def process(self) -> bytes: """Process any data in ``self.read_fd``, then return it.""" data = os.read(self.read_fd, _BUF_SIZE) @@ -65,11 +65,11 @@ def process(self) -> bytes: self._streambuf += self._linebuf os.write(self.write_fd, self._linebuf) self._linebuf.clear() - self._linebuf.extend(data[i + 1:]) + self._linebuf.extend(data[i + 1 :]) return data - else: - self._linebuf.extend(data) - return b"" + self._linebuf.extend(data) + return b"" + def run( command: Command, @@ -144,7 +144,9 @@ def run( if stderr == DEVNULL: os.close(err_fd) - result = ProcessResult(proc.returncode, out_handler.singular, err_handler.singular, combined) + result = ProcessResult( + proc.returncode, out_handler.singular, err_handler.singular, combined + ) if check and result.returncode != 0: raise ProcessError(result=result, cwd=cwd, command=command) diff --git a/tests/integration/utils/test_process.py b/tests/integration/utils/test_process.py index 25e9a5927..4bb83966d 100644 --- a/tests/integration/utils/test_process.py +++ b/tests/integration/utils/test_process.py @@ -34,7 +34,7 @@ def test_complex_script(case_dir, capfd) -> None: def _build_expected(raw: list[int]) -> str: sorted_output = sorted(raw) return "\n".join([str(n) for n in sorted_output]) + "\n" - + result = process.run(["/bin/bash", case_dir / "complex.sh"]) out, err = capfd.readouterr() From 1269ba44adec237a34517d758a8b5d62ab750072 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Wed, 20 Nov 2024 10:30:24 -0500 Subject: [PATCH 36/56] fix: avoid nesting the same type of quotes in a string --- craft_parts/errors.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index 112759b2c..157d98b01 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -493,7 +493,7 @@ def __init__( brief += "\nCaptured standard error:" for line in stderr.split(b"\n"): if line: - brief += f"\n:: {line.decode("utf-8", errors="replace")}" + brief += f"\n:: {line.decode('utf-8', errors='replace')}" resolution = f"Check the build output and verify the project can work with the {plugin_name!r} plugin." super().__init__( From 7001e4f4897380a7fe8018b395b365d24b42b626 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Wed, 20 Nov 2024 15:02:27 -0500 Subject: [PATCH 37/56] feat: pr feedback, add dynamically generated brief property --- craft_parts/errors.py | 58 +++++++++++++++++++---- tests/integration/executor/test_errors.py | 3 +- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index 157d98b01..e5e9968a3 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -16,7 +16,8 @@ """Craft parts errors.""" -import dataclasses +import functools +from overrides import override import pathlib from collections.abc import Iterable from typing import TYPE_CHECKING @@ -25,7 +26,6 @@ from pydantic.error_wrappers import ErrorDict, Loc -@dataclasses.dataclass(repr=True) class PartsError(Exception): """Unexpected error. @@ -37,11 +37,17 @@ class PartsError(Exception): the Craft Parts documentation. """ - brief: str + _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] @@ -53,6 +59,15 @@ def __str__(self) -> str: return "\n".join(components) + + @property + def brief(self) -> str: + return self._brief + + + 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})" + class FeatureError(PartsError): """A feature is not configured as expected.""" @@ -487,19 +502,44 @@ def __init__( ) -> 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}." - if stderr is not None: - brief += "\nCaptured standard error:" - for line in stderr.split(b"\n"): - if line: - brief += f"\n:: {line.decode('utf-8', errors='replace')}" - 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 + @functools.cache + @override + def brief(self) -> str: + brief = f"Failed to run the build script for part {self.part_name!r}." + + if self.stderr is None: + return brief + + stderr = self.stderr.decode("utf-8", errors="replace") + brief += "\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: + brief += f"\n:: {line}" + + return brief + class PluginCleanError(PartsError): """Script to clean strict build preparation failed at runtime. diff --git a/tests/integration/executor/test_errors.py b/tests/integration/executor/test_errors.py index 0f3172d8c..44355423e 100644 --- a/tests/integration/executor/test_errors.py +++ b/tests/integration/executor/test_errors.py @@ -69,12 +69,11 @@ def test_plugin_build_errors(new_dir, partitions): with pytest.raises(errors.PluginBuildError) as raised: with lf.action_executor() as ctx: ctx.execute(actions) - + assert str(raised.value) == textwrap.dedent( """\ Failed to run the build script for part 'foo'. Captured standard error: - :: + go mod download all :: + go install -p 1 ./... :: # example.com/hello :: ./hello.go:9:9: undefined: fmt.Printfs From c6331d92d48fcb7b51ed9c352c0c63b53f172a01 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Wed, 20 Nov 2024 15:06:07 -0500 Subject: [PATCH 38/56] doc: pr feedback, break up docstrings that were too long --- craft_parts/utils/process.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/craft_parts/utils/process.py b/craft_parts/utils/process.py index cf1e68de3..2de572ec4 100644 --- a/craft_parts/utils/process.py +++ b/craft_parts/utils/process.py @@ -79,17 +79,21 @@ def run( stderr: Stream = None, check: bool = True, ) -> ProcessResult: - """Execute a subprocess and collects its stdout and stderr streams as separate accounts and a singular, combined account. + """Execute a subprocess and collects its stdout and stderr streams as separate + accounts and a singular, combined account. :param command: Command to execute. :type Command: :param cwd: Path to execute in. :type Path | None: - :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. + :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. :type Stream: - :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. + :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. :type Stream: - :param check: If True, a ProcessError exception will be raised if ``command`` returns a non-zero return code. + :param check: If True, a ProcessError exception will be raised if ``command`` + returns a non-zero return code. :type bool: :raises ProcessError: If process exits with a non-zero return code. @@ -168,7 +172,8 @@ def _select_stream(stream: Stream, default: int) -> int: @dataclass class ProcessError(Exception): - """Simple error for failed processes. Generally raised if the return code of a process is non-zero.""" + """Simple error for failed processes. Generally raised if the return code of + a process is non-zero.""" result: ProcessResult cwd: Path | None From ae5bd1a8570d53b2b41760d96f3d917251c9a140 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Wed, 20 Nov 2024 15:14:49 -0500 Subject: [PATCH 39/56] style: fix linter warnings --- craft_parts/errors.py | 23 +++++++++++++++-------- craft_parts/utils/process.py | 10 +++++++--- tests/integration/executor/test_errors.py | 2 +- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index e5e9968a3..adc3a01aa 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -16,12 +16,12 @@ """Craft parts errors.""" -import functools -from overrides import override import pathlib from collections.abc import Iterable from typing import TYPE_CHECKING +from overrides import override + if TYPE_CHECKING: from pydantic.error_wrappers import ErrorDict, Loc @@ -42,7 +42,13 @@ class PartsError(Exception): 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: + 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 @@ -59,11 +65,10 @@ def __str__(self) -> str: return "\n".join(components) - @property def brief(self) -> str: + """A brief summary of the error.""" return self._brief - 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})" @@ -510,16 +515,18 @@ def __init__( brief=brief, resolution=resolution, doc_slug="/reference/plugins.html" ) - @property - @functools.cache @override def brief(self) -> str: + """A brief summary of the error. + + Discards all trace lines that come before the last-executed script line + """ brief = f"Failed to run the build script for part {self.part_name!r}." if self.stderr is None: return brief - + stderr = self.stderr.decode("utf-8", errors="replace") brief += "\nCaptured standard error:" diff --git a/craft_parts/utils/process.py b/craft_parts/utils/process.py index 2de572ec4..c48f80e23 100644 --- a/craft_parts/utils/process.py +++ b/craft_parts/utils/process.py @@ -79,7 +79,9 @@ def run( stderr: Stream = None, check: bool = True, ) -> ProcessResult: - """Execute a subprocess and collects its stdout and stderr streams as separate + """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. @@ -172,8 +174,10 @@ def _select_stream(stream: Stream, default: int) -> int: @dataclass class ProcessError(Exception): - """Simple error for failed processes. Generally raised if the return code of - a process is non-zero.""" + """Simple error for failed processes. + + Generally raised if the return code of a process is non-zero. + """ result: ProcessResult cwd: Path | None diff --git a/tests/integration/executor/test_errors.py b/tests/integration/executor/test_errors.py index 44355423e..d74be9122 100644 --- a/tests/integration/executor/test_errors.py +++ b/tests/integration/executor/test_errors.py @@ -69,7 +69,7 @@ def test_plugin_build_errors(new_dir, partitions): with pytest.raises(errors.PluginBuildError) as raised: with lf.action_executor() as ctx: ctx.execute(actions) - + assert str(raised.value) == textwrap.dedent( """\ Failed to run the build script for part 'foo'. From 8cc4e550abedbfb1462a99b2729e3028ab9ad095 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 21 Nov 2024 08:50:37 -0500 Subject: [PATCH 40/56] fix: pr feedback, improve performance by using StringIO --- craft_parts/errors.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index adc3a01aa..4a4bb37c0 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -16,10 +16,10 @@ """Craft parts errors.""" +from io import StringIO import pathlib from collections.abc import Iterable from typing import TYPE_CHECKING - from overrides import override if TYPE_CHECKING: @@ -522,13 +522,14 @@ def brief(self) -> str: Discards all trace lines that come before the last-executed script line """ - brief = f"Failed to run the build script for part {self.part_name!r}." + brief_io = StringIO() + brief_io.write(f"Failed to run the build script for part {self.part_name!r}.") if self.stderr is None: - return brief + return brief_io.read() stderr = self.stderr.decode("utf-8", errors="replace") - brief += "\nCaptured standard error:" + brief_io.write("\nCaptured standard error:") stderr_lines = stderr.split("\n") # Find the final command captured in the logs @@ -543,9 +544,9 @@ def brief(self) -> str: for line in stderr_lines[last_command:]: if line: - brief += f"\n:: {line}" + brief_io.write(f"\n:: {line}") - return brief + return brief_io.read() class PluginCleanError(PartsError): From 31cb7557a0a4a00efd68b6edf86ea2260b30639e Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 21 Nov 2024 09:08:49 -0500 Subject: [PATCH 41/56] test: pr feedback, improve coverage of a test --- tests/integration/utils/test_process.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/integration/utils/test_process.py b/tests/integration/utils/test_process.py index 4bb83966d..dbfe41b1a 100644 --- a/tests/integration/utils/test_process.py +++ b/tests/integration/utils/test_process.py @@ -37,10 +37,17 @@ def _build_expected(raw: list[int]) -> str: result = process.run(["/bin/bash", case_dir / "complex.sh"]) + out, err = capfd.readouterr() out_n = [int(s) for s in out.split()] err_n = [int(s) for s in err.split()] + # From complex.sh + expected_out_size = 100 + expected_err_size = 300 + assert len(out_n) == expected_out_size + assert len(err_n) == expected_err_size + comb_n = out_n + err_n expected = _build_expected(comb_n) assert expected == result.combined.decode("utf-8") From 0d35b53c81ac079a9d16dbfbc34c62829bd90001 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 21 Nov 2024 09:32:23 -0500 Subject: [PATCH 42/56] fix: correctly read from stringio object --- craft_parts/errors.py | 5 ++++- tests/integration/utils/test_process.py | 1 - 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index 4a4bb37c0..0446ffcb4 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -16,10 +16,11 @@ """Craft parts errors.""" -from io import StringIO import pathlib from collections.abc import Iterable +from io import StringIO from typing import TYPE_CHECKING + from overrides import override if TYPE_CHECKING: @@ -526,6 +527,7 @@ def brief(self) -> str: brief_io.write(f"Failed to run the build script for part {self.part_name!r}.") if self.stderr is None: + brief_io.seek(0) return brief_io.read() stderr = self.stderr.decode("utf-8", errors="replace") @@ -546,6 +548,7 @@ def brief(self) -> str: if line: brief_io.write(f"\n:: {line}") + brief_io.seek(0) return brief_io.read() diff --git a/tests/integration/utils/test_process.py b/tests/integration/utils/test_process.py index dbfe41b1a..8f3cb57ea 100644 --- a/tests/integration/utils/test_process.py +++ b/tests/integration/utils/test_process.py @@ -37,7 +37,6 @@ def _build_expected(raw: list[int]) -> str: result = process.run(["/bin/bash", case_dir / "complex.sh"]) - out, err = capfd.readouterr() out_n = [int(s) for s in out.split()] err_n = [int(s) for s in err.split()] From 6373a5c59da1556c0f1d3dd2252884fcdbe898be Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 21 Nov 2024 16:59:44 -0500 Subject: [PATCH 43/56] chore: pr feedback, improve usage of StringIO Co-authored-by: Dariusz Duda --- craft_parts/errors.py | 55 ++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index 0446ffcb4..e542644ee 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -16,6 +16,7 @@ """Craft parts errors.""" +import contextlib import pathlib from collections.abc import Iterable from io import StringIO @@ -523,33 +524,33 @@ def brief(self) -> str: Discards all trace lines that come before the last-executed script line """ - brief_io = StringIO() - brief_io.write(f"Failed to run the build script for part {self.part_name!r}.") - - if self.stderr is None: - brief_io.seek(0) - return brief_io.read() - - stderr = self.stderr.decode("utf-8", errors="replace") - brief_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: - brief_io.write(f"\n:: {line}") - - brief_io.seek(0) - return brief_io.read() + with contextlib.closing(StringIO()) as brief_io: + brief_io.write( + f"Failed to run the build script for part {self.part_name!r}." + ) + + if self.stderr is None: + return brief_io.getvalue() + + stderr = self.stderr.decode("utf-8", errors="replace") + brief_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: + brief_io.write(f"\n:: {line}") + + return brief_io.getvalue() class PluginCleanError(PartsError): From 9adea8fbd6882bdf0ffeb689b91a71d095b36296 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Fri, 22 Nov 2024 10:01:17 -0500 Subject: [PATCH 44/56] feat: pr feedback, move stderr output to error details --- craft_parts/errors.py | 28 ++++++++++------------- tests/integration/executor/test_errors.py | 1 + 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index e542644ee..1ff5aeca0 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -39,8 +39,8 @@ class PartsError(Exception): the Craft Parts documentation. """ - _brief: str - details: str | None = None + brief: str + _details: str | None = None resolution: str | None = None doc_slug: str | None = None @@ -51,8 +51,8 @@ def __init__( resolution: str | None = None, doc_slug: str | None = None, ) -> None: - self._brief = brief - self.details = details + self.brief = brief + self._details = details self.resolution = resolution self.doc_slug = doc_slug @@ -67,14 +67,14 @@ def __str__(self) -> str: return "\n".join(components) - @property - def brief(self) -> str: - """A brief summary of the error.""" - return self._brief - 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.""" @@ -519,18 +519,14 @@ def __init__( @property @override - def brief(self) -> str: - """A brief summary of the error. + 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 brief_io: - brief_io.write( - f"Failed to run the build script for part {self.part_name!r}." - ) - if self.stderr is None: - return brief_io.getvalue() + return None stderr = self.stderr.decode("utf-8", errors="replace") brief_io.write("\nCaptured standard error:") diff --git a/tests/integration/executor/test_errors.py b/tests/integration/executor/test_errors.py index d74be9122..bdd46fd7f 100644 --- a/tests/integration/executor/test_errors.py +++ b/tests/integration/executor/test_errors.py @@ -73,6 +73,7 @@ def test_plugin_build_errors(new_dir, partitions): assert str(raised.value) == textwrap.dedent( """\ Failed to run the build script for part 'foo'. + Captured standard error: :: + go install -p 1 ./... :: # example.com/hello From 76aac75bab69bc2780bcc333d9f04ac2cb568c10 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Fri, 22 Nov 2024 15:41:48 -0500 Subject: [PATCH 45/56] chore: pr feedback, don't declare private field --- craft_parts/errors.py | 1 - 1 file changed, 1 deletion(-) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index 1ff5aeca0..bdf908da5 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -40,7 +40,6 @@ class PartsError(Exception): """ brief: str - _details: str | None = None resolution: str | None = None doc_slug: str | None = None From 8f975fb70072218c471605567552699428b037cc Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Mon, 25 Nov 2024 11:33:04 -0500 Subject: [PATCH 46/56] chore: rename variable to reflect change in purpose --- craft_parts/errors.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index bdf908da5..38f5a4ba8 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -523,12 +523,12 @@ def details(self) -> str | None: Discards all trace lines that come before the last-executed script line """ - with contextlib.closing(StringIO()) as brief_io: + with contextlib.closing(StringIO()) as details_io: if self.stderr is None: return None stderr = self.stderr.decode("utf-8", errors="replace") - brief_io.write("\nCaptured standard error:") + details_io.write("\nCaptured standard error:") stderr_lines = stderr.split("\n") # Find the final command captured in the logs @@ -543,9 +543,9 @@ def details(self) -> str | None: for line in stderr_lines[last_command:]: if line: - brief_io.write(f"\n:: {line}") + details_io.write(f"\n:: {line}") - return brief_io.getvalue() + return details_io.getvalue() class PluginCleanError(PartsError): From a33929866bb970ac957285468302b1dc550ec5f9 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Mon, 25 Nov 2024 12:25:50 -0500 Subject: [PATCH 47/56] chore: pr feedback, remove extra newline Co-authored-by: Dariusz Duda --- craft_parts/errors.py | 1 - 1 file changed, 1 deletion(-) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index 38f5a4ba8..17f95ae30 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -510,7 +510,6 @@ def __init__( 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" From 94fed0362d97cafeefc787de608b429f1be13937 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Mon, 25 Nov 2024 12:26:24 -0500 Subject: [PATCH 48/56] chore(docs): pr feedback, improve docstring Co-authored-by: Dariusz Duda --- craft_parts/utils/process.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/craft_parts/utils/process.py b/craft_parts/utils/process.py index c48f80e23..c62796c27 100644 --- a/craft_parts/utils/process.py +++ b/craft_parts/utils/process.py @@ -57,7 +57,7 @@ def singular(self) -> bytes: return self._streambuf def process(self) -> bytes: - """Process any data in ``self.read_fd``, then return it.""" + """Forward any data from ``self.read_fd`` to ``self.write_fd`` and return a copy of it.""" data = os.read(self.read_fd, _BUF_SIZE) i = data.rfind(b"\n") if i >= 0: From 00820793baa2ec9bbf41db729c725b945f135907 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Mon, 25 Nov 2024 13:02:54 -0500 Subject: [PATCH 49/56] chore: pr feedback, clarify raised error --- craft_parts/executor/step_handler.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/craft_parts/executor/step_handler.py b/craft_parts/executor/step_handler.py index 4a23b8aef..2ea741727 100644 --- a/craft_parts/executor/step_handler.py +++ b/craft_parts/executor/step_handler.py @@ -151,12 +151,12 @@ def _builtin_build(self) -> StepContents: stdout=self._stdout, stderr=self._stderr, ) - except process.ProcessError as procerror: + except process.ProcessError as process_error: raise errors.PluginBuildError( part_name=self._part.name, plugin_name=self._part.plugin_name, - stderr=procerror.result.stderr, - ) + stderr=process_error.result.stderr, + ) from process_error return StepContents() From 9a3af900e8d878f6fe85f2392792a7f06100658d Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Mon, 25 Nov 2024 13:03:04 -0500 Subject: [PATCH 50/56] chore: pr feedback, add method to check returncode --- craft_parts/utils/process.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/craft_parts/utils/process.py b/craft_parts/utils/process.py index c48f80e23..038027cad 100644 --- a/craft_parts/utils/process.py +++ b/craft_parts/utils/process.py @@ -42,6 +42,12 @@ class ProcessResult: 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: @@ -151,11 +157,11 @@ def run( os.close(err_fd) result = ProcessResult( - proc.returncode, out_handler.singular, err_handler.singular, combined + proc.returncode, out_handler.singular, err_handler.singular, combined, command ) if check and result.returncode != 0: - raise ProcessError(result=result, cwd=cwd, command=command) + raise ProcessError(result) return result @@ -180,5 +186,3 @@ class ProcessError(Exception): """ result: ProcessResult - cwd: Path | None - command: Command From 7d8c359a3bf6e4494beb745fce8d2edd3eee07db Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Mon, 25 Nov 2024 14:16:04 -0500 Subject: [PATCH 51/56] chore: pr feedback, use context managers and BytesIO where possible To speed up the program and improve code cleanliness, proper BytesIO objects should be used and the file descriptors they read from should be wrapped in a context manager that guarantees they will be closed properly. This commit addresses both statements. --- craft_parts/utils/process.py | 103 ++++++++++++++++++++--------------- 1 file changed, 58 insertions(+), 45 deletions(-) diff --git a/craft_parts/utils/process.py b/craft_parts/utils/process.py index c535aa346..e509a7add 100644 --- a/craft_parts/utils/process.py +++ b/craft_parts/utils/process.py @@ -20,8 +20,10 @@ import select import subprocess import sys -from collections.abc import Sequence +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, cast @@ -114,50 +116,44 @@ def run( command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd ) - out_fd = _select_stream(stdout, sys.stdout.fileno()) - err_fd = _select_stream(stderr, sys.stderr.fileno()) + with ( + _select_stream(stdout, sys.stdout.fileno()) as out_fd, + _select_stream(stderr, sys.stderr.fileno()) as err_fd, + ): + # stdout and stderr are guaranteed not `None` because we called with `subprocess.PIPE` + proc_stdout = cast(IO[bytes], proc.stdout).fileno() + proc_stderr = cast(IO[bytes], proc.stderr).fileno() - # stdout and stderr are guaranteed not `None` because we called with `subprocess.PIPE` - proc_stdout = cast(IO[bytes], proc.stdout).fileno() - proc_stderr = cast(IO[bytes], proc.stderr).fileno() + os.set_blocking(proc_stdout, False) + os.set_blocking(proc_stderr, False) - os.set_blocking(proc_stdout, False) - os.set_blocking(proc_stderr, False) + out_handler = _ProcessStream(proc_stdout, out_fd) + err_handler = _ProcessStream(proc_stderr, err_fd) - out_handler = _ProcessStream(proc_stdout, out_fd) - err_handler = _ProcessStream(proc_stderr, err_fd) - combined = b"" + with closing(BytesIO()) as combined_io: + while True: + r, _, _ = select.select([proc_stdout, proc_stderr], [], []) - while True: - r, _, _ = select.select([proc_stdout, proc_stderr], [], []) + try: + if proc_stdout in r: + combined_io.write(out_handler.process()) - try: - if proc_stdout in r: - combined += out_handler.process() + if proc_stderr in r: + combined_io.write(err_handler.process()) - if proc_stderr in r: - combined += err_handler.process() + except BlockingIOError: + pass - except BlockingIOError: - pass - - except Exception: - if stdout == DEVNULL: - os.close(out_fd) - if stderr == DEVNULL: - os.close(err_fd) - raise - - if proc.poll() is not None: - break - - if stdout == DEVNULL: - os.close(out_fd) - if stderr == DEVNULL: - os.close(err_fd) + if proc.poll() is not None: + combined = combined_io.getvalue() + break result = ProcessResult( - proc.returncode, out_handler.singular, err_handler.singular, combined, command + proc.returncode, + out_handler.singular, + err_handler.singular, + combined, + command, ) if check and result.returncode != 0: @@ -166,16 +162,33 @@ def run( return result -def _select_stream(stream: Stream, default: int) -> int: - """Translate a ``Stream`` object into a raw FD.""" - if stream == DEVNULL: - return os.open(os.devnull, os.O_WRONLY) - if isinstance(stream, int): - return stream - if stream is None: - return default +@contextmanager +def _select_stream(stream: Stream, default: int) -> Generator[int]: + """Select and return an appropriate raw file descriptor. - return stream.fileno() + 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. + """ + s: int + close = False + if stream == DEVNULL: + s = os.open(os.devnull, os.O_WRONLY) + close = True + elif isinstance(stream, int): + s = stream + elif stream is None: + s = default + else: + s = stream.fileno() + + try: + yield s + finally: + if close: + os.close(s) @dataclass From 55392958b1b59d876d74f0f15c36bf7ff231f04d Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Mon, 25 Nov 2024 15:04:24 -0500 Subject: [PATCH 52/56] chore: pr feedback --- craft_parts/executor/step_handler.py | 6 ++++-- craft_parts/utils/process.py | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/craft_parts/executor/step_handler.py b/craft_parts/executor/step_handler.py index 2ea741727..56ab60951 100644 --- a/craft_parts/executor/step_handler.py +++ b/craft_parts/executor/step_handler.py @@ -122,8 +122,10 @@ def _builtin_pull(self) -> StepContents: stdout=self._stdout, stderr=self._stderr, ) - except process.ProcessError: - 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() diff --git a/craft_parts/utils/process.py b/craft_parts/utils/process.py index e509a7add..8fb7396c8 100644 --- a/craft_parts/utils/process.py +++ b/craft_parts/utils/process.py @@ -156,8 +156,8 @@ def run( command, ) - if check and result.returncode != 0: - raise ProcessError(result) + if check: + result.check_returncode() return result From f42c890897ec0c6aaebc53a2c54c4ac2eb31c9a5 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Tue, 26 Nov 2024 15:29:06 -0500 Subject: [PATCH 53/56] chore: pr feedback, remove attribute declarations --- craft_parts/errors.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/craft_parts/errors.py b/craft_parts/errors.py index 17f95ae30..b31196fdf 100644 --- a/craft_parts/errors.py +++ b/craft_parts/errors.py @@ -39,10 +39,6 @@ class PartsError(Exception): the Craft Parts documentation. """ - brief: str - resolution: str | None = None - doc_slug: str | None = None - def __init__( self, brief: str, From 91c81cedc4d85b56624f49084578295b67b41a8f Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Wed, 27 Nov 2024 10:07:46 -0500 Subject: [PATCH 54/56] feat: better match the functionality of subprocess.run with DEVNULL --- craft_parts/utils/process.py | 102 ++++++++++++++---------- tests/integration/utils/test_process.py | 2 +- 2 files changed, 63 insertions(+), 41 deletions(-) diff --git a/craft_parts/utils/process.py b/craft_parts/utils/process.py index 8fb7396c8..a65fab0ec 100644 --- a/craft_parts/utils/process.py +++ b/craft_parts/utils/process.py @@ -17,7 +17,7 @@ """Utilities for executing subprocesses and handling their stdout and stderr streams.""" import os -import select +import selectors import subprocess import sys from collections.abc import Generator, Sequence @@ -25,7 +25,7 @@ from dataclasses import dataclass from io import BytesIO from pathlib import Path -from typing import IO, TextIO, cast +from typing import IO, TextIO Command = str | Path | Sequence[str | Path] Stream = TextIO | int | None @@ -60,12 +60,18 @@ def __init__(self, read_fd: int, write_fd: int) -> None: 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.""" + """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: @@ -77,6 +83,10 @@ def process(self) -> bytes: return data self._linebuf.extend(data) return b"" + + def _process_nothing(self) -> bytes: + """Do nothing.""" + return b"" def run( @@ -97,10 +107,12 @@ def run( :param cwd: Path to execute in. :type Path | None: :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. + to ``sys.stdout``, and process.DEVNULL can be passed for no printing or + stream capturing. :type Stream: :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. + to ``sys.stderr``, and process.DEVNULL can be passed for no printing or + stream capturing. :type Stream: :param check: If True, a ProcessError exception will be raised if ``command`` returns a non-zero return code. @@ -112,34 +124,37 @@ def run( :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=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd + 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.fileno()) as out_fd, - _select_stream(stderr, sys.stderr.fileno()) as err_fd, + _select_stream(stdout, sys.stdout) as out_fd, + _select_stream(stderr, sys.stderr) as err_fd, ): - # stdout and stderr are guaranteed not `None` because we called with `subprocess.PIPE` - proc_stdout = cast(IO[bytes], proc.stdout).fileno() - proc_stderr = cast(IO[bytes], proc.stderr).fileno() - - os.set_blocking(proc_stdout, False) - os.set_blocking(proc_stderr, False) - - out_handler = _ProcessStream(proc_stdout, out_fd) - err_handler = _ProcessStream(proc_stderr, 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: - r, _, _ = select.select([proc_stdout, proc_stderr], [], []) - try: - if proc_stdout in r: - combined_io.write(out_handler.process()) - - if proc_stderr in r: - combined_io.write(err_handler.process()) + for event, _ in selector.select(): + combined_io.write(event.data.process()) except BlockingIOError: pass @@ -148,10 +163,13 @@ def run( 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, - out_handler.singular, - err_handler.singular, + stdout_res, + stderr_res, combined, command, ) @@ -163,7 +181,7 @@ def run( @contextmanager -def _select_stream(stream: Stream, default: int) -> Generator[int]: +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 @@ -172,23 +190,27 @@ def _select_stream(stream: Stream, default: int) -> Generator[int]: If determining the file handle involves opening our own, this generator handles closing it afterwards. """ - s: int - close = False if stream == DEVNULL: - s = os.open(os.devnull, os.O_WRONLY) - close = True + with open(os.devnull, "wb") as s: + yield s.fileno() elif isinstance(stream, int): - s = stream + yield stream elif stream is None: - s = default + yield default_stream.fileno() else: - s = stream.fileno() - - try: - yield s - finally: - if close: - os.close(s) + 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 diff --git a/tests/integration/utils/test_process.py b/tests/integration/utils/test_process.py index 8f3cb57ea..7dfda2ecf 100644 --- a/tests/integration/utils/test_process.py +++ b/tests/integration/utils/test_process.py @@ -70,4 +70,4 @@ def test_devnull(capfd): result = process.run(["echo", "hello"], stdout=process.DEVNULL) assert capfd.readouterr().out == "" - assert result.stdout == b"hello\n" + assert result.stdout == b"" From d0393129d5a1bd1dcd891b53fcc773e937ef1d98 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Wed, 27 Nov 2024 10:08:42 -0500 Subject: [PATCH 55/56] doc: pr feedback, remove unnecessary docstring content --- craft_parts/utils/process.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/craft_parts/utils/process.py b/craft_parts/utils/process.py index a65fab0ec..3f350ce0c 100644 --- a/craft_parts/utils/process.py +++ b/craft_parts/utils/process.py @@ -103,20 +103,15 @@ def run( accounts and a singular, combined account. :param command: Command to execute. - :type Command: :param cwd: Path to execute in. - :type Path | None: :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. - :type Stream: :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. - :type Stream: :param check: If True, a ProcessError exception will be raised if ``command`` returns a non-zero return code. - :type bool: :raises ProcessError: If process exits with a non-zero return code. :raises OSError: If the specified executable is not found. From d828148ced6cd38b9f76abf12fcebe2d97a166cc Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Wed, 27 Nov 2024 10:22:24 -0500 Subject: [PATCH 56/56] style: run linters --- craft_parts/utils/process.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/craft_parts/utils/process.py b/craft_parts/utils/process.py index 3f350ce0c..76102f5c8 100644 --- a/craft_parts/utils/process.py +++ b/craft_parts/utils/process.py @@ -62,7 +62,7 @@ def __init__(self, read_fd: int, write_fd: int) -> None: # (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] + self.process = self._process_nothing # type: ignore[method-assign] @property def singular(self) -> bytes: @@ -70,8 +70,9 @@ def singular(self) -> bytes: 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.""" + + Does nothing if ``read_fd`` is DEVNULL. + """ data = os.read(self.read_fd, _BUF_SIZE) i = data.rfind(b"\n") if i >= 0: @@ -83,7 +84,7 @@ def process(self) -> bytes: return data self._linebuf.extend(data) return b"" - + def _process_nothing(self) -> bytes: """Do nothing.""" return b"" @@ -128,7 +129,7 @@ def run( if check: result.check_returncode() return result - + proc = subprocess.Popen( command, stdout=DEVNULL if stdout == DEVNULL else subprocess.PIPE, @@ -196,11 +197,13 @@ def _select_stream(stream: Stream, default_stream: TextIO) -> Generator[int]: yield stream.fileno() -def _get_stream_handler(proc_std: IO[bytes] | None, write_fd: int, selector: selectors.BaseSelector) -> _ProcessStream | None: +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)