print better errors when external commands fail

helps to better understand: #361

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()

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)

nixpkgs_review/errors.py

@@ -0,0 +1,3 @@
+class NixpkgsReviewError(Exception):
+    """Base class for exceptions in this module."""
+    pass

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
 
 

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
 
 

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: