diff --git a/scripts/check_buildbot_system.py b/scripts/check_buildbot_system.py index 273f0c67..3346f98a 100644 --- a/scripts/check_buildbot_system.py +++ b/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". diff --git a/scripts/validate_pr.py b/scripts/validate_pr.py index 0a9f9d92..22a0f743 100644 --- a/scripts/validate_pr.py +++ b/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: - 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, - ) - 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 + """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"): + result = subprocess.run( + ["git", "diff", "--name-only", f"origin/{base}...HEAD"], + capture_output=True, + text=True, + check=False, + ) + 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 + 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: - files = get_changed_files() + 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 diff --git a/tests/test_refresh_failures.py b/tests/test_refresh_failures.py new file mode 100644 index 00000000..61367320 --- /dev/null +++ b/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()