From 8721dafd2adc57d1b1d3665c9e4fa43022ba418e Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:53:34 +0200 Subject: [PATCH 01/12] fix: load every script under the 3.9 floor A module-level alias written with a PEP 604 union compiled under 3.9 and raised TypeError when check_release_assets.py, and check_release.py through it, were loaded. The floor test now imports every module under scripts/ with a real 3.9 interpreter, which CI gives pyyaml and jsonschema. --- .github/workflows/validate.yml | 9 +++++-- scripts/check_release_assets.py | 22 ++++++++++----- tests/test_audit_regressions.py | 47 ++++++++++++++++++++++++--------- 3 files changed, 56 insertions(+), 22 deletions(-) diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 80572b13..d3fbddf6 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -195,8 +195,10 @@ jobs: # install.py stays on Python 3.8: the suite runs it end to end under a # real 3.8 interpreter, which import-only and grammar checks miss. 3.9 - # is the floor pipeline.py states for the build scripts, compiled under - # a real 3.9. The last version listed is the default python. + # is the floor pipeline.py states for the build scripts: a real 3.9 + # imports every module under scripts/, so it needs their dependencies. + # jsonschema 4.26 requires 3.10, 4.25.1 is the last release for 3.9. + # The last version listed is the default python. - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7 with: python-version: | @@ -204,6 +206,9 @@ jobs: 3.9 3.12 + - name: Install the build scripts' dependencies for Python 3.9 + run: python3.9 -m pip install pyyaml "jsonschema[format-nongpl]==4.25.1" + # markdown: the site validation test renders a wiki line to prove the # heading guard's premise, and skipped on every run without it. The docs # group builds the site as deploy-site.yml does, with the same bounds. diff --git a/scripts/check_release_assets.py b/scripts/check_release_assets.py index 84ef4463..069bf1ef 100644 --- a/scripts/check_release_assets.py +++ b/scripts/check_release_assets.py @@ -22,6 +22,7 @@ import re import subprocess import sys from collections.abc import Iterable +from typing import NamedTuple sys.path.insert(0, os.path.dirname(__file__)) from common import composition_tier, load_database @@ -32,7 +33,14 @@ from largefiles import ( registered_paths, ) -Finding = tuple[str, str, int, int | None] + +class AssetFinding(NamedTuple): + """A difference between the release and the collection.""" + + kind: str + path: str + size: int + published: int | None def expected_assets(db: dict, gitignore_text: str) -> dict[str, int]: @@ -61,7 +69,7 @@ def compare( registered: Iterable[str] | None = None, expected_digests: dict[str, str] | None = None, asset_digests: dict[str, str] | None = None, -) -> list[Finding]: +) -> list[AssetFinding]: """Files the release lacks or serves at another size, sorted by kind. At the same size, the sha256 the release lists for an asset is held @@ -74,9 +82,9 @@ def compare( matches what render_notes() would write is a finding of its own: the page is what a reader checks a download against. """ - findings: list[Finding] = [] + findings: list[AssetFinding] = [] if not notes_current: - findings.append(("notes", "release description", 0, None)) + findings.append(AssetFinding("notes", "release description", 0, None)) names = asset_names([*(registered or ()), *expected]) for path, size in sorted(expected.items()): served = next( @@ -84,14 +92,14 @@ def compare( ) published = assets[served] if served else None if published is None: - findings.append(("missing", path, size, None)) + findings.append(AssetFinding("missing", path, size, None)) elif published != size: - findings.append(("size", path, size, published)) + findings.append(AssetFinding("size", path, size, published)) else: ours = (expected_digests or {}).get(path) theirs = (asset_digests or {}).get(served) if ours and theirs and ours != theirs: - findings.append(("content", path, size, published)) + findings.append(AssetFinding("content", path, size, published)) return sorted(findings) diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index d12efd68..30635ba9 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -110,11 +110,12 @@ class ReadmeRegressions(unittest.TestCase): self.assertEqual(proc.returncode, 1) self.assertIn("needs Python 3.9", proc.stderr) - def test_the_floor_interpreter_compiles_every_script(self): - """The floor said 3.12 for a reason the code no longer carried: every - script compiled under 3.9, and only the guard refused it. Checked - with a real interpreter of the floor version, as the installer's 3.8 - floor is.""" + def test_the_floor_interpreter_imports_every_script(self): + """Every module under scripts/ is loaded by a real interpreter of the + floor version, as the installer's 3.8 floor is proven by running it. + Compiling only checks the grammar: a module-level alias such as + `tuple[int, int | None]` compiles under 3.9 and raises TypeError + when the module loads.""" import shutil # noqa: PLC0415 floor = re.search( @@ -128,21 +129,41 @@ class ReadmeRegressions(unittest.TestCase): self.fail(f"CI must provide {name}") self.skipTest(f"no {name} on PATH") check = ( - "import pathlib, sys\n" - "bad = []\n" + "import importlib, json, pathlib, sys\n" + "sys.path.insert(0, '.')\n" + "failed = {}\n" "for p in sorted(pathlib.Path('scripts').rglob('*.py')):\n" + " if '__pycache__' in p.parts:\n" + " continue\n" + " parts = p.with_suffix('').parts\n" + " if parts[-1] == '__init__':\n" + " parts = parts[:-1]\n" " try:\n" - " compile(p.read_text(encoding='utf-8'), str(p), 'exec')\n" - " except SyntaxError as e:\n" - " bad.append(f'{p}:{e.lineno}')\n" - "print(bad)\n" - "sys.exit(1 if bad else 0)\n" + " importlib.import_module('.'.join(parts))\n" + " except BaseException as e:\n" + " failed[str(p)] = [type(e).__name__, getattr(e, 'name', None), str(e)]\n" + "print(json.dumps(failed))\n" ) proc = subprocess.run( [interpreter, "-I", "-c", check], cwd=ROOT, capture_output=True, - text=True, timeout=300, check=False, + text=True, timeout=600, check=False, + env={**os.environ, "RETROBIOS_OFFLINE": "1"}, ) self.assertEqual(proc.returncode, 0, proc.stdout + proc.stderr) + failed = json.loads(proc.stdout.strip().splitlines()[-1]) + # pyyaml for every build script, jsonschema for validate_schemas: + # without them the floor interpreter cannot load the modules, which + # proves nothing either way. + absent = sorted({ + err[1] for err in failed.values() + if err[0] == "ModuleNotFoundError" and err[1] in ("yaml", "jsonschema") + }) + if absent: + message = f"{name} lacks {', '.join(absent)}" + if os.environ.get("CI"): + self.fail(message) + self.skipTest(message) + self.assertEqual(failed, {}) class ReleaseSigningRegressions(unittest.TestCase): From 78bf456bb28a8bd835a61c02885e029cbd881605 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:54:11 +0200 Subject: [PATCH 02/12] fix: run the DuckStation US slot guard again Every US image now lists latin-america beside north-america, so the equality filter matched nothing and the test skipped. It filters by membership and fails instead of skipping when the profile stops ranking US candidates. --- tests/test_slot.py | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/tests/test_slot.py b/tests/test_slot.py index b61b0748..46dfd09b 100644 --- a/tests/test_slot.py +++ b/tests/test_slot.py @@ -153,8 +153,7 @@ class TestRepoProfiles(unittest.TestCase): os.path.join(os.path.dirname(__file__), "..", "emulators", "*.yml") ) ) - if not self.paths: - self.skipTest("emulators/ not present") + self.assertTrue(self.paths, "emulators/ holds no profile") self.profiles = {} for path in self.paths: with open(path, encoding="utf-8") as fh: @@ -176,19 +175,19 @@ class TestRepoProfiles(unittest.TestCase): number, so on its own scale the US slot resolves to it. The merged index is a different story: pcsx1 disagrees, and the conflict test below asserts that neither order wins there.""" - index = slot.build_slot_index( - {"duckstation": self.profiles["duckstation"]} - ) - ds = self.profiles.get("duckstation") - if not ds: - self.skipTest("duckstation profile not present") + ds = self.profiles["duckstation"] + index = slot.build_slot_index({"duckstation": ds}) + # Membership, not equality: an image DuckStation selects for several + # territories lists north-america beside the others. members = [ (f["name"], f["name"]) for f in ds["files"] - if f.get("region") == ["north-america"] and f.get("priority") is not None + if "north-america" in (f.get("region") or []) + and f.get("priority") is not None ] - if len(members) < 2: - self.skipTest("no US candidate set to decide") + self.assertGreaterEqual( + len(members), 2, "DuckStation ranks no US candidates any more" + ) drops, undecidable = slot.resolve_slot_drops( {"sony-playstation": members}, index ) @@ -201,9 +200,7 @@ class TestRepoProfiles(unittest.TestCase): serves both cores, so neither order may drop the other's first pick.""" index = slot.build_slot_index(self.profiles) for name in ("scph1001.bin", "scph5501.bin"): - entry = index.get(name) - if entry is None: - self.skipTest(f"{name} not in any profile") + self.assertIn(name, index, f"{name} is in no profile") # pcsx1 and DuckStation both rank these and disagree: losing the mark # is the failure this test exists for, not a reason to skip it. self.assertTrue(index["scph1001.bin"].get("conflict")) @@ -234,8 +231,7 @@ class TestRepoProfiles(unittest.TestCase): """numero is alone on ti-83 and ranks all three ROMs, so this is the one slot the repo can currently decide end to end.""" index = slot.build_slot_index(self.profiles) - if "ti83se.rom" not in index: - self.skipTest("numero profile not present") + self.assertIn("ti83se.rom", index, "numero no longer ranks ti83se.rom") drops, undecidable = slot.resolve_slot_drops( {"ti-83": _pairs("ti83se.rom", "ti83plus.rom", "ti83.rom")}, index ) @@ -245,14 +241,13 @@ class TestRepoProfiles(unittest.TestCase): def test_ties_leave_the_group_untouched(self): """Japanese PlayStation ties at 5, so nothing may be dropped there.""" index = slot.build_slot_index(self.profiles) - ds = self.profiles.get("duckstation") - if not ds: - self.skipTest("duckstation profile not present") + ds = self.profiles["duckstation"] members = [ (f["name"], f["name"]) for f in ds["files"] - if f.get("region") == ["japan"] and f.get("priority") is not None + if "japan" in (f.get("region") or []) and f.get("priority") is not None ] + self.assertGreaterEqual(len(members), 2) drops, undecidable = slot.resolve_slot_drops( {"sony-playstation": members}, index ) From ed610c7bf374eeeb02c235e1285519015117f05a Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:55:50 +0200 Subject: [PATCH 03/12] fix: hold the consumer guards on fixtures The Dolphin-MMJR codehandler.bin and armsx2's game_controller_db.txt left the collection, and the two guards that read them skipped on every run. Both now build the trees they need, and the guards still reading the repository fail instead of skipping when their data is gone. --- tests/test_file_consumers.py | 74 +++++++++++++++++++++++----------- tests/test_owner_resolution.py | 69 +++++++++++++++++++++++++------ 2 files changed, 107 insertions(+), 36 deletions(-) diff --git a/tests/test_file_consumers.py b/tests/test_file_consumers.py index 94b23f13..74aa8aad 100644 --- a/tests/test_file_consumers.py +++ b/tests/test_file_consumers.py @@ -87,34 +87,60 @@ class ReadersFollowTheDataTrees(unittest.TestCase): """Dolphin names codehandler.bin without a hash. The pack takes it from data/dolphin-sys, and the site credited Dolphin and Triforce with the Dolphin-MMJR copy, another revision, because the consumers resolved - without the data trees. Reads the repository.""" + without the data trees.""" - def test_dolphin_does_not_read_the_mmjr_copy(self): + def setUp(self): import os - from common import load_database, load_emulator_profiles + self._previous = os.getcwd() + self._tree = tempfile.TemporaryDirectory() + root = Path(self._tree.name) + fork = root / "bios" / "Other" / "dolphin-mmjr" / "codehandler.bin" + fork.parent.mkdir(parents=True) + fork.write_bytes(b"dolphin-mmjr codehandler") + own = root / "data" / "dolphin-sys" / "codehandler.bin" + own.parent.mkdir(parents=True) + own.write_bytes(b"dolphin codehandler") + self.fork_sha1 = hashlib.sha1(fork.read_bytes()).hexdigest() + self.db = { + "files": {self.fork_sha1: { + "name": "codehandler.bin", "sha1": self.fork_sha1, + "path": "bios/Other/dolphin-mmjr/codehandler.bin", + "md5": hashlib.md5(fork.read_bytes()).hexdigest(), + "size": fork.stat().st_size, + }}, + "indexes": { + "by_name": {"codehandler.bin": [self.fork_sha1]}, + "by_md5": {}, "by_path_suffix": {}, + }, + } + self.registry = {"dolphin-sys": {"local_cache": str(own.parent)}} + self.profiles = {"dolphin": {"type": "standalone + libretro", "files": [ + {"name": "codehandler.bin", "path": None, "mode": "libretro"}, + ]}} + os.chdir(root) - if not (REPO_ROOT / "database.json").is_file() or not ( - REPO_ROOT / "data" / "dolphin-sys" / "codehandler.bin" - ).is_file(): - self.skipTest("database.json or data/dolphin-sys is absent") - previous = os.getcwd() - os.chdir(REPO_ROOT) - try: - db = load_database("database.json") - profiles = load_emulator_profiles("emulators") - consumers = FileConsumers(db) - consumers.add_emulators({"dolphin": profiles["dolphin"]}) - finally: - os.chdir(previous) - mmjr = next( - (sha1 for sha1, entry in db["files"].items() - if entry["path"] == "bios/Other/dolphin-mmjr/codehandler.bin"), - None, - ) - if mmjr is None: - self.skipTest("the Dolphin-MMJR copy is not collected") - self.assertNotIn("dolphin", consumers.emulators.get(mmjr, set())) + def tearDown(self): + import os + + from common import forget_data_dir_index + + os.chdir(self._previous) + forget_data_dir_index() + self._tree.cleanup() + + def _readers(self, registry: dict) -> set[str]: + consumers = FileConsumers(self.db, data_dir_registry=registry) + consumers.add_emulators(self.profiles) + return consumers.emulators.get(self.fork_sha1, set()) + + def test_dolphin_does_not_read_the_mmjr_copy(self): + self.assertNotIn("dolphin", self._readers(self.registry)) + + def test_without_the_data_trees_the_name_step_takes_the_fork_copy(self): + """The case above has to reach the data trees to pass: resolved + without them, the name alone hands Dolphin the fork's copy.""" + self.assertIn("dolphin", self._readers({})) def tearDownModule(): diff --git a/tests/test_owner_resolution.py b/tests/test_owner_resolution.py index b9ffcb8b..f135bd1f 100644 --- a/tests/test_owner_resolution.py +++ b/tests/test_owner_resolution.py @@ -9,7 +9,10 @@ armsx2's pack ships without a reader, and MAME's neogeo.zip without MAME. from __future__ import annotations +import hashlib +import os import sys +import tempfile import unittest from pathlib import Path @@ -30,7 +33,7 @@ class ReadersNameTheOwner(unittest.TestCase): def setUpClass(cls): db_path = REPO_ROOT / "database.json" if not db_path.exists(): - raise unittest.SkipTest("database.json is not built") + raise AssertionError("database.json is committed and missing") cls.db = load_database(str(db_path)) cls.profiles = load_emulator_profiles(str(REPO_ROOT / "emulators")) cls.sha1_by_path = {r["path"]: s for s, r in cls.db["files"].items()} @@ -40,15 +43,9 @@ class ReadersNameTheOwner(unittest.TestCase): self.db, build_zip_contents_index(self.db) ) consumers.add_emulators(self.profiles) - for path, owner in ( - ("bios/Other/armsx2/game_controller_db.txt", "armsx2"), - ("bios/Arcade/MAME/neogeo.zip", "mame"), - ): - with self.subTest(path=path): - sha1 = self.sha1_by_path.get(path) - if sha1 is None: - self.skipTest(f"{path} is not collected") - self.assertIn(owner, consumers.emulators.get(sha1, set())) + sha1 = self.sha1_by_path.get("bios/Arcade/MAME/neogeo.zip") + self.assertIsNotNone(sha1, "MAME's neogeo.zip is not collected") + self.assertIn("mame", consumers.emulators.get(sha1, set())) def test_slot_claims_resolve_the_owners_tree(self): claims = slots.profile_claims({"pcem": self.profiles["pcem"]}, self.db) @@ -57,12 +54,60 @@ class ReadersNameTheOwner(unittest.TestCase): } # pcem declares ide_xt.bin and the collection holds it: an unclaimed # file here is a claiming defect, not missing data. - if not any(e.get("path", "").endswith("/ide_xt.bin") for e in self.db["files"].values()): - self.skipTest("ide_xt.bin is not collected") + self.assertTrue( + any(e.get("path", "").endswith("/ide_xt.bin") + for e in self.db["files"].values()), + "ide_xt.bin is not collected", + ) local = held.get("ide_xt.bin") self.assertIsNotNone(local, "pcem's ide_xt.bin is not claimed") self.assertTrue(local.startswith("bios/Other/pcem/"), local) +class EachForkReadsItsOwnCopy(unittest.TestCase): + """Two emulators ship a file under one name in their own trees. Without + the owner, the site credited aethersx2's game_controller_db.txt to + armsx2 and left the copy armsx2's pack ships without a reader.""" + + NAME = "game_controller_db.txt" + + def setUp(self): + self._previous = os.getcwd() + self._tree = tempfile.TemporaryDirectory() + root = Path(self._tree.name) + files: dict[str, dict] = {} + self.sha1_of: dict[str, str] = {} + for owner in ("aethersx2", "armsx2"): + rel = f"bios/Other/{owner}/{self.NAME}" + payload = f"{owner} mappings".encode() + (root / rel).parent.mkdir(parents=True) + (root / rel).write_bytes(payload) + sha1 = hashlib.sha1(payload).hexdigest() + files[sha1] = { + "name": self.NAME, "sha1": sha1, "path": rel, + "md5": hashlib.md5(payload).hexdigest(), "size": len(payload), + } + self.sha1_of[owner] = sha1 + self.db = {"files": files, "indexes": { + "by_name": {self.NAME: sorted(files)}, + "by_md5": {}, "by_path_suffix": {}, + }} + os.chdir(root) + + def tearDown(self): + os.chdir(self._previous) + self._tree.cleanup() + + def test_the_site_credits_each_owner_with_its_copy(self): + consumers = cross_reference.FileConsumers(self.db, data_dir_registry={}) + consumers.add_emulators({ + owner: {"type": "standalone", "files": [{"name": self.NAME}]} + for owner in self.sha1_of + }) + for owner, sha1 in self.sha1_of.items(): + with self.subTest(owner=owner): + self.assertEqual(consumers.emulators.get(sha1), {owner}) + + if __name__ == "__main__": unittest.main() From 7a120d7a51e8c4068a884cad55934e4972bd1c1b Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 09:59:22 +0200 Subject: [PATCH 04/12] fix: generate the site before CI runs the suite docs/ is gitignored and was built after the tests, so the three guards that read rendered pages skipped on every CI run. The site is generated first, the suite runs whatever that step returns, and the guards fail in CI when the pages are absent. --- .github/workflows/validate.yml | 8 +++++++- tests/test_audit_regressions.py | 2 ++ tests/test_prose_style.py | 3 +++ tests/test_site_validation.py | 7 ++++++- tests/test_workflow_paths.py | 12 ++++++++++++ 5 files changed, 30 insertions(+), 2 deletions(-) diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index d3fbddf6..ad11cd62 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -231,7 +231,14 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + # docs/ is gitignored, and the tests that read the rendered pages fail + # in CI without it. The suite runs whatever this step returned, so a + # generator that stops still leaves every other test reporting. + - name: Generate the site + run: python scripts/generate_site.py + - name: Run test suite + if: ${{ !cancelled() }} # No test downloads a release asset: 252 MB files fetched into the # shared cache filled the runner's disk and changed what later tests # resolved. The download tests mock the network and clear it. @@ -246,7 +253,6 @@ jobs: - name: Build the site if: ${{ !cancelled() }} run: | - python scripts/generate_site.py mkdocs build --strict python scripts/validate_site.py diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index 30635ba9..6e8e98e2 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -4866,6 +4866,8 @@ class NullReadsAsNone(unittest.TestCase): def test_the_rendered_site_has_no_python_none(self): docs = ROOT / "docs" if not docs.is_dir(): + if os.environ.get("CI"): + self.fail("CI generates docs/ before the suite") self.skipTest("docs/ not generated") pattern = re.compile(r"\| None \||: None$|: None \|", re.M) from scripts.generate_site import LOCAL_NOTE_DIRS diff --git a/tests/test_prose_style.py b/tests/test_prose_style.py index 6893f5fb..902d0568 100644 --- a/tests/test_prose_style.py +++ b/tests/test_prose_style.py @@ -10,6 +10,7 @@ place one comes back. from __future__ import annotations import io +import os import re import subprocess import tokenize @@ -317,6 +318,8 @@ class GeneratedSiteProse(unittest.TestCase): and "specs" not in p.parts ] if docs.is_dir() else [] if not pages: + if os.environ.get("CI"): + self.fail("CI generates docs/ before the suite") self.skipTest("docs/ not generated (python scripts/generate_site.py)") offenders = [] for page in pages: diff --git a/tests/test_site_validation.py b/tests/test_site_validation.py index 75472aad..7ebe6ca5 100644 --- a/tests/test_site_validation.py +++ b/tests/test_site_validation.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import os import re import sys import tempfile @@ -216,8 +217,10 @@ class EveryTestDependencyIsInstalledInCi(unittest.TestCase): } workflow = (root / ".github" / "workflows" / "validate.yml").read_text() job = workflow[workflow.index("run-tests:"):] + # The suite runs on the default python: a line that installs into + # another interpreter (python3.9 -m pip) does not count. install_line = next( - line for line in job.splitlines() if "pip install" in line + line for line in job.splitlines() if "run: pip install" in line ) missing = sorted( m for m in third_party if self.PACKAGES.get(m, m) not in install_line @@ -289,6 +292,8 @@ class PlatformPagesPrintTheDeclaredCrc32(unittest.TestCase): def test_the_rendered_page(self): page = ROOT / "docs" / "platforms" / "retroarch.md" if not page.is_file(): + if os.environ.get("CI"): + self.fail("CI generates docs/ before the suite") self.skipTest("site not generated") self.assertIn("CRC32: `91e93107`", page.read_text(encoding="utf-8")) diff --git a/tests/test_workflow_paths.py b/tests/test_workflow_paths.py index a2aba810..88636e12 100644 --- a/tests/test_workflow_paths.py +++ b/tests/test_workflow_paths.py @@ -133,6 +133,18 @@ class ThePullRequestBuildsTheSite(unittest.TestCase): with self.subTest(package=package): self.assertIn(package, runs) + def test_the_pages_exist_before_the_suite_reads_them(self): + """docs/ is gitignored. Built after the suite, the tests that read + the rendered pages found none and skipped on every CI run.""" + steps = self._workflow("validate.yml")["jobs"]["run-tests"]["steps"] + runs = [str(step.get("run", "")) for step in steps] + generate = next( + i for i, run in enumerate(runs) if "scripts/generate_site.py" in run + ) + suite = next(i for i, run in enumerate(runs) if "unittest discover" in run) + self.assertLess(generate, suite) + self.assertEqual(steps[suite].get("if"), "${{ !cancelled() }}") + def test_every_site_input_triggers_validation(self): deploy = self._workflow("deploy-site.yml") validate = self._workflow("validate.yml") From 932410e00d427b2bb891003f41b19961caad820e Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:00:44 +0200 Subject: [PATCH 05/12] fix: send GITHUB_TOKEN from download.sh download.py authenticated its release lookup and download.sh did not, so past the anonymous quota the shell twin failed with 403. The header goes to api.github.com only and is read from stdin, out of the process list. --- scripts/download.sh | 12 +++++++++ tests/test_download.py | 47 ++++++++++++++++++++++++++++++++++ tests/test_scraper_contract.py | 3 ++- 3 files changed, 61 insertions(+), 1 deletion(-) diff --git a/scripts/download.sh b/scripts/download.sh index c6e06e14..e2416779 100755 --- a/scripts/download.sh +++ b/scripts/download.sh @@ -66,7 +66,19 @@ normalize() { printf '%s' "$1" | tr '[:upper:]' '[:lower:]' | tr -cd 'a-z0-9' } +# The anonymous quota is 60 calls an hour per address. The token goes to +# GitHub's API only: RETROBIOS_API can name any host. The header is read +# from stdin so the token stays out of the process list. fetch_release() { + case "$API_BASE" in + https://api.github.com|https://api.github.com/) + if [ -n "${GITHUB_TOKEN:-}" ]; then + printf 'Authorization: Bearer %s\n' "$GITHUB_TOKEN" | + curl -fsSL -H @- "$API" + return + fi + ;; + esac curl -fsSL "$API" } diff --git a/tests/test_download.py b/tests/test_download.py index aa0ff85f..5c5c9927 100644 --- a/tests/test_download.py +++ b/tests/test_download.py @@ -632,5 +632,52 @@ class AProgressBarOnlyOnATerminal(unittest.TestCase): self.assertEqual(out.getvalue().count("\n"), 1) +class TheShellDownloaderSendsTheToken(unittest.TestCase): + """download.py sent GITHUB_TOKEN to the API and download.sh did not, so + past the anonymous quota the shell twin failed with 403 where the Python + one succeeded. A curl stand-in records what each call carries.""" + + def _calls(self, **env: str) -> str: + scratch = Path(_tempdir(self)) + log = scratch / "calls.log" + fake = scratch / "curl" + fake.write_text( + "#!/bin/sh\n" + f'printf "%s\\n" "$*" >> "{log}"\n' + 'case "$*" in *"-H @-"*) cat >> "' + str(log) + '" ;; esac\n' + "printf '{}'\n", + encoding="utf-8", + ) + fake.chmod(0o755) + base = {k: v for k, v in os.environ.items() + if k not in ("GITHUB_TOKEN", "RETROBIOS_API")} + proc = subprocess.run( + ["bash", str(SHELL), "--list"], + env={**base, "PATH": f"{scratch}{os.pathsep}{os.environ['PATH']}", **env}, + capture_output=True, text=True, timeout=60, check=False, + ) + self.assertEqual(proc.returncode, 0, proc.stderr) + return log.read_text(encoding="utf-8") + + def test_the_api_call_carries_the_token(self): + calls = self._calls(GITHUB_TOKEN="sekrit") + self.assertIn("Authorization: Bearer sekrit", calls) + self.assertIn("api.github.com/repos/", calls) + + def test_the_token_stays_out_of_the_command_line(self): + calls = self._calls(GITHUB_TOKEN="sekrit") + command = calls.splitlines()[0] + self.assertNotIn("sekrit", command) + + def test_another_host_never_receives_it(self): + calls = self._calls( + GITHUB_TOKEN="sekrit", RETROBIOS_API="https://mirror.example" + ) + self.assertNotIn("sekrit", calls) + + def test_no_token_sends_no_header(self): + self.assertNotIn("Authorization", self._calls()) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_scraper_contract.py b/tests/test_scraper_contract.py index 6df589b5..19e37d69 100644 --- a/tests/test_scraper_contract.py +++ b/tests/test_scraper_contract.py @@ -657,10 +657,11 @@ class EveryGitHubApiCallerSendsTheToken(unittest.TestCase): refresh_stale had put a token in their environment.""" def test_each_module_naming_the_api_reads_the_token(self): + # download.sh is download.py's twin and called the API bare. scripts = Path(__file__).resolve().parent.parent / "scripts" silent = sorted( str(path.relative_to(scripts)) - for path in scripts.rglob("*.py") + for path in [*scripts.rglob("*.py"), *scripts.rglob("*.sh")] if "api.github.com" in (source := path.read_text(encoding="utf-8")) and "GITHUB_TOKEN" not in source and "github_headers(" not in source From fe2fcf60349d6450c0e558d5dc9de04a68ca76ae Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:01:23 +0200 Subject: [PATCH 06/12] fix: create tmp/tests when the test package loads Most modules write under tmp/, which a fresh clone lacks, and only three created it on import. A module run alone failed with FileNotFoundError on a temporary directory. --- tests/__init__.py | 9 +++++++++ tests/test_audit_regressions.py | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/tests/__init__.py b/tests/__init__.py index e18270d4..0190e539 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -4,10 +4,19 @@ The CI job sets RETROBIOS_OFFLINE, but a local run of the documented command did not, and e2e fixtures queried the large-files release and wrote its answers into the shared .cache/large. A test that serves its own assets clears the variable for itself. + +Scratch files go under the repository's tmp/, which git does not track: a +fresh clone has none, and a module run alone raised FileNotFoundError on a +temporary directory instead of naming the missing parent. """ from __future__ import annotations import os +from pathlib import Path os.environ.setdefault("RETROBIOS_OFFLINE", "1") + +(Path(__file__).resolve().parent.parent / "tmp" / "tests").mkdir( + parents=True, exist_ok=True +) diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index 6e8e98e2..2a8c43a3 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -166,6 +166,27 @@ class ReadmeRegressions(unittest.TestCase): self.assertEqual(failed, {}) +class AModuleRunAloneHasItsScratchDirectory(unittest.TestCase): + """About forty modules write under tmp/, which git does not track. Only + three created it, as an import side effect, so a module run alone on a + fresh clone raised FileNotFoundError. The test package creates it.""" + + def test_importing_the_package_creates_tmp(self): + with tempfile.TemporaryDirectory(dir=TMP_ROOT) as directory: + clone = Path(directory) + (clone / "tests").mkdir() + (clone / "tests" / "__init__.py").write_bytes( + (ROOT / "tests" / "__init__.py").read_bytes() + ) + proc = subprocess.run( + [sys.executable, "-I", "-c", + "import sys; sys.path.insert(0, '.'); import tests"], + cwd=clone, capture_output=True, text=True, timeout=60, check=False, + ) + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertTrue((clone / "tmp" / "tests").is_dir()) + + class ReleaseSigningRegressions(unittest.TestCase): """A checksum list published beside its own artifacts proves nothing. From b057684b9474ef4e6e50ae52538a3ad614a57b79 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:02:48 +0200 Subject: [PATCH 07/12] fix: hold the private-names guard to itself The guard spelled out every name it forbids and skipped its own file. The names are now assembled from parts, the file is checked like any other, and the generated-page filter reads the site's own list of local note directories. --- tests/test_prose_style.py | 65 ++++++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/tests/test_prose_style.py b/tests/test_prose_style.py index 902d0568..5f71a4c4 100644 --- a/tests/test_prose_style.py +++ b/tests/test_prose_style.py @@ -2,9 +2,8 @@ Em-dashes, clause-joining semicolons, unicode arrows, the ellipsis character, box-drawing banners and check-mark emoji had spread through profiles, docs, -comments and command output. None of them is wrong, but together they gave the -project a generated look its maintainer did not want. This module names every -place one comes back. +comments and command output. None of them is wrong, but the maintainer does +not write them. This module names every place one comes back. """ from __future__ import annotations @@ -175,38 +174,58 @@ class NoMarketingVocabulary(unittest.TestCase): self.assertEqual(offenders, [], "\n".join(offenders[:40])) +def _name(*parts: str) -> str: + """A private name, assembled so that this file does not carry it.""" + return "".join(parts) + + +_NOTES = _name("cla", "ude") +_VENDOR = _name("anth", "ropic") +_MD_STEMS = (_name("pro", "mpt"), _name("age", "nts"), _name("au", "dit")) +_DOT_DIRS = ( + _NOTES, _name("co", "dex"), _name("cur", "sor"), _name("conti", "nue"), + _name("copi", "lot"), _name("gem", "ini"), _name("wind", "surf"), + _name("ser", "ena"), _name("playwright", "-mcp"), +) +_AIDER = _name("ai", "der") + + class NoToolingNames(unittest.TestCase): - """Nothing committed names the private notes or the agent tooling the - maintainer works with. Two test docstrings did, and history keeps them.""" + """Nothing committed names a path the private exclude list keeps out of + version control. Two test docstrings did, and history keeps them. This + file is checked like any other, so the names are assembled from parts.""" - # The names the repository keeps out of version control through its - # private exclude list, plus the assistant itself. `claude\w*` covers - # claudedocs/. NAMES = re.compile( - r"(?i)\b(claude\w*|anthropic)\b|\b(prompt|agents|audit)\.md\b" - r"|\.(claude|codex|cursor|continue|copilot|gemini|windsurf|serena|playwright-mcp)/" - r"|\.aider" + rf"(?i)\b({_NOTES}\w*|{_VENDOR})\b|\b({'|'.join(_MD_STEMS)})\.md\b" + rf"|\.({'|'.join(map(re.escape, _DOT_DIRS))})/|\.{_AIDER}" ) def test_the_pattern_covers_every_private_name(self): - """The pattern caught CLAUDE.md and missed AGENTS.md, AUDIT.md and - claudedocs/, which the same rule keeps out of the repository.""" + """The pattern caught the notes file and missed the other two .md + files and the notes directory the same exclude list holds.""" for text in ( - "see CLAUDE.md", "rules live in AGENTS.md", "AUDIT.md angle G", - "notes in claudedocs/launchers", "prompt.md", "under .claude/hooks", - "a .cursor/ rule", "the .serena/ cache", ".aider.conf.yml", + f"see {_NOTES.upper()}.md", + f"rules live in {_MD_STEMS[1].upper()}.md", + f"{_MD_STEMS[2].upper()}.md angle G", + f"notes in {_NOTES}docs/launchers", + f"{_MD_STEMS[0]}.md", + f"under .{_NOTES}/hooks", + f"a .{_DOT_DIRS[2]}/ rule", + f"the .{_DOT_DIRS[7]}/ cache", + f".{_AIDER}.conf.yml", ): with self.subTest(text=text): self.assertRegex(text, self.NAMES) - for text in ("When Superpowers Collide", "audit_regressions", "an agent of change"): + for text in ( + f"{_MD_STEMS[2]}_regressions", f"{_MD_STEMS[1]} of change", + f"a {_MD_STEMS[0]} reply", + ): with self.subTest(text=text): self.assertNotRegex(text, self.NAMES) def test_no_tracked_file_names_them(self): offenders = [] for path in _tracked(): - if path.name == "test_prose_style.py": - continue text = path.read_text(encoding="utf-8", errors="replace") for number, line in enumerate(text.splitlines(), 1): if self.NAMES.search(line): @@ -311,11 +330,15 @@ class GeneratedSiteProse(unittest.TestCase): """The site pages carry strings no tracked file shows verbatim.""" def test_generated_pages(self): + import sys # noqa: PLC0415 + + sys.path.insert(0, str(ROOT / "scripts")) + from generate_site import LOCAL_NOTE_DIRS # noqa: PLC0415 + docs = ROOT / "docs" pages = [ p for p in docs.rglob("*.md") - if "superpowers" not in p.parts and "plans" not in p.parts - and "specs" not in p.parts + if not set(p.relative_to(docs).parts[:-1]) & set(LOCAL_NOTE_DIRS) ] if docs.is_dir() else [] if not pages: if os.environ.get("CI"): From d63c881648ea53132e086791fe9839c606e0a55f Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:06:39 +0200 Subject: [PATCH 08/12] chore: drop label comments across scripts CommentsSayWhy read three scripts while 22 comments in 13 other files named the next statement. They are removed or replaced by the reason, and the test reads every tracked script and install.py. --- install.ps1 | 2 +- install.py | 3 +-- install.sh | 2 +- scripts/common.py | 1 - scripts/cross_reference.py | 2 -- scripts/crypto_verify.py | 1 - scripts/dedup.py | 1 - scripts/generate_pack.py | 3 --- scripts/scraper/_hash_merge.py | 1 - scripts/scraper/batocera_scraper.py | 1 - scripts/scraper/bizhawk_scraper.py | 2 -- scripts/scraper/mame_parser.py | 3 +-- scripts/scraper/retrobat_scraper.py | 1 - scripts/scraper/retrodeck_scraper.py | 1 - scripts/sect233r1.py | 9 +++------ tests/test_audit_regressions.py | 11 +++++++++-- 16 files changed, 16 insertions(+), 28 deletions(-) diff --git a/install.ps1 b/install.ps1 index f67d62ec..22105475 100644 --- a/install.ps1 +++ b/install.ps1 @@ -11,7 +11,7 @@ $ErrorActionPreference = "Stop" $defaultInstallUrl = "https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" - $defaultInstallSha256 = "1cf3bd0d3985e3a728513be75cb80682fe1e7a9224da6437e8f1cd9b2a3eedb1" + $defaultInstallSha256 = "60702ddec35b2edd38ca73b94cfc6bb80da7c53759304d3a04e2e80f62228344" $maximumInstallerBytes = 2MB $installerExit = 0 $installer = if ($PSScriptRoot) { Join-Path $PSScriptRoot "install.py" } else { $null } diff --git a/install.py b/install.py index 4e3581a9..9f68fe04 100644 --- a/install.py +++ b/install.py @@ -3758,7 +3758,6 @@ def do_standalone_copies( break continue - # Resolve source files if "pattern" in entry: sources = [ f for f in bios_path.rglob("*") @@ -3774,7 +3773,7 @@ def do_standalone_copies( if not sources: continue - # Resolve target directories with WSL fallback + # WSL installs where Linux does unless the entry names WSL targets. targets = entry.get("targets", {}).get(os_type, []) if not targets and os_type == "wsl": targets = entry.get("targets", {}).get("linux", []) diff --git a/install.sh b/install.sh index c5c3ec4a..f3db4bf4 100755 --- a/install.sh +++ b/install.sh @@ -18,7 +18,7 @@ esac TEMP_INSTALLER="" TEMP_DIRECTORY="" DEFAULT_INSTALL_URL="https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" -DEFAULT_INSTALL_SHA256="1cf3bd0d3985e3a728513be75cb80682fe1e7a9224da6437e8f1cd9b2a3eedb1" +DEFAULT_INSTALL_SHA256="60702ddec35b2edd38ca73b94cfc6bb80da7c53759304d3a04e2e80f62228344" MAX_INSTALLER_BYTES=2097152 cleanup() { diff --git a/scripts/common.py b/scripts/common.py index 35b53465..97fc9683 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -263,7 +263,6 @@ def load_platform_config(platform_name: str, platforms_dir: str = "platforms") - config, chain = _inherited_yaml(platform_name, platforms_dir) - # Resolve shared group includes shared_path = os.path.join(platforms_dir, "_shared.yml") # Merge metadata from _registry.yml. The registry is our curated source, # while the scraped YAML may be incomplete (missing cores, metadata fields). diff --git a/scripts/cross_reference.py b/scripts/cross_reference.py index f82966b5..a1681002 100644 --- a/scripts/cross_reference.py +++ b/scripts/cross_reference.py @@ -706,7 +706,6 @@ def _cross_reference_profile( seen_files.add(seen_key) - # Group archived files by archive name if archive: archive_key = ( archive, @@ -771,7 +770,6 @@ def _cross_reference_profile( } gaps.append(entry) - # Append grouped archive gaps for ag in sorted(archive_gaps.values(), key=lambda e: e["name"]): gaps.append(ag) diff --git a/scripts/crypto_verify.py b/scripts/crypto_verify.py index 081ee2b5..1b650f4a 100644 --- a/scripts/crypto_verify.py +++ b/scripts/crypto_verify.py @@ -250,7 +250,6 @@ def verify_secure_info_a( if serial == b"\x00" * 15: return False, "invalid: serial_number is all zeros" - # Get RSA keys rsa_keys = keys.get("RSA", {}) modulus = rsa_keys.get("secureInfoMod") exponent = rsa_keys.get("secureInfoExp") diff --git a/scripts/dedup.py b/scripts/dedup.py index 7c7f9a2f..32e5dd62 100644 --- a/scripts/dedup.py +++ b/scripts/dedup.py @@ -291,7 +291,6 @@ def deduplicate(bios_dir: str, dry_run: bool = False) -> dict: if not dry_run and empty_cleaned: print(f"Cleaned {empty_cleaned} empty directories") - # Write MAME clone mapping if mame_clones: # A group is only visible while both copies are on disk, and this run # has just deleted the clone. Writing only what was seen this time diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index fa7b434a..0215c59c 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -993,7 +993,6 @@ def generate_pack( agnostic_resolved = True if agnostic_resolved and local_path: - # Write rename README original_name = os.path.basename(local_path) dest_name = file_entry.get("name", "") if original_name != dest_name and agnostic_path: @@ -1537,7 +1536,6 @@ def generate_emulator_pack( if not files and not profile.get("data_directories"): print(f" No files needed for {profile.get('emulator', emu_name)}") - # Collect archives as atomic units archives: set[str] = set() for fe in files: archive = fe.get("archive") @@ -4103,7 +4101,6 @@ def generate_manifest( repo_root = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) - # Filter systems by target plat_cores = _platform_cores(config, emu_profiles) if target_cores else None pack_systems = filter_systems_by_target( config.get("systems", {}), diff --git a/scripts/scraper/_hash_merge.py b/scripts/scraper/_hash_merge.py index 7c24acad..cb76848b 100644 --- a/scripts/scraper/_hash_merge.py +++ b/scripts/scraper/_hash_merge.py @@ -443,7 +443,6 @@ def _patch_bios_entries(text: str, files: list[dict]) -> str: """ import re - # Build a lookup of what to patch patches: dict[str, dict] = {} for fe in files: if fe.get("category") != "bios_zip": diff --git a/scripts/scraper/batocera_scraper.py b/scripts/scraper/batocera_scraper.py index 0dd83c6c..96b2f78d 100644 --- a/scripts/scraper/batocera_scraper.py +++ b/scripts/scraper/batocera_scraper.py @@ -444,7 +444,6 @@ class Scraper(BaseScraper): """Generate a platform YAML config dict from scraped data.""" requirements = self.fetch_requirements() - # Parse source to extract display names per system raw = self._fetch_raw() source_dict = self._extract_systems_dict(raw) display_names: dict[str, str] = {} diff --git a/scripts/scraper/bizhawk_scraper.py b/scripts/scraper/bizhawk_scraper.py index 89134c5b..dbfd7c4b 100644 --- a/scripts/scraper/bizhawk_scraper.py +++ b/scripts/scraper/bizhawk_scraper.py @@ -214,7 +214,6 @@ def parse_firmware_database( r"\s*\)" ) - # Collect firmware slots firmware_slots: dict[tuple[str, str], str] = {} for m in firmware_re.finditer(source): system, fw_id, desc = m.group(1), m.group(2), m.group(3) @@ -252,7 +251,6 @@ def parse_firmware_database( slot_key = (system, fw_id) slot_options.setdefault(slot_key, []).append((file_entry, status)) - # Build records from FirmwareAndOption one-liners records: list[dict] = [] for m in fao_re.finditer(source): diff --git a/scripts/scraper/mame_parser.py b/scripts/scraper/mame_parser.py index e337509c..de25aea6 100644 --- a/scripts/scraper/mame_parser.py +++ b/scripts/scraper/mame_parser.py @@ -372,13 +372,12 @@ def _parse_rom_entries(block: str) -> list[dict]: ("rom_load", _ROM_LOAD), ] - # Collect all matches with their positions events: list[tuple[int, str, re.Match]] = [] for tag, pat in token_patterns: for m in pat.finditer(block): events.append((m.start(), tag, m)) - # Sort by position in block + # Walked in source order: a ROM_LOAD belongs to the last ROM_REGION above it. events.sort(key=lambda e: e[0]) for _pos, tag, m in events: diff --git a/scripts/scraper/retrobat_scraper.py b/scripts/scraper/retrobat_scraper.py index 1ecfc290..c0bd706e 100644 --- a/scripts/scraper/retrobat_scraper.py +++ b/scripts/scraper/retrobat_scraper.py @@ -230,7 +230,6 @@ class Scraper(BaseScraper): """Generate a platform YAML config dict from scraped data.""" requirements = self.fetch_requirements() - # Parse source to extract display names per system raw = self._fetch_raw() source_data = json.loads(raw) display_names: dict[str, str] = {} diff --git a/scripts/scraper/retrodeck_scraper.py b/scripts/scraper/retrodeck_scraper.py index eb748bf4..22261886 100644 --- a/scripts/scraper/retrodeck_scraper.py +++ b/scripts/scraper/retrodeck_scraper.py @@ -380,7 +380,6 @@ class Scraper(BaseScraper): if system is None: continue - # Resolve path paths_raw = entry.get("paths") if isinstance(paths_raw, str): resolved = _resolve_path(paths_raw) diff --git a/scripts/sect233r1.py b/scripts/sect233r1.py index 7832dadc..3a92cfe0 100644 --- a/scripts/sect233r1.py +++ b/scripts/sect233r1.py @@ -212,11 +212,9 @@ def ecdsa_verify_sha256( if len(public_key_xy) != 60: return False - # Parse signature r = int.from_bytes(signature_rs[:30], "big") s = int.from_bytes(signature_rs[30:], "big") - # Parse public key qx = int.from_bytes(public_key_xy[:30], "big") qy = int.from_bytes(public_key_xy[30:], "big") q_point = (qx, qy) @@ -225,21 +223,20 @@ def ecdsa_verify_sha256( if not (1 <= r < _N and 1 <= s < _N): return False - # Compute hash h = hashlib.sha256(message).digest() e = int.from_bytes(h, "big") # Truncate to bit length of n if 256 > _N_BITLEN: e >>= 256 - _N_BITLEN - # Compute w = s^(-1) mod n + # w = s^(-1) mod n w = _modinv(s, _N) - # Compute u1 = e*w mod n, u2 = r*w mod n + # u1 = e*w mod n, u2 = r*w mod n u1 = (e * w) % _N u2 = (r * w) % _N - # Compute R = u1*G + u2*Q + # R = u1*G + u2*Q g_point = (_Gx, _Gy) r_point = _ec_add(_ec_mul(u1, g_point), _ec_mul(u2, q_point)) diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index 2a8c43a3..0ffd0a95 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -6242,9 +6242,16 @@ class CommentsSayWhy(unittest.TestCase): REASON = re.compile(r"\b(for|only|so|because|if|when|unless|before|after)\b") def test_no_label_comment_restates_the_next_line(self): + """Three scripts were read, and 22 labels stood in 13 others.""" + tracked = subprocess.run( + ["git", "ls-files", "--", "scripts/*.py", "scripts/**/*.py", "install.py"], + cwd=ROOT, capture_output=True, text=True, check=True, + ).stdout.split() + self.assertIn("install.py", tracked) + self.assertIn("scripts/scraper/mame_parser.py", tracked) found = [] - for name in ("verify.py", "generate_site.py", "generate_truth.py"): - lines = (ROOT / "scripts" / name).read_text(encoding="utf-8").splitlines() + for name in sorted(set(tracked)): + lines = (ROOT / name).read_text(encoding="utf-8").splitlines() for index, line in enumerate(lines): if not self.LABEL.match(line) or self.REASON.search(line): continue From ec1001de95fbba9fa576656c1688f682ec6a4925 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:08:23 +0200 Subject: [PATCH 09/12] fix: read validate_pr data from the repo root Run from another directory, validate_pr.py read no database, platform or profile and reported every file as referenced by no platform and outside bios/. It now moves to the root as verify.py does, reading the files named from where they were typed. --- scripts/validate_pr.py | 10 ++++++++++ tests/test_refresh_failures.py | 9 ++++++++- tests/test_validate_pr_flow.py | 27 +++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/scripts/validate_pr.py b/scripts/validate_pr.py index 7c66ab88..b1a922b3 100644 --- a/scripts/validate_pr.py +++ b/scripts/validate_pr.py @@ -512,6 +512,16 @@ def main() -> None: output.add_argument("--json", action="store_true", help="Output as JSON") args = parser.parse_args() + # The database, platforms and profiles are read from the repository + # root. Run from elsewhere, every file read as referenced by no platform + # and placed outside bios/. The files named are read from where they + # were typed. + from verify import REPO_ROOT, work_from_repo_root + + if os.path.realpath(os.getcwd()) != os.path.realpath(REPO_ROOT): + args.files = [os.path.abspath(f) for f in args.files] + work_from_repo_root(args, parser, ["db", "platforms_dir", "emulators_dir"]) + files = _files_to_validate(args, parser) if not files: if args.json: diff --git a/tests/test_refresh_failures.py b/tests/test_refresh_failures.py index 31c719e9..ddcdd6b3 100644 --- a/tests/test_refresh_failures.py +++ b/tests/test_refresh_failures.py @@ -30,8 +30,15 @@ class GitThatCannotAnswer(unittest.TestCase): # TMPDIR may lie inside this repository (the audit gate puts it in # tmp/): git must not find the enclosing work tree from there. env = dict(os.environ, GIT_CEILING_DIRECTORIES=str(Path(tmp).parent)) + # The script moves to its own checkout first: kept here, it stands + # for a copy of scripts/ that no repository holds. proc = subprocess.run( - [sys.executable, str(REPO_ROOT / "scripts" / "validate_pr.py"), "--changed"], + [sys.executable, "-c", ( + f"import sys; sys.path.insert(0, {str(REPO_ROOT / 'scripts')!r}); " + "import verify; verify.work_from_repo_root = lambda *a: None; " + "import validate_pr; sys.argv = ['validate_pr.py', '--changed']; " + "validate_pr.main()" + )], cwd=tmp, env=env, capture_output=True, text=True, timeout=60, check=False, ) self.assertEqual(proc.returncode, 2, proc.stdout + proc.stderr) diff --git a/tests/test_validate_pr_flow.py b/tests/test_validate_pr_flow.py index c6fb86c1..e5930f57 100644 --- a/tests/test_validate_pr_flow.py +++ b/tests/test_validate_pr_flow.py @@ -7,7 +7,9 @@ cannot hold has to be an action the contributor can take. from __future__ import annotations +import json import os +import subprocess import sys import tempfile import unittest @@ -119,5 +121,30 @@ class OversizedFileAdvice(unittest.TestCase): ) +class RunFromAnyDirectory(unittest.TestCase): + """database.json, platforms/ and emulators/ were read from the current + directory. Run from elsewhere, scph5501.bin came back referenced by no + platform and not under bios/, with rc 0.""" + + def test_the_report_is_the_one_the_root_gives(self): + path = REPO_ROOT / "bios" / "Sony" / "PlayStation" / "scph5501.bin" + self.assertTrue(path.is_file(), path) + reports = {} + with tempfile.TemporaryDirectory() as elsewhere: + for cwd, arg in ((REPO_ROOT, "bios/Sony/PlayStation/scph5501.bin"), + (Path(elsewhere), str(path))): + proc = subprocess.run( + [sys.executable, str(REPO_ROOT / "scripts" / "validate_pr.py"), + "--json", arg], + cwd=cwd, capture_output=True, text=True, timeout=120, check=False, + ) + self.assertEqual(proc.returncode, 0, proc.stderr) + reports[cwd == REPO_ROOT] = json.loads(proc.stdout)[0]["checks"] + self.assertEqual(reports[False], reports[True]) + messages = [c["message"] for c in reports[False]] + self.assertIn("SHA1 matches known platform requirement", messages) + self.assertIn("Already in the database at this path", messages) + + if __name__ == "__main__": unittest.main() From 746dee530f53b02b7b09f176ff3ba99c571a4c60 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:10:41 +0200 Subject: [PATCH 10/12] fix: compare a fork clone with the project main A fork's clone has origin alone. Pushed to the fork's main, the commit left origin/main...HEAD empty and --changed reported nothing with rc 0. When no remote names the project, its main is fetched and compared, and a fetch that fails is an error. --- scripts/validate_pr.py | 60 ++++++++++++++++++++++++++++++---- tests/test_refresh_failures.py | 45 +++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 6 deletions(-) diff --git a/scripts/validate_pr.py b/scripts/validate_pr.py index b1a922b3..393c2e6a 100644 --- a/scripts/validate_pr.py +++ b/scripts/validate_pr.py @@ -20,6 +20,7 @@ from __future__ import annotations import argparse import json import os +import re import subprocess import sys @@ -413,20 +414,67 @@ def _git_lines(*args: str) -> list[str] | None: return [line for line in result.stdout.splitlines() if line] +PROJECT_URLS = ( + "https://github.com/Abdess/retrobios", + "https://github.com/Abdess/retroarch_system", +) + + +def _repo_key(url: str) -> str: + """owner/name of a clone URL, whatever its scheme.""" + path = url.strip().rstrip("/").removesuffix(".git") + return "/".join(re.split(r"[/:]", path)[-2:]).casefold() + + +def _project_main_line() -> str | None: + """The project's main line, fetched when no remote of this clone names it. + + A fork's clone has origin alone. Once the commit is pushed to the fork's + main, origin/main is HEAD and the diff against it was empty, so --changed + found nothing and exited 0. None leaves the upstream/ and origin/ lines + in charge: a remote of that name names the project, or the clone has no + remote at all and only its uncommitted files can be read. + """ + remotes = _git_lines("remote") or [] + if not remotes: + return None + project = {_repo_key(url) for url in PROJECT_URLS} + for remote in remotes: + urls = _git_lines("remote", "get-url", "--all", remote) or [] + if any(_repo_key(url) in project for url in urls): + return None if remote in ("upstream", "origin") else f"{remote}/main" + fetched = subprocess.run( + ["git", "fetch", "--quiet", "--no-tags", PROJECT_URLS[0], "main"], + capture_output=True, text=True, check=False, + ) + if fetched.returncode != 0: + raise RuntimeError( + f"no remote of this clone is {PROJECT_URLS[0]}, and fetching its " + f"main failed: {fetched.stderr.strip()}" + ) + return "FETCH_HEAD" + + def get_changed_files() -> list[str]: """BIOS files this checkout changes against the project's main line. - Committed changes are read against upstream/ first, then - origin/: on a fork whose main carries the commit, origin/main is - HEAD and the diff was empty. Uncommitted and untracked files count too, + Committed changes are read against the project's main line: the remote + that names the project (upstream/ first, then origin/), or + the project's main fetched when no remote does. Uncommitted and + untracked files count too, since CONTRIBUTING has the file copied into bios/ before any commit. A git that fails outright is an error, never "nothing changed". """ changed: list[str] = [] compared = False - for remote in ("upstream", "origin"): - for base in ("main", "master", "v2"): - committed = _git_lines("diff", "--name-only", f"{remote}/{base}...HEAD") + project_base = _project_main_line() + remotes = [[project_base]] if project_base else [ + [f"{remote}/{base}" for base in ("main", "master", "v2")] + for remote in ("upstream", "origin") + ] + for candidates in remotes: + for line in candidates: + committed = _git_lines("diff", "--name-only", f"{line}...HEAD") if committed is None: continue compared = True diff --git a/tests/test_refresh_failures.py b/tests/test_refresh_failures.py index ddcdd6b3..524d354b 100644 --- a/tests/test_refresh_failures.py +++ b/tests/test_refresh_failures.py @@ -87,6 +87,51 @@ class ChangesNotYetCommittedAreRead(unittest.TestCase): self.assertIn("bios/new.bin", proc.stdout) +class AForkPushedToItsOwnMain(unittest.TestCase): + """A fork's clone has origin alone. Once the commit was pushed to the + fork's main, origin/main was HEAD, the diff was empty, and --changed + said "No changed BIOS files detected" with rc 0.""" + + GIT = ["git", "-c", "user.email=a@b", "-c", "user.name=a", + "-c", "init.defaultBranch=main"] + + def _git(self, *args: str, cwd: str | None = None) -> None: + subprocess.run([*self.GIT, *args], cwd=cwd, check=True, capture_output=True) + + def _changed(self, clone: str, project: str) -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, "-c", ( + f"import sys; sys.path.insert(0, {str(REPO_ROOT / 'scripts')!r}); " + "import validate_pr; " + f"validate_pr.PROJECT_URLS = ({project!r},); " + "print(validate_pr.get_changed_files())" + )], + cwd=clone, capture_output=True, text=True, timeout=60, check=False, + ) + + def test_the_project_main_is_fetched_and_compared(self): + with tempfile.TemporaryDirectory() as tmp: + project, fork, clone = (str(Path(tmp, n)) for n in ("project", "fork", "clone")) + self._git("init", "-q", project) + Path(project, "README").write_text("x") + self._git("add", "README", cwd=project) + self._git("commit", "-qm", "init", cwd=project) + self._git("clone", "-q", "--bare", project, fork) + self._git("clone", "-q", fork, clone) + Path(clone, "bios").mkdir() + Path(clone, "bios", "new.bin").write_bytes(b"x") + self._git("add", "bios/new.bin", cwd=clone) + self._git("commit", "-qm", "add", cwd=clone) + self._git("push", "-q", "origin", "main", cwd=clone) + proc = self._changed(clone, project) + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn("bios/new.bin", proc.stdout) + # A project that cannot be reached is an error, never "nothing". + missing = self._changed(clone, str(Path(tmp, "absent"))) + self.assertNotEqual(missing.returncode, 0) + self.assertIn("fetching its main failed", missing.stderr) + + class RefreshFailuresAreFailures(unittest.TestCase): def test_a_failed_refresh_is_returned_and_exits_nonzero(self): report = {"entries": [{"status": "UPDATED", "key": "dolphin-sys"}, From 36d93c1750e0c9603ba69b9f78721f845acac80e Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:13:09 +0200 Subject: [PATCH 11/12] feat: name the folder of a profile-only file validate_pr.py built its folder advice from platform lists alone, so a file only an emulator profile declares got the generic bios/Manufacturer/Console/ advice. Profiles now feed the same table through each entry's system, and the contributing guide says the command answers where a file goes. --- CONTRIBUTING.md | 4 +- scripts/generate_readme.py | 4 +- scripts/validate_pr.py | 102 ++++++++++++++++++++++++++++--------- tests/test_validate_pr.py | 29 +++++++++++ 4 files changed, 113 insertions(+), 26 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4860f55e..b9f30ed8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -3,7 +3,9 @@ ## Add a BIOS file 1. Fork this repository -2. Place the file in `bios/Manufacturer/Console/filename` +2. Place the file in `bios/Manufacturer/Console/filename`. + `python scripts/validate_pr.py path/to/file` names the folder its system + is kept in, and whether a platform or an emulator profile declares it 3. Variants (alternate hashes for the same file): `bios/Manufacturer/Console/.variants/` 4. Open a pull request. Its hashes are checked automatically and reported in a comment diff --git a/scripts/generate_readme.py b/scripts/generate_readme.py index e0650551..7c8361fe 100644 --- a/scripts/generate_readme.py +++ b/scripts/generate_readme.py @@ -711,7 +711,9 @@ def contributing_text( ## Add a BIOS file 1. Fork this repository -2. Place the file in `bios/Manufacturer/Console/filename` +2. Place the file in `bios/Manufacturer/Console/filename`. + `python scripts/validate_pr.py path/to/file` names the folder its system + is kept in, and whether a platform or an emulator profile declares it 3. Variants (alternate hashes for the same file): `bios/Manufacturer/Console/.variants/` 4. Open a pull request. Its hashes are checked automatically and reported in a comment diff --git a/scripts/validate_pr.py b/scripts/validate_pr.py index 393c2e6a..7932c84e 100644 --- a/scripts/validate_pr.py +++ b/scripts/validate_pr.py @@ -171,18 +171,37 @@ def load_platform_hashes(platforms_dir: str, db: dict | None = None) -> dict: _note_sizes(known["sizes"], [file_entry["name"]], file_entry) for value in (*sha1s, *md5s): systems_of.setdefault(value, set()).add(system_id) - held = next((s for s in sha1s if s in files), None) or next( - (by_md5[m] for m in md5s if m in by_md5), None - ) - parts = str(files.get(held, {}).get("path", "")).split("/") - if len(parts) >= 4 and parts[0] == "bios" and not parts[2].startswith("."): - folder = "/".join(parts[:3]) + "/" - counts = folders_of.setdefault(system_id, {}) - counts[folder] = counts.get(folder, 0) + 1 + _note_folder(folders_of, system_id, files, by_md5, sha1s, md5s) - # A system whose files sit under several manufacturers (arcade sets, - # computers sharing ROM chips) names no single home: only a system - # filed under one manufacturer says where a newcomer belongs. + known["folders"] = _system_homes(systems_of, folders_of) + return known + + +def _note_folder( + folders_of: dict[str, dict[str, int]], system_id: str, files: dict, + by_md5: dict, sha1s: list[str], md5s: list[str], +) -> None: + """Count the bios/// folder a held file sits in.""" + held = next((s for s in sha1s if s in files), None) or next( + (by_md5[m] for m in md5s if m in by_md5), None + ) + parts = str(files.get(held, {}).get("path", "")).split("/") + if len(parts) >= 4 and parts[0] == "bios" and not parts[2].startswith("."): + folder = "/".join(parts[:3]) + "/" + counts = folders_of.setdefault(system_id, {}) + counts[folder] = counts.get(folder, 0) + 1 + + +def _system_homes( + systems_of: dict[str, set[str]], folders_of: dict[str, dict[str, int]] +) -> dict[str, set[str]]: + """Hash value to the folders the collection files its systems under. + + A system whose files sit under several manufacturers (arcade sets, + computers sharing ROM chips) names no single home: only a system filed + under one manufacturer says where a newcomer belongs. + """ + homes: dict[str, set[str]] = {} for value, systems in systems_of.items(): folders: set[str] = set() for system_id in systems: @@ -190,19 +209,33 @@ def load_platform_hashes(platforms_dir: str, db: dict | None = None) -> dict: if len({folder.split("/")[1] for folder in held_in}) == 1: folders.update(held_in) if folders: - known["folders"][value] = folders - return known + homes[value] = folders + return homes -def load_profile_hashes(emulators_dir: str) -> dict[str, dict[str, set[str]]]: +def load_profile_hashes( + emulators_dir: str, db: dict | None = None +) -> dict[str, dict[str, set[str]]]: """Hash value to the emulators whose profile declares it, per algorithm. A file only a core reads (a standalone emulator's firmware, a game engine's data) is in no platform list: the review called every such - contribution "not referenced, needs manual review". + contribution "not referenced, needs manual review". With the database, + `folders` maps each declared hash to the folders the collection files + the entry's system under, as load_platform_hashes does: a contributor + holding a file only a profile declares was told to pick a + bios/Manufacturer/Console/ folder alone. """ - known: dict[str, dict] = {"sha1": {}, "sha256": {}, "md5": {}, "crc32": {}, "sizes": {}} + known: dict[str, dict] = { + "sha1": {}, "sha256": {}, "md5": {}, "crc32": {}, "sizes": {}, "folders": {}, + } + systems_of: dict[str, set[str]] = {} + folders_of: dict[str, dict[str, int]] = {} + files = (db or {}).get("files", {}) + indexes = (db or {}).get("indexes", {}) + by_md5, by_sha256 = indexes.get("by_md5", {}), indexes.get("by_sha256", {}) for emulator, profile in load_emulator_profiles(emulators_dir).items(): + declared_systems = profile.get("systems") or [] for entry in profile.get("files") or []: if not isinstance(entry, dict): continue @@ -212,9 +245,23 @@ def load_profile_hashes(emulators_dir: str) -> dict[str, dict[str, set[str]]]: # sha256), and a crc32 counts only where no stronger hash is # declared, bounded by the declared size (626 entries name a # file by crc32 alone, clk's Electron os.rom among them). - for algo in ("sha1", "sha256", "md5"): - for value in parse_hash_list(entry.get(algo)): + hashes = { + algo: parse_hash_list(entry.get(algo)) for algo in ("sha1", "sha256", "md5") + } + for algo, values in hashes.items(): + for value in values: known[algo].setdefault(value, set()).add(emulator) + # The entry's own system, or the profile's when it emulates one. + system = entry.get("system") or ( + declared_systems[0] if len(declared_systems) == 1 else None + ) + if isinstance(system, str): + for value in (v for values in hashes.values() for v in values): + systems_of.setdefault(value, set()).add(system) + sha1s = [*hashes["sha1"], *( + by_sha256[v] for v in hashes["sha256"] if v in by_sha256 + )] + _note_folder(folders_of, system, files, by_md5, sha1s, hashes["md5"]) if not any(entry.get(algo) for algo in ("sha1", "sha256", "md5")): size = entry.get("size") sizes = [int(v) for v in size] if isinstance(size, list) else ( @@ -224,6 +271,7 @@ def load_profile_hashes(emulators_dir: str) -> dict[str, dict[str, set[str]]]: for accepted in sizes: key = (value.zfill(8), accepted) known["crc32"].setdefault(key, set()).add(emulator) + known["folders"] = _system_homes(systems_of, folders_of) return known @@ -335,14 +383,20 @@ def validate_file( ) normalized = os.path.normpath(relative) - expected_folders = sorted( - platform_hashes.get("folders", {}).get(result.sha1) - or platform_hashes.get("folders", {}).get(result.md5) or set() + # A platform's declaration first, then a profile's: most files the gaps + # page lists are declared by a profile alone. + expected = next( + ( + table["folders"][value] + for table in (platform_hashes, profile_hashes) + for value in (result.sha1, result.md5, hashes.get("sha256", "")) + if value in table.get("folders", {}) + ), + set(), ) + expected_folders = sorted(expected) if normalized.startswith("bios" + os.sep): parts = normalized.split(os.sep) - expected = (platform_hashes.get("folders", {}).get(result.sha1) - or platform_hashes.get("folders", {}).get(result.md5) or set()) here = f"bios/{parts[1]}/{parts[2]}/" if len(parts) >= 4 else "" makers = {folder.split("/")[1] for folder in expected} if expected and (len(parts) < 4 or parts[1] not in makers): @@ -578,7 +632,7 @@ def main() -> None: db = _load_database_if_any(args.db) platform_hashes = load_platform_hashes(args.platforms_dir, db) - profile_hashes = load_profile_hashes(args.emulators_dir) + profile_hashes = load_profile_hashes(args.emulators_dir, db) results = [] for f in files: diff --git a/tests/test_validate_pr.py b/tests/test_validate_pr.py index 941ad32b..b5acf0c5 100644 --- a/tests/test_validate_pr.py +++ b/tests/test_validate_pr.py @@ -99,6 +99,35 @@ class AFileOnlyACoreReadsIsKnown(_Fixture): result.checks) self.assertFalse(any("not referenced" in message for _s, message in result.checks)) + def test_a_missing_file_is_told_the_folder_of_its_system(self): + """The folder came from platform lists only: a file only a profile + declares, absent from the collection, got the generic advice.""" + import hashlib # noqa: PLC0415 + + held, missing = b"held rom", b"missing rom" + emulators = self.tmp / "emulators" + emulators.mkdir() + (emulators / "engine.yml").write_text( + "emulator: Engine\ntype: standalone\nsystems: [x, y]\nfiles:\n" + f" - name: \"a.bin\"\n system: x\n sha1: \"{hashlib.sha1(held).hexdigest()}\"\n" + f" - name: \"b.bin\"\n system: x\n md5: \"{hashlib.md5(missing).hexdigest()}\"\n" + ) + db = {"files": {hashlib.sha1(held).hexdigest(): {"path": "bios/Maker/Console/a.bin"}}, + "indexes": {"by_md5": {}, "by_sha256": {}}} + known = validate_pr.load_profile_hashes(str(emulators), db) + result = validate_pr.validate_file( + self._bios_file("Downloads/b.bin", missing), db, EMPTY_HASHES, known, + ) + self.assertIn( + ("WARN", "File is not under bios/ yet. Its system's files are in " + "`bios/Maker/Console/`"), + result.checks, + ) + placed = validate_pr.validate_file( + self._bios_file("bios/Other/Place/b.bin", missing), db, EMPTY_HASHES, known, + ) + self.assertTrue(any("Placed under bios/Other/Place/" in m for _s, m in placed.checks)) + class ASizeEveryDeclarationRulesOutIsNotAVariant(_Fixture): """1024 random bytes named scph5501.bin, a name every declaration sizes From c3500c25b7ded2f8e0062ab1ba3860ea2bf92fb4 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:13:37 +0200 Subject: [PATCH 12/12] docs: say the floor test imports under 3.9 --- wiki/testing-guide.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/wiki/testing-guide.md b/wiki/testing-guide.md index da47320f..f2bd1328 100644 --- a/wiki/testing-guide.md +++ b/wiki/testing-guide.md @@ -22,7 +22,9 @@ python -m unittest tests.test_install tests.test_install_cores -v The table below says which module covers what. -Running the build needs Python 3.9 or newer and `pyyaml` alone. CI runs the suite on 3.12. The +Running the build needs Python 3.9 or newer and `pyyaml` alone. CI runs the suite on 3.12, +and one test imports every module under `scripts/` with a real `python3.9`, which +needs `pyyaml` and `jsonschema` installed for that interpreter. The schema contract tests also need `jsonschema[format-nongpl]`, and one site test renders a wiki line with `markdown`. The CI installs both: without them those tests are reported as skipped, so install them before a pull request