From: Daan De Meyer Date: Tue, 18 Apr 2023 13:09:58 +0000 (+0200) Subject: Rework exception handling X-Git-Tag: v15~238 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e25d746f9d7668edc5134cde4ec381c8b2da0136;p=thirdparty%2Fmkosi.git Rework exception handling Let's always use regular exceptions and stop outputting stack traces on exceptions by default. To get a stacktrace, users can run mkosi with --debug run and we will output the usual stacktrace. --- diff --git a/mkosi/__init__.py b/mkosi/__init__.py index f79facdb6..a764c3f33 100644 --- a/mkosi/__init__.py +++ b/mkosi/__init__.py @@ -42,14 +42,7 @@ from mkosi.backend import ( tmp_dir, ) from mkosi.install import add_dropin_config_from_resource, copy_path, flock -from mkosi.log import ( - ARG_DEBUG, - MkosiException, - MkosiNotSupportedException, - MkosiPrinter, - die, - warn, -) +from mkosi.log import ARG_DEBUG, MkosiPrinter, die, warn from mkosi.manifest import GenericVersion, Manifest from mkosi.mounts import dissect_and_mount, mount_overlay, scandir_recursive from mkosi.pager import page @@ -1130,10 +1123,10 @@ def load_args(args: argparse.Namespace) -> MkosiConfig: OutputFormat.tar, OutputFormat.cpio, ): - die("Directory, subvolume, tar, cpio, and plain squashfs images cannot be booted in qemu.", MkosiNotSupportedException) + die("Directory, subvolume, tar, cpio, and plain squashfs images cannot be booted in qemu.") if shutil.which("bsdtar") and args.distribution == Distribution.openmandriva and args.tar_strip_selinux_context: - die("Sorry, bsdtar on OpenMandriva is incompatible with --tar-strip-selinux-context", MkosiNotSupportedException) + die("Sorry, bsdtar on OpenMandriva is incompatible with --tar-strip-selinux-context") if args.cache_dir: args.cache_dir = args.cache_dir / f"{args.distribution}~{args.release}" @@ -1224,13 +1217,13 @@ def load_args(args: argparse.Namespace) -> MkosiConfig: if args.verb in (Verb.shell, Verb.boot): opname = "acquire shell" if args.verb == Verb.shell else "boot" if args.output_format in (OutputFormat.tar, OutputFormat.cpio): - die(f"Sorry, can't {opname} with a {args.output_format} archive.", MkosiNotSupportedException) + die(f"Sorry, can't {opname} with a {args.output_format} archive.") if should_compress_output(args): - die(f"Sorry, can't {opname} with a compressed image.", MkosiNotSupportedException) + die(f"Sorry, can't {opname} with a compressed image.") if args.verb == Verb.qemu: if not args.output_format == OutputFormat.disk: - die("Sorry, can't boot non-disk images with qemu.", MkosiNotSupportedException) + die("Sorry, can't boot non-disk images with qemu.") if args.repo_dirs and not (is_dnf_distribution(args.distribution) or args.distribution == Distribution.arch): die("--repo-dir is only supported on DNF based distributions and Arch") @@ -1930,15 +1923,6 @@ def check_root() -> None: die("Must be invoked as root.") -@contextlib.contextmanager -def suppress_stacktrace() -> Iterator[None]: - try: - yield - except subprocess.CalledProcessError as e: - # MkosiException is silenced in main() so it doesn't print a stacktrace. - raise MkosiException() from e - - def machine_name(config: Union[MkosiConfig, argparse.Namespace]) -> str: return config.image_id or config.output.with_suffix("").name.partition("_")[0] @@ -2480,15 +2464,14 @@ def run_verb(config: MkosiConfig) -> None: if config.auto_bump: bump_image_version(config) - with suppress_stacktrace(): - if config.verb in (Verb.shell, Verb.boot): - run_shell(config) + if config.verb in (Verb.shell, Verb.boot): + run_shell(config) - if config.verb == Verb.qemu: - run_qemu(config) + if config.verb == Verb.qemu: + run_qemu(config) - if config.verb == Verb.ssh: - run_ssh(config) + if config.verb == Verb.ssh: + run_ssh(config) if config.verb == Verb.serve: run_serve(config) diff --git a/mkosi/__main__.py b/mkosi/__main__.py index 52dddac67..8ca7d53a8 100644 --- a/mkosi/__main__.py +++ b/mkosi/__main__.py @@ -3,13 +3,13 @@ import contextlib import os +import subprocess import sys from collections.abc import Iterator -from subprocess import CalledProcessError from mkosi import load_args, run_verb from mkosi.config import MkosiConfigParser -from mkosi.log import MkosiException, die +from mkosi.log import ARG_DEBUG, MkosiPrinter, die from mkosi.run import excepthook @@ -17,10 +17,28 @@ from mkosi.run import excepthook def propagate_failed_return() -> Iterator[None]: try: yield - except MkosiException as e: - cause = e.__cause__ - if cause and isinstance(cause, CalledProcessError): - sys.exit(cause.returncode) + except SystemExit as e: + if ARG_DEBUG: + raise e + + sys.exit(e.code) + except KeyboardInterrupt as e: + if ARG_DEBUG: + raise e + + MkosiPrinter.error("Interrupted") + sys.exit(1) + except subprocess.CalledProcessError as e: + if ARG_DEBUG: + raise e + + # We always log when subprocess.CalledProcessError is raised, so we don't log again here. + sys.exit(e.returncode) + except Exception as e: + if ARG_DEBUG: + raise e + + MkosiPrinter.error("Error: mkosi failed because of an exception, rerun mkosi with --debug run to get more information") sys.exit(1) diff --git a/mkosi/backend.py b/mkosi/backend.py index 8f5c1adb4..ad3c7a1c8 100644 --- a/mkosi/backend.py +++ b/mkosi/backend.py @@ -21,7 +21,7 @@ from pathlib import Path from typing import Any, Callable, Optional, TypeVar, Union from mkosi.distributions import DistributionInstaller -from mkosi.log import MkosiException, die +from mkosi.log import die T = TypeVar("T") V = TypeVar("V") @@ -428,7 +428,7 @@ def patch_file(filepath: Path, line_rewriter: Callable[[str], str]) -> None: def safe_tar_extract(tar: tarfile.TarFile, path: Path=Path("."), *, numeric_owner: bool=False) -> None: """Extract a tar without CVE-2007-4559. - Throws a MkosiException if a member of the tar resolves to a path that would + Throws an exception if a member of the tar resolves to a path that would be outside of the passed in target path. Omits the member argument from TarFile.extractall, since we don't need it at @@ -446,7 +446,7 @@ def safe_tar_extract(tar: tarfile.TarFile, path: Path=Path("."), *, numeric_owne target.resolve().relative_to(path) members += [member] except ValueError as e: - raise MkosiException(f"Attempted path traversal in tar file {tar.name!r}") from e + die(f"Attempted path traversal in tar file {tar.name!r}", e) tar.extractall(path, members=members, numeric_owner=numeric_owner) diff --git a/mkosi/distributions/gentoo.py b/mkosi/distributions/gentoo.py index 3a389b536..30a699c15 100644 --- a/mkosi/distributions/gentoo.py +++ b/mkosi/distributions/gentoo.py @@ -12,7 +12,7 @@ from textwrap import dedent from mkosi.backend import MkosiState, safe_tar_extract from mkosi.distributions import DistributionInstaller from mkosi.install import copy_path, flock -from mkosi.log import ARG_DEBUG, MkosiException, MkosiPrinter, complete_step, die +from mkosi.log import ARG_DEBUG, MkosiPrinter, complete_step, die from mkosi.remove import unlink_try_hard from mkosi.run import run_workspace_command @@ -139,7 +139,7 @@ class Gentoo: except ImportError as e: MkosiPrinter.warn(NEED_PORTAGE_MSG) MkosiPrinter.info(PORTAGE_INSTALL_INSTRUCTIONS) - raise MkosiException(e) + raise e return dict(profile_path=PROFILE_PATH, custom_profile_path=CUSTOM_PROFILE_PATH, diff --git a/mkosi/log.py b/mkosi/log.py index 9101976e6..9db56f77e 100644 --- a/mkosi/log.py +++ b/mkosi/log.py @@ -6,17 +6,9 @@ from typing import Any, Iterator, NoReturn, Optional ARG_DEBUG: set[str] = set() -class MkosiException(Exception): - """Leads to sys.exit""" - - -class MkosiNotSupportedException(MkosiException): - """Leads to sys.exit when an invalid combination of parsed arguments happens""" - - -def die(message: str, exception: type[MkosiException] = MkosiException) -> NoReturn: +def die(message: str, exception: Optional[Exception] = None) -> NoReturn: MkosiPrinter.error(f"Error: {message}") - raise exception(message) + raise exception or RuntimeError(message) def warn(message: str) -> None: diff --git a/mkosi/run.py b/mkosi/run.py index 6fcb9dc5e..c00ffa151 100644 --- a/mkosi/run.py +++ b/mkosi/run.py @@ -187,8 +187,7 @@ def fork_and_wait(target: Callable[[], T]) -> T: return result - -def run( +def _run( cmdline: Sequence[PathString], check: bool = True, stdout: _FILE = None, @@ -222,8 +221,22 @@ def run( try: return subprocess.run(cmdline, check=check, stdout=stdout, stderr=stderr, env=env, **kwargs, preexec_fn=foreground, close_fds=False) - except FileNotFoundError: - die(f"{cmdline[0]} not found in PATH.") + except FileNotFoundError as e: + die(f"{cmdline[0]} not found in PATH.", e) + + +def run( + cmdline: Sequence[PathString], + check: bool = True, + stdout: _FILE = None, + stderr: _FILE = None, + env: Mapping[str, PathString] = {}, + **kwargs: Any, +) -> CompletedProcess: + try: + return _run(cmdline, check, stdout, stderr, env, **kwargs) + except subprocess.CalledProcessError as e: + die(f"\"{shlex.join(str(s) for s in cmdline)}\" returned non-zero exit code {e.returncode}.", e) def spawn( @@ -245,6 +258,8 @@ def spawn( return subprocess.Popen(cmdline, stdout=stdout, stderr=stderr, **kwargs, preexec_fn=foreground) except FileNotFoundError: die(f"{cmdline[0]} not found in PATH.") + except subprocess.CalledProcessError as e: + die(f"\"{shlex.join(str(s) for s in cmdline)}\" returned non-zero exit code {e.returncode}.", e) def run_with_apivfs( @@ -290,13 +305,12 @@ def run_with_apivfs( template = f"chmod 1777 {state.root / 'tmp'} {state.root / 'var/tmp'} {state.root / 'dev/shm'} && exec {{}} || exit $?" try: - return run([*cmdline, template.format(shlex.join(str(s) for s in cmd))], + return _run([*cmdline, template.format(shlex.join(str(s) for s in cmd))], text=True, stdout=stdout, stderr=stderr, env=env) except subprocess.CalledProcessError as e: if "run" in ARG_DEBUG: - run([*cmdline, template.format("sh")], check=False, env=env) - die(f"\"{shlex.join(str(s) for s in cmd)}\" returned non-zero exit code {e.returncode}.") - + _run([*cmdline, template.format("sh")], check=False, env=env) + raise e def run_workspace_command( state: MkosiState, @@ -348,12 +362,12 @@ def run_workspace_command( template = "chmod 1777 /tmp /var/tmp /dev/shm && PATH=$PATH:/usr/bin:/usr/sbin exec {} || exit $?" try: - return run([*cmdline, template.format(shlex.join(str(s) for s in cmd))], + return _run([*cmdline, template.format(shlex.join(str(s) for s in cmd))], text=True, stdout=stdout, env=env) except subprocess.CalledProcessError as e: if "run" in ARG_DEBUG: - run([*cmdline, template.format("sh")], check=False, env=env) - die(f"\"{shlex.join(str(s) for s in cmd)}\" returned non-zero exit code {e.returncode}.") + _run([*cmdline, template.format("sh")], check=False, env=env) + raise e finally: if state.workspace.joinpath("resolv.conf").is_symlink(): resolve.unlink(missing_ok=True) diff --git a/tests/test_backend.py b/tests/test_backend.py index be8b966bd..70108ed6d 100644 --- a/tests/test_backend.py +++ b/tests/test_backend.py @@ -14,8 +14,6 @@ from mkosi.backend import ( set_umask, strip_suffixes, ) -from mkosi.log import MkosiException - def test_distribution() -> None: assert Distribution.fedora.package_type == PackageType.rpm @@ -55,7 +53,7 @@ def test_safe_tar_extract(tmp_path: Path) -> None: assert (safe_target / name).exists() evil_target = tmp_path / "evil_target" - with pytest.raises(MkosiException): + with pytest.raises(ValueError): with tarfile.TarFile.open(evil_tar) as t: safe_tar_extract(t, evil_target) assert not (evil_target / name).exists() diff --git a/tests/test_parse_load_args.py b/tests/test_parse_load_args.py index d20523102..1328b8053 100644 --- a/tests/test_parse_load_args.py +++ b/tests/test_parse_load_args.py @@ -11,7 +11,6 @@ import pytest import mkosi from mkosi.backend import Distribution, MkosiConfig, Verb -from mkosi.log import MkosiException from mkosi.config import MkosiConfigParser @@ -77,13 +76,13 @@ def test_parse_config_files_filter() -> None: def test_shell_boot() -> None: with cd_temp_dir(): - with pytest.raises(MkosiException, match=".boot.*tar"): + with pytest.raises(RuntimeError, match=".boot.*tar"): parse(["--format", "tar", "boot"]) - with pytest.raises(MkosiException, match=".boot.*cpio"): + with pytest.raises(RuntimeError, match=".boot.*cpio"): parse(["--format", "cpio", "boot"]) - with pytest.raises(MkosiException, match=".boot.*compressed" ): + with pytest.raises(RuntimeError, match=".boot.*compressed" ): parse(["--format", "disk", "--compress-output=yes", "boot"])