From 6d97fbe1278b1b601d219a67adedd6e403a4264d Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Mon, 7 Sep 2026 05:39:50 +0200 Subject: [PATCH] fix: stop a check that cannot answer exiting zero --- scripts/check_buildbot_system.py | 5 ++ scripts/common.py | 12 +++++ scripts/generate_pack.py | 5 ++ scripts/restore_large_files.py | 22 ++++++-- tests/test_audit_regressions.py | 92 ++++++++++++++++++++++++++++++-- 5 files changed, 129 insertions(+), 7 deletions(-) diff --git a/scripts/check_buildbot_system.py b/scripts/check_buildbot_system.py index 4743cba7..273f0c67 100644 --- a/scripts/check_buildbot_system.py +++ b/scripts/check_buildbot_system.py @@ -196,6 +196,11 @@ def main() -> None: if args.update: update_changed(report) + # Unreachable upstream means the freshness question was not answered, and + # a zero exit says it was answered "fresh". + if report.get("error"): + raise SystemExit(1) + if __name__ == "__main__": main() diff --git a/scripts/common.py b/scripts/common.py index eefa36e1..6710b22e 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -11,6 +11,7 @@ import functools import json import os import re +import sys import zipfile from pathlib import Path @@ -930,10 +931,21 @@ def load_emulator_profiles( try: import yaml except ImportError: + # Every consumer reads {} as "this repo documents no emulator", which + # is what a broken install looks like from the outside: zero coverage, + # no gap, nothing to fix. + print( + "warning: pyyaml is not installed, no emulator profile was loaded", + file=sys.stderr, + ) return {} profiles = {} emu_path = Path(emulators_dir) if not emu_path.exists(): + print( + f"warning: no emulator profile directory at {emulators_dir}", + file=sys.stderr, + ) return profiles for f in sorted(emu_path.glob("*.yml")): if f.name.endswith(".old.yml"): diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 4491cc83..048867c7 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -2134,6 +2134,11 @@ def _run_verify_packs(args): break if not zip_path: print(f" {platform_name}: SKIP (no pack in {args.output_dir})") + # Naming a platform is asking about its pack. Answering SKIP and + # exiting 0 says the pack passed; with --all, a platform whose pack + # was not built is genuinely out of scope. + if args.platform: + all_ok = False continue if _narrows_contents(os.path.basename(zip_path)): diff --git a/scripts/restore_large_files.py b/scripts/restore_large_files.py index 0945a39d..194397ca 100644 --- a/scripts/restore_large_files.py +++ b/scripts/restore_large_files.py @@ -52,27 +52,39 @@ def index_cache(cache_dir: str) -> dict[str, str]: return index -def restore(cache_dir: str, db_path: str, gitignore: str) -> int: +def restore( + cache_dir: str, db_path: str, gitignore: str +) -> tuple[int, list[str]]: if not os.path.isdir(cache_dir): print(f"No cache at {cache_dir}, nothing to restore") - return 0 + return 0, [] ignored = gitignored_paths(gitignore) index = index_cache(cache_dir) db = load_database(db_path) restored = 0 + unsatisfied: list[str] = [] for sha1, entry in db.get("files", {}).items(): path = entry.get("path", "") if path not in ignored or os.path.exists(path): continue source = index.get(sha1) if not source: + unsatisfied.append(path) continue os.makedirs(os.path.dirname(path), exist_ok=True) shutil.copy2(source, path) print(f"Restored: {path}") restored += 1 print(f"Total: {restored} files restored") - return restored + if unsatisfied: + # Every consumer downstream resolves against the disk, so a path the + # cache cannot supply is not a smaller restore: it drops entries from + # the manifest and inflates the missing count the README publishes. + print(f"Unsatisfied: {len(unsatisfied)} declared paths the cache " + "cannot supply", file=sys.stderr) + for path in sorted(unsatisfied)[:10]: + print(f" {path}", file=sys.stderr) + return restored, unsatisfied def main() -> None: @@ -81,7 +93,9 @@ def main() -> None: parser.add_argument("--db", default="database.json") parser.add_argument("--gitignore", default=".gitignore") args = parser.parse_args() - restore(args.cache, args.db, args.gitignore) + _restored, unsatisfied = restore(args.cache, args.db, args.gitignore) + if unsatisfied: + raise SystemExit(1) if __name__ == "__main__": diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index c9b7c432..f4a6ee35 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -10,6 +10,7 @@ import json import os import re import stat +import subprocess import sys import tempfile import unittest @@ -1078,10 +1079,16 @@ class CheckoutCompletenessRegressions(unittest.TestCase): cwd = os.getcwd() os.chdir(root) try: - self.assertEqual(restore(str(cache), "database.json", ".gitignore"), 1) + restored, unsatisfied = restore( + str(cache), "database.json", ".gitignore" + ) + self.assertEqual((restored, unsatisfied), (1, [])) self.assertEqual((root / "bios/Sony/big.pup").read_bytes(), payload) # A path already in the checkout is never overwritten. - self.assertEqual(restore(str(cache), "database.json", ".gitignore"), 0) + restored, unsatisfied = restore( + str(cache), "database.json", ".gitignore" + ) + self.assertEqual((restored, unsatisfied), (0, [])) finally: os.chdir(cwd) @@ -1103,7 +1110,10 @@ class CheckoutCompletenessRegressions(unittest.TestCase): cwd = os.getcwd() os.chdir(root) try: - self.assertEqual(restore(str(cache), "database.json", ".gitignore"), 0) + restored, unsatisfied = restore( + str(cache), "database.json", ".gitignore" + ) + self.assertEqual((restored, unsatisfied), (0, [])) self.assertFalse((root / "bios/tracked.bin").exists()) finally: os.chdir(cwd) @@ -1275,6 +1285,82 @@ class PipelineReportsWhatItDid(unittest.TestCase): stale = re.findall(r'results\["(\w+)"\] = True', source) self.assertEqual(stale, [], f"steps still claiming OK when skipped: {stale}") +class ACheckThatCannotAnswerDoesNotPass(unittest.TestCase): + """Exiting zero says the question was answered and the answer was yes. + + Four scripts said that without answering: a refresh that reached no + remote, a freshness check whose upstream was unreachable, a restore whose + cache could not supply a declared path, and a pack verification asked + about one platform whose pack was not there. + """ + + def test_naming_a_platform_with_no_pack_is_not_a_pass(self): + with tempfile.TemporaryDirectory(dir=TMP_ROOT) as directory: + named = subprocess.run( + [sys.executable, "scripts/generate_pack.py", "--platform", + "retroarch", "--verify-packs", "--output-dir", directory], + capture_output=True, text=True, cwd=str(ROOT), timeout=300, + ) + self.assertNotEqual( + named.returncode, 0, + "a named platform with no pack reported success:\n" + + named.stdout + named.stderr, + ) + # --all is a sweep: a platform nobody built is out of scope. + swept = subprocess.run( + [sys.executable, "scripts/generate_pack.py", "--all", + "--verify-packs", "--output-dir", directory], + capture_output=True, text=True, cwd=str(ROOT), timeout=300, + ) + self.assertEqual(swept.returncode, 0, swept.stdout + swept.stderr) + + def test_an_unsatisfiable_declared_path_is_reported(self): + from scripts.restore_large_files import restore + + with tempfile.TemporaryDirectory(dir=TMP_ROOT) as directory: + root = Path(directory) + cache = root / "cache" + cache.mkdir() + (root / ".gitignore").write_text("bios/absent.bin\n", encoding="utf-8") + (root / "database.json").write_text( + json.dumps({"files": {"a" * 40: {"path": "bios/absent.bin"}}}), + encoding="utf-8", + ) + cwd = os.getcwd() + os.chdir(root) + try: + restored, unsatisfied = restore( + str(cache), "database.json", ".gitignore" + ) + finally: + os.chdir(cwd) + self.assertEqual(restored, 0) + self.assertEqual(unsatisfied, ["bios/absent.bin"]) + + def test_an_unreachable_buildbot_is_not_a_fresh_verdict(self): + source = (ROOT / "scripts" / "check_buildbot_system.py").read_text( + encoding="utf-8" + ) + self.assertIn( + 'if report.get("error"):', + source.split("def main(")[-1], + "main() ignores the error the report carries", + ) + + def test_a_missing_profile_directory_says_so(self): + import io as _io + + from scripts import common as _common + + stderr = _io.StringIO() + with contextlib.redirect_stderr(stderr): + profiles = _common.load_emulator_profiles( + str(ROOT / "no-such-emulator-dir") + ) + self.assertEqual(profiles, {}) + self.assertIn("no emulator profile directory", stderr.getvalue()) + + class TestEntryPointsRunEveryClass(unittest.TestCase): """`unittest.main()` has to sit after the last test class.