fix: a step that could not run says so

scripts/check_buildbot_system.py

@@ -152,12 +152,13 @@ def print_report(report: dict) -> None:
     print(f"\nSummary: {', '.join(parts)}")
 
 
-def update_changed(report: dict) -> None:
-    """Refresh entries that have changed."""
+def update_changed(report: dict) -> list[str]:
+    """Refresh entries that have changed; the keys whose refresh failed."""
+    failed: list[str] = []
     for e in report.get("entries", []):
         if e["status"] == "UPDATED" and e.get("key"):
             log.info("refreshing %s ...", e["key"])
-            subprocess.run(
+            result = subprocess.run(
                 [
                     sys.executable,
                     "scripts/refresh_data_dirs.py",
@@ -167,6 +168,10 @@ def update_changed(report: dict) -> None:
                 ],
                 check=False,
             )
+            if result.returncode != 0:
+                log.error("refresh of %s failed (exit %d)", e["key"], result.returncode)
+                failed.append(e["key"])
+    return failed
 
 
 def main() -> None:
@@ -194,7 +199,10 @@ def main() -> None:
         print_report(report)
 
     if args.update:
-        update_changed(report)
+        failed = update_changed(report)
+        if failed:
+            print(f"ERROR: refresh failed for {', '.join(failed)}", file=sys.stderr)
+            sys.exit(1)
 
     # Unreachable upstream means the freshness question was not answered, and
     # a zero exit says it was answered "fresh".

scripts/validate_pr.py

@@ -228,33 +228,42 @@ def validate_file(
 
 
 def get_changed_files() -> list[str]:
-    """Get list of changed files in current PR/branch using git."""
-    try:
+    """BIOS files the branch changes against origin's main line.
+
+    A base git cannot resolve is skipped; when none resolves the staged
+    changes are read and said so. A git that fails outright is an error,
+    never "nothing changed": a clone whose remote is not called origin
+    used to answer that no BIOS had changed while the branch committed one.
+    """
+    compared = False
     for base in ("main", "master", "v2"):
-            try:
         result = subprocess.run(
             ["git", "diff", "--name-only", f"origin/{base}...HEAD"],
             capture_output=True,
             text=True,
-                    check=True,
+            check=False,
         )
-                files = [
-                    f
-                    for f in result.stdout.strip().split("\n")
-                    if f.startswith("bios/")
-                ]
+        if result.returncode != 0:
+            continue
+        compared = True
+        files = [f for f in result.stdout.strip().split("\n") if f.startswith("bios/")]
         if files:
             return files
-            except subprocess.CalledProcessError:
-                continue
-    except (subprocess.CalledProcessError, OSError):
-        pass
-
+    if compared:
+        return []
+    print(
+        "no origin/main, origin/master or origin/v2 to compare against; "
+        "reading the staged changes only",
+        file=sys.stderr,
+    )
     result = subprocess.run(
         ["git", "diff", "--cached", "--name-only"],
         capture_output=True,
         text=True,
+        check=False,
     )
+    if result.returncode != 0:
+        raise RuntimeError(f"git diff --cached failed: {result.stderr.strip()}")
     return [f for f in result.stdout.strip().split("\n") if f.startswith("bios/") and f]
 
 
@@ -274,7 +283,11 @@ def main():
 
     files = args.files
     if args.changed:
+        try:
             files = get_changed_files()
+        except (RuntimeError, OSError) as exc:
+            print(f"Error: {exc}", file=sys.stderr)
+            sys.exit(2)
         if not files:
             print("No changed BIOS files detected")
             return

tests/test_refresh_failures.py

@@ -0,0 +1,67 @@
+"""A step that could not do its job says so with its exit code.
+
+validate_pr --changed answered "No changed BIOS files detected" and exited
+0 when git itself had failed (no origin, or no repository), and
+check_buildbot_system --update exited 0 after a refresh it launched had
+failed, the cache left at the old version.
+"""
+
+from __future__ import annotations
+
+import subprocess
+import sys
+import tempfile
+import unittest
+from pathlib import Path
+from unittest import mock
+
+REPO_ROOT = Path(__file__).resolve().parent.parent
+sys.path.insert(0, str(REPO_ROOT / "scripts"))
+
+import check_buildbot_system  # noqa: E402
+
+
+class GitThatCannotAnswer(unittest.TestCase):
+    def test_outside_a_repository_is_an_error_not_an_empty_change(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            proc = subprocess.run(
+                [sys.executable, str(REPO_ROOT / "scripts" / "validate_pr.py"), "--changed"],
+                cwd=tmp, capture_output=True, text=True, timeout=60, check=False,
+            )
+        self.assertEqual(proc.returncode, 2, proc.stdout + proc.stderr)
+        self.assertNotIn("No changed BIOS files detected", proc.stdout)
+
+    def test_a_clone_without_origin_reads_the_index_and_says_so(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            subprocess.run(["git", "init", "-q", tmp], check=True)
+            Path(tmp, "bios").mkdir()
+            Path(tmp, "bios", "x.bin").write_bytes(b"x")
+            subprocess.run(["git", "-C", tmp, "add", "bios/x.bin"], check=True)
+            proc = subprocess.run(
+                [sys.executable, "-c", (
+                    f"import sys; sys.path.insert(0, {str(REPO_ROOT / 'scripts')!r}); "
+                    "import validate_pr; print(validate_pr.get_changed_files())"
+                )],
+                cwd=tmp, capture_output=True, text=True, timeout=60, check=False,
+            )
+        self.assertEqual(proc.returncode, 0, proc.stderr)
+        self.assertIn("bios/x.bin", proc.stdout)
+        self.assertIn("staged changes only", proc.stderr)
+
+
+class RefreshFailuresAreFailures(unittest.TestCase):
+    def test_a_failed_refresh_is_returned_and_exits_nonzero(self):
+        report = {"entries": [{"status": "UPDATED", "key": "dolphin-sys"},
+                              {"status": "OK", "key": "ppsspp-assets"}]}
+        with mock.patch.object(
+            check_buildbot_system.subprocess, "run",
+            return_value=subprocess.CompletedProcess([], 1),
+        ):
+            self.assertEqual(check_buildbot_system.update_changed(report), ["dolphin-sys"])
+        source = (REPO_ROOT / "scripts" / "check_buildbot_system.py").read_text(encoding="utf-8")
+        self.assertIn("failed = update_changed(report)", source)
+        self.assertIn("sys.exit(1)", source[source.index("failed = update_changed(report)"):])
+
+
+if __name__ == "__main__":
+    unittest.main()