From fba62c7989dee577913910b8607329dc7e1a3d7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Thu, 14 Sep 2023 20:10:00 +0200 Subject: [PATCH] print better errors when external commands fail helps to better understand: https://github.com/Mic92/nixpkgs-review/issues/361 --- nixpkgs_review/builddir.py | 6 +++++- nixpkgs_review/cli/pr.py | 10 ++++++++-- nixpkgs_review/errors.py | 3 +++ nixpkgs_review/nix.py | 21 +++++++-------------- nixpkgs_review/review.py | 31 ++++++++++++++++++++++--------- nixpkgs_review/utils.py | 4 ++-- 6 files changed, 47 insertions(+), 28 deletions(-) create mode 100644 nixpkgs_review/errors.py diff --git a/nixpkgs_review/builddir.py b/nixpkgs_review/builddir.py index e4fd9d5..af055e4 100644 --- a/nixpkgs_review/builddir.py +++ b/nixpkgs_review/builddir.py @@ -73,6 +73,10 @@ def __exit__(self, exc_type: Any, exc_value: Any, traceback: Any) -> None: os.environ.update(self.environ) with DisableKeyboardInterrupt(): - sh(["git", "worktree", "remove", "-f", str(self.worktree_dir)]) + res = sh(["git", "worktree", "remove", "-f", str(self.worktree_dir)]) + if res.returncode != 0: + warn( + f"Failed to remove worktree at {self.worktree_dir}. Please remove it manually. Git failed with: {res.returncode}" + ) self.overlay.cleanup() diff --git a/nixpkgs_review/cli/pr.py b/nixpkgs_review/cli/pr.py index 272c376..a14fb2a 100644 --- a/nixpkgs_review/cli/pr.py +++ b/nixpkgs_review/cli/pr.py @@ -4,6 +4,8 @@ import sys from contextlib import ExitStack +from nixpkgs_review.errors import NixpkgsReviewError + from ..allow import AllowedFeatures from ..builddir import Builddir from ..buildenv import Buildenv @@ -45,9 +47,11 @@ def pr_command(args: argparse.Namespace) -> str: allow = AllowedFeatures(args.allow) + builddir = None with Buildenv( allow.aliases, args.extra_nixpkgs_config ) as nixpkgs_config, ExitStack() as stack: + review = None for pr in prs: builddir = stack.enter_context(Builddir(f"pr-{pr}")) try: @@ -72,12 +76,14 @@ def pr_command(args: argparse.Namespace) -> str: extra_nixpkgs_config=args.extra_nixpkgs_config, ) contexts.append((pr, builddir.path, review.build_pr(pr))) - except subprocess.CalledProcessError: - warn(f"https://github.com/NixOS/nixpkgs/pull/{pr} failed to build") + except NixpkgsReviewError as e: + warn(f"https://github.com/NixOS/nixpkgs/pull/{pr} failed to build: {e}") + assert review is not None for pr, path, attrs in contexts: review.start_review(attrs, path, pr, args.post_result, args.print_result) if len(contexts) != len(prs): sys.exit(1) + assert builddir is not None return str(builddir.path) diff --git a/nixpkgs_review/errors.py b/nixpkgs_review/errors.py new file mode 100644 index 0000000..92e4443 --- /dev/null +++ b/nixpkgs_review/errors.py @@ -0,0 +1,3 @@ +class NixpkgsReviewError(Exception): + """Base class for exceptions in this module.""" + pass diff --git a/nixpkgs_review/nix.py b/nixpkgs_review/nix.py index fb7a485..61eb628 100644 --- a/nixpkgs_review/nix.py +++ b/nixpkgs_review/nix.py @@ -9,6 +9,8 @@ from tempfile import NamedTemporaryFile from typing import Any +from nixpkgs_review.errors import NixpkgsReviewError + from .allow import AllowedFeatures from .utils import ROOT, escape_attr, info, sh, warn @@ -66,7 +68,7 @@ def nix_shell( args = [nix_shell, str(shell), "--nix-path", nix_path] if run: args.extend(["--run", run]) - sh(args, cwd=cache_directory, check=False) + sh(args, cwd=cache_directory) def _nix_shell_sandbox( @@ -222,16 +224,10 @@ def nix_eval( f"(import {eval_script} {{ attr-json = {attr_json.name}; }})", ] - try: - nix_eval = subprocess.run( - cmd, check=True, stdout=subprocess.PIPE, text=True - ) - except subprocess.CalledProcessError: - warn( - f"{' '.join(cmd)} failed to run, {attr_json.name} was stored inspection" - ) + nix_eval = subprocess.run(cmd, stdout=subprocess.PIPE, text=True) + if nix_eval.returncode != 0: delete = False - raise + raise NixpkgsReviewError(f"{' '.join(cmd)} failed to run, {attr_json.name} was stored inspection") return _nix_eval_filter(json.loads(nix_eval.stdout)) finally: @@ -293,10 +289,7 @@ def nix_build( str(build), ] + shlex.split(args) - try: - sh(command) - except subprocess.CalledProcessError: - pass + sh(command) return attrs diff --git a/nixpkgs_review/review.py b/nixpkgs_review/review.py index 337ea28..393a13c 100644 --- a/nixpkgs_review/review.py +++ b/nixpkgs_review/review.py @@ -10,6 +10,7 @@ from re import Pattern from typing import IO +from .errors import NixpkgsReviewError from .allow import AllowedFeatures from .builddir import Builddir from .github import GithubClient @@ -123,7 +124,9 @@ def worktree_dir(self) -> str: return str(self.builddir.worktree_dir) def git_merge(self, commit: str) -> None: - sh(["git", "merge", "--no-commit", "--no-ff", commit], cwd=self.worktree_dir()) + res = sh(["git", "merge", "--no-commit", "--no-ff", commit], cwd=self.worktree_dir()) + if res.returncode != 0: + raise NixpkgsReviewError(f"Failed to merge {commit} into {self.worktree_dir()}. git merge failed with exit code {res.returncode}") def apply_unstaged(self, staged: bool = False) -> None: args = ["git", "--no-pager", "diff", "--no-ext-diff"] @@ -175,7 +178,9 @@ def build_commit( return self.build(changed_attrs, self.build_args) def git_worktree(self, commit: str) -> None: - sh(["git", "worktree", "add", self.worktree_dir(), commit]) + res = sh(["git", "worktree", "add", self.worktree_dir(), commit]) + if res.returncode != 0: + raise NixpkgsReviewError(f"Failed to add worktree for {commit} in {self.worktree_dir()}. git worktree failed with exit code {res.returncode}") def checkout_pr(self, base_rev: str, pr_rev: str) -> None: if self.checkout == CheckoutOption.MERGE: @@ -232,10 +237,13 @@ def build_pr(self, pr_number: int) -> list[Attr]: else: run = subprocess.run( ["git", "merge-base", merge_rev, pr_rev], - check=True, stdout=subprocess.PIPE, text=True, ) + if run.returncode != 0: + raise NixpkgsReviewError( + f"Failed to get the merge base of {merge_rev} with PR {pr_rev}" + ) base_rev = run.stdout.strip() if packages_per_system is None: @@ -379,7 +387,9 @@ def list_packages( cmd.append("--meta") info("$ " + " ".join(cmd)) with tempfile.NamedTemporaryFile(mode="w") as tmp: - subprocess.run(cmd, stdout=tmp, check=True) + res = subprocess.run(cmd, stdout=tmp) + if res.returncode != 0: + raise NixpkgsReviewError("Failed to list packages: nix-env failed with exit code %d" % res.returncode) tmp.flush() with open(tmp.name) as f: return parse_packages_xml(f) @@ -501,13 +511,16 @@ def fetch_refs(repo: str, *refs: str) -> list[str]: cmd = ["git", "-c", "fetch.prune=false", "fetch", "--no-tags", "--force", repo] for i, ref in enumerate(refs): cmd.append(f"{ref}:refs/nixpkgs-review/{i}") - sh(cmd) + res = sh(cmd) + if res.returncode != 0: + raise NixpkgsReviewError(f"Failed to fetch {refs} from {repo}. git fetch failed with exit code {res.returncode}") shas = [] for i, ref in enumerate(refs): - out = subprocess.check_output( - ["git", "rev-parse", "--verify", f"refs/nixpkgs-review/{i}"], text=True - ) - shas.append(out.strip()) + cmd = ["git", "rev-parse", "--verify", f"refs/nixpkgs-review/{i}"] + out = subprocess.run(cmd, text=True, stdout=subprocess.PIPE) + if out.returncode != 0: + raise NixpkgsReviewError(f"Failed to fetch {ref} from {repo} with command: {''.join(cmd)}") + shas.append(out.stdout.strip()) return shas diff --git a/nixpkgs_review/utils.py b/nixpkgs_review/utils.py index a149033..0f98a7b 100644 --- a/nixpkgs_review/utils.py +++ b/nixpkgs_review/utils.py @@ -27,10 +27,10 @@ def wrapper(text: str) -> None: def sh( - command: list[str], cwd: Path | str | None = None, check: bool = True + command: list[str], cwd: Path | str | None = None ) -> "subprocess.CompletedProcess[str]": info("$ " + " ".join(command)) - return subprocess.run(command, cwd=cwd, check=check, text=True) + return subprocess.run(command, cwd=cwd, text=True) def verify_commit_hash(commit: str) -> str: