From 95d10ee812d0ebee76f0c1b9266755b4a0dbad68 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Tue, 6 Oct 2026 01:22:56 +0200 Subject: [PATCH] fix: count what each export format writes --- scripts/export_native.py | 38 +++++-- scripts/exporter/base_exporter.py | 9 ++ scripts/exporter/baseline.py | 27 ++++- scripts/exporter/emudeck_exporter.py | 25 ++++- scripts/exporter/retrobat_exporter.py | 22 +++- scripts/exporter/retrodeck_exporter.py | 87 +++++++++++++--- tests/test_export_counts.py | 139 +++++++++++++++++++++++++ 7 files changed, 319 insertions(+), 28 deletions(-) create mode 100644 tests/test_export_counts.py diff --git a/scripts/export_native.py b/scripts/export_native.py index a687a949..97a3d8ff 100644 --- a/scripts/export_native.py +++ b/scripts/export_native.py @@ -267,16 +267,28 @@ def export_platform( path.parent.mkdir(parents=True, exist_ok=True) path.write_text(content, encoding="utf-8") - # Only what the format can state: a correction to a field the file has - # no place for is not a change the maintainer will find in the diff, and - # counting it would announce work the export did not do. - carried = exporter.carries() - applied = [ - correction - for correction in report.hashes_corrected - if correction.rsplit(" ", 1)[-1] in carried - ] - requirements = len(report.required_corrected) if "required" in carried else 0 + # Only what the format states: a correction to a field the file has no + # place for, or one its render keeps out, is not a change the maintainer + # will find in the diff, and counting it would announce work the export + # did not do. A hash written where the platform left none changes the + # check from existence to content, so it is counted too. + applied: list[str] = [] + filled: list[str] = [] + requirements = 0 + for system in systems.values(): + for entry in system.files: + for field_name in entry.corrections: + if not exporter.states(entry, field_name): + continue + if field_name == "required": + requirements += 1 + else: + applied.append(f"{entry.native_system}/{entry.name} {field_name}") + filled.extend( + f"{entry.native_system}/{entry.name} {field_name}" + for field_name in entry.filled + if exporter.states(entry, field_name) + ) landed = 0 refused = 0 @@ -300,6 +312,8 @@ def export_platform( f"{report.files_kept} kept, {landed} added, " f"{len(applied)} hashes corrected, {requirements} requirements corrected" ) + if filled: + summary += f", {len(filled)} hashes filled" if refused: summary += f", {refused} the format cannot state" if lost: @@ -309,6 +323,10 @@ def export_platform( messages.append(f"hash corrected: {correction}") if len(applied) > 5: messages.append(f"and {len(applied) - 5} more hash corrections") + for fill in filled[:5]: + messages.append(f"hash filled: {fill}") + if len(filled) > 5: + messages.append(f"and {len(filled) - 5} more hash fills") messages.extend(f"INVALID: {issue}" for issue in issues[:10]) if len(issues) > 10: diff --git a/scripts/exporter/base_exporter.py b/scripts/exporter/base_exporter.py index 85ae7c34..d4189bb8 100644 --- a/scripts/exporter/base_exporter.py +++ b/scripts/exporter/base_exporter.py @@ -133,6 +133,15 @@ class BaseExporter(ABC): require = require or cls.requires() return not require or bool(fe.hash(require)) + def states(self, fe: NativeFile, field_name: str) -> bool: + """Whether the written file carries this entry's corrected field. + + The summary counts what lands, read entry by entry: a correction the + model makes and the format keeps out of the file is not a change the + maintainer will find in the diff. + """ + return field_name in self.carries() + def outcome( self, systems: dict[str, NativeSystem], diff --git a/scripts/exporter/baseline.py b/scripts/exporter/baseline.py index 8f2f53fd..704752d0 100644 --- a/scripts/exporter/baseline.py +++ b/scripts/exporter/baseline.py @@ -48,6 +48,8 @@ class NativeFile: platform: dict | None = None truth: dict | None = None corrections: list[str] = field(default_factory=list) + # Hash fields the platform left empty and the truth fills. + filled: list[str] = field(default_factory=list) @property def origin(self) -> str: @@ -104,7 +106,16 @@ class NativeFile: return None def size(self) -> int | None: - for entry in (self.truth, self.platform): + """Size of the content the written hashes describe. + + hashes() keeps the platform's values when the truth has none, so the + truth's size is only taken when the truth also speaks for the hash: + a name-matched 480-byte fbneo boot.bin turned RomM's Dreamcast + boot.bin into size 480 beside its 2 MB md5, which never verifies. + """ + truth_hashed = any(_hash_values(self.truth or {}, f) for f in HASH_FIELDS) + order = (self.truth, self.platform) if truth_hashed else (self.platform, self.truth) + for entry in order: if entry and entry.get("size"): return int(entry["size"]) return None @@ -310,6 +321,8 @@ def build_native_model( report.hashes_corrected.append( f"{matched.native_system}/{matched.name} {field_name}" ) + elif ours and not theirs: + matched.filled.append(field_name) t_req = truth_entry.get("required") p_req = (matched.platform or {}).get("required") if ( @@ -323,6 +336,18 @@ def build_native_model( ) continue + # A file another core's entry already claimed is still the + # platform's file, not a new one: scph101.bin, declared by two + # PSX cores, was written into RetroDECK's manifest a second time. + declared = [ + candidate + for native_id in target_ids + for candidate in systems.get(native_id, NativeSystem(native_id)).files + if candidate.platform is not None + ] + if any(by_destination(c) or by_name(c) for c in declared): + continue + # The truth knows a file the platform does not declare. native_id = target_ids[0] system = systems.get(native_id) diff --git a/scripts/exporter/emudeck_exporter.py b/scripts/exporter/emudeck_exporter.py index df400a0e..aef7d45e 100644 --- a/scripts/exporter/emudeck_exporter.py +++ b/scripts/exporter/emudeck_exporter.py @@ -18,7 +18,7 @@ sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) from scraper.emudeck_scraper import FUNCTION_HASH_MAP, _RE_FUNC, _RE_LOCAL_HASHES from .base_exporter import BaseExporter -from .baseline import NativeSystem, Report +from .baseline import NativeFile, NativeSystem, Report, _hash_values SOURCE_URL = ( "https://raw.githubusercontent.com/dragoonDorise/EmuDeck/main" @@ -71,6 +71,27 @@ class Exporter(BaseExporter): """ return False + @staticmethod + def _entry_md5s(fe: NativeFile) -> list[str]: + """The values one platform entry puts in the array, corrected in place. + + An entry keeps its own values unless the truth contradicts all of + them, and then the truth's replace them. An entry without a hash + adds nothing, and the truth's extra accepted revisions are not + appended: either would grow an array other consumers read too. + """ + theirs = _hash_values(fe.platform or {}, "md5") + if not theirs: + return [] + if "md5" in fe.corrections: + return fe.hashes("md5") + return theirs + + def states(self, fe: NativeFile, field_name: str) -> bool: + if field_name in fe.filled: + return False + return super().states(fe, field_name) + @classmethod def _md5s(cls, systems: dict[str, NativeSystem], system_id: str) -> list[str]: """Every MD5 the system accepts, in a stable order, deduplicated.""" @@ -81,7 +102,7 @@ class Exporter(BaseExporter): for fe in system.files: if not cls.writable(fe): continue - for value in fe.hashes("md5"): + for value in cls._entry_md5s(fe): if _MD5.match(value) and value not in seen: seen.append(value) return seen diff --git a/scripts/exporter/retrobat_exporter.py b/scripts/exporter/retrobat_exporter.py index 57875e6b..04d4c133 100644 --- a/scripts/exporter/retrobat_exporter.py +++ b/scripts/exporter/retrobat_exporter.py @@ -10,7 +10,7 @@ import json from collections import OrderedDict from .base_exporter import BaseExporter -from .baseline import NativeSystem, Report +from .baseline import NativeFile, NativeSystem, Report SOURCE_URL = ( "https://raw.githubusercontent.com/RetroBat-Official/emulatorlauncher/master" @@ -41,6 +41,24 @@ class Exporter(BaseExporter): def native_sources() -> dict[str, str]: return {"batocera-systems.json": SOURCE_URL} + @staticmethod + def _md5(fe: NativeFile) -> str: + """The one md5 the entry is checked against, empty for existence. + + RetroBat writes an empty md5 to check a file by existence. Filling it + turns that into a content check on a single value: right when the + truth accepts exactly one image, wrong when the core takes several + and the field can hold only the first. + """ + if "md5" in fe.filled and len(fe.hashes("md5")) != 1: + return "" + return fe.hash("md5") + + def states(self, fe: NativeFile, field_name: str) -> bool: + if field_name == "md5" and "md5" in fe.filled: + return bool(self._md5(fe)) + return super().states(fe, field_name) + def render( self, systems: dict[str, NativeSystem], @@ -71,7 +89,7 @@ class Exporter(BaseExporter): bios_files = [] for fe in files: entry: OrderedDict[str, str] = OrderedDict() - entry["md5"] = fe.hash("md5") + entry["md5"] = self._md5(fe) declared = fe.native("native_path", "") entry["file"] = str(declared) if declared else f"bios/{fe.destination}" bios_files.append(entry) diff --git a/scripts/exporter/retrodeck_exporter.py b/scripts/exporter/retrodeck_exporter.py index 61bb03a1..946571fe 100644 --- a/scripts/exporter/retrodeck_exporter.py +++ b/scripts/exporter/retrodeck_exporter.py @@ -19,6 +19,10 @@ COMPONENTS_REPO = "RetroDECK/components" COMPONENTS_BRANCH = "main" RAW_BASE = f"https://raw.githubusercontent.com/{COMPONENTS_REPO}/{COMPONENTS_BRANCH}" MANIFEST = "component_manifest.json" +# Labels that only say yes or no, so a corrected requirement can be written +# over them. Every other label is a sentence ("At least one BIOS file +# required", "Required for some Japanese games.") and is the platform's own. +PLAIN_LABELS = ("Required", "Optional") class Exporter(BaseExporter): @@ -42,6 +46,42 @@ class Exporter(BaseExporter): # one from BIOS data alone would throw the component away. return True + @classmethod + def writable(cls, fe: NativeFile, require: str = "") -> bool: + """An addition lands only in a manifest it can be placed in.""" + if fe.platform is None and not fe.native("component", ""): + return False + return super().writable(fe, require) + + def states(self, fe: NativeFile, field_name: str) -> bool: + if field_name == "required": + label = str(fe.native("required_label", "")) + return not label or label in PLAIN_LABELS + return super().states(fe, field_name) + + @staticmethod + def _place_additions(systems: dict[str, NativeSystem]) -> None: + """Give each addition the component its system already lives in. + + A component is one emulator's manifest, and only the platform's + entries name it. A system whose entries all sit in one component + takes the addition there; one spread over several, or with none of + its own, cannot say which manifest is meant, and the addition is + left out and counted as such. + """ + for system in systems.values(): + components = { + str(fe.native("component", "")) + for fe in system.files + if fe.platform is not None and fe.native("component", "") + } + if len(components) != 1: + continue + component = components.pop() + for fe in system.files: + if fe.platform is None and fe.truth is not None: + fe.truth = {**fe.truth, "component": component} + @staticmethod def component_url(component: str) -> str: return f"{RAW_BASE}/{component}/{MANIFEST}" @@ -73,11 +113,13 @@ class Exporter(BaseExporter): # RetroDECK words the requirement in prose ("Required", "At least one # BIOS file required"), so the platform's own wording is kept and a # boolean is only rendered when there is none to keep. - label = fe.native("required_label", "") - if label: - entry["required"] = str(label) + label = str(fe.native("required_label", "")) + if label and label not in PLAIN_LABELS: + entry["required"] = label elif fe.required: entry["required"] = "Required" + elif label: + entry["required"] = "Optional" destination = fe.destination if destination and destination not in (fe.name, f"bios/{fe.name}"): directory = destination.rsplit("/", 1)[0] @@ -116,29 +158,47 @@ class Exporter(BaseExporter): truth has something to say and left alone where it does not, and what the platform does not declare is appended. """ - by_name: OrderedDict[str, OrderedDict] = OrderedDict() + # Keyed by name AND system: the retroarch manifest declares + # ATARIOSB.ROM for atari5200 and atari800 with different md5 lists, + # and a name-only key wrote the first of ours over both. + by_key: OrderedDict[tuple[str, str], OrderedDict] = OrderedDict() for entry in ours: - name = str(entry.get("filename", "")) - if name and name not in by_name: - by_name[name] = entry + key = (str(entry.get("filename", "")), str(entry.get("system", ""))) + if key[0] and key not in by_key: + by_key[key] = entry merged: list[OrderedDict] = [] - corrected: set[str] = set() + corrected: set[tuple[str, str]] = set() for entry in existing if isinstance(existing, list) else []: if not isinstance(entry, dict): continue name = str(entry.get("filename", "")) - ours_entry = by_name.get(name) - if ours_entry is None: + declared = entry.get("system") + # One entry can serve several systems: neogeo.zip is declared + # once for neogeo, fbneo and arcade. + systems = ( + [str(s) for s in declared] if isinstance(declared, list) + else [str(declared)] if declared else [] + ) + keys = [(name, s) for s in systems if (name, s) in by_key] + if not systems: + # An entry without a system matches ours only when one + # system alone declares the name. + named = [k for k in by_key if k[0] == name] + keys = named if len(named) == 1 else [] + if not keys: merged.append(OrderedDict(entry)) continue combined = OrderedDict(entry) - combined.update(ours_entry) + combined.update( + (field, value) for field, value in by_key[keys[0]].items() + if not (field == "system" and declared) + ) merged.append(combined) - corrected.add(name) + corrected.update(keys) merged.extend( - entry for name, entry in by_name.items() if name not in corrected + entry for key, entry in by_key.items() if key not in corrected ) return merged @@ -149,6 +209,7 @@ class Exporter(BaseExporter): originals: dict[str, str], scraped: dict | None = None, ) -> dict[str, str]: + self._place_additions(systems) grouped = self._by_component(systems) produced: dict[str, str] = {} diff --git a/tests/test_export_counts.py b/tests/test_export_counts.py new file mode 100644 index 00000000..2238f556 --- /dev/null +++ b/tests/test_export_counts.py @@ -0,0 +1,139 @@ +"""An export writes what its summary announces, and nothing else. + +RetroDECK announced 1474 additions and wrote none, counted 161 requirement +corrections while keeping the platform's prose labels, and wrote one +system's ATARIOSB.ROM over another's. RetroBat and EmuDeck turned empty or +partial hash lists into content checks nobody counted, and a size was taken +from a file other than the one the hash describes. +""" + +from __future__ import annotations + +import json +import sys +import unittest +from collections import OrderedDict +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(REPO_ROOT / "scripts")) + +from exporter.baseline import NativeFile, NativeSystem, build_native_model # noqa: E402 +from exporter.emudeck_exporter import Exporter as EmuDeck # noqa: E402 +from exporter.retrobat_exporter import Exporter as RetroBat # noqa: E402 +from exporter.retrodeck_exporter import Exporter as RetroDeck # noqa: E402 + +A = "a" * 32 +B = "b" * 32 +C = "c" * 32 + + +class RetroDeckWritesWhatItCounts(unittest.TestCase): + def test_same_name_in_two_systems_stays_two_entries(self): + existing = [ + {"filename": "ATARIOSB.ROM", "system": "atari5200", "md5": f"{A},{B}"}, + {"filename": "ATARIOSB.ROM", "system": "atari800", "md5": B}, + ] + ours = [ + OrderedDict(filename="ATARIOSB.ROM", system="atari800", md5=C), + ] + merged = RetroDeck._merge(existing, ours) + by_system = {e["system"]: e["md5"] for e in merged} + self.assertEqual(by_system, {"atari5200": f"{A},{B}", "atari800": C}) + self.assertEqual(len(merged), 2) + + def test_a_list_of_systems_is_kept(self): + existing = [{"filename": "neogeo.zip", "system": ["neogeo", "fbneo"], "md5": A}] + ours = [OrderedDict(filename="neogeo.zip", system="fbneo", md5=B)] + merged = RetroDeck._merge(existing, ours) + self.assertEqual(len(merged), 1) + self.assertEqual(merged[0]["system"], ["neogeo", "fbneo"]) + self.assertEqual(merged[0]["md5"], B) + + def test_prose_label_is_neither_rewritten_nor_counted(self): + exporter = RetroDeck() + prose = NativeFile( + "x.bin", "bios/x.bin", "psx", + platform={"required": True, "required_label": "At least one BIOS file required"}, + truth={"required": False}, + ) + plain = NativeFile( + "y.bin", "bios/y.bin", "psx", + platform={"required": True, "required_label": "Required"}, + truth={"required": False}, + ) + self.assertFalse(exporter.states(prose, "required")) + self.assertEqual(RetroDeck._entry(prose)["required"], "At least one BIOS file required") + self.assertTrue(exporter.states(plain, "required")) + self.assertEqual(RetroDeck._entry(plain)["required"], "Optional") + + def test_addition_goes_to_the_system_component(self): + declared = NativeFile("a.bin", "bios/a.bin", "psx", + platform={"component": "duckstation", "md5": A}) + added = NativeFile("b.bin", "bios/b.bin", "psx", truth={"md5": B}) + spread = NativeSystem("amiga", files=[ + NativeFile("k1", "bios/k1", "amiga", platform={"component": "retroarch"}), + NativeFile("k2", "bios/k2", "amiga", platform={"component": "puae"}), + NativeFile("k3", "bios/k3", "amiga", truth={"md5": C}), + ]) + systems = {"psx": NativeSystem("psx", files=[declared, added]), "amiga": spread} + manifest = json.dumps({"duckstation": {"name": "DuckStation", "bios": []}}) + produced = RetroDeck().render( + systems, None, {"duckstation/component_manifest.json": manifest} + ) + written = json.loads(produced["duckstation/component_manifest.json"]) + names = [e["filename"] for e in written["duckstation"]["bios"]] + self.assertIn("b.bin", names) + self.assertFalse(RetroDeck.writable(spread.files[2])) + + +class HashesTheFormatCannotHold(unittest.TestCase): + def test_retrobat_fills_only_a_single_accepted_value(self): + exporter = RetroBat() + single = NativeFile("s.bin", "s.bin", "nds", platform={"md5": ""}, + truth={"md5": A}, filled=["md5"]) + several = NativeFile("m.bin", "m.bin", "nds", platform={"md5": ""}, + truth={"md5": [A, B]}, filled=["md5"]) + self.assertEqual(RetroBat._md5(single), A) + self.assertTrue(exporter.states(single, "md5")) + self.assertEqual(RetroBat._md5(several), "") + self.assertFalse(exporter.states(several, "md5")) + + def test_emudeck_never_grows_an_array(self): + unhashed = NativeFile("scph5501.bin", "scph5501.bin", "psx", + platform={"md5": ""}, truth={"md5": C}, filled=["md5"]) + extra = NativeFile("scph1001.bin", "scph1001.bin", "psx", + platform={"md5": A}, truth={"md5": [A, B]}) + corrected = NativeFile("scph7001.bin", "scph7001.bin", "psx", + platform={"md5": B}, truth={"md5": C}, + corrections=["md5"]) + systems = {"psx": NativeSystem("psx", files=[unhashed, extra, corrected])} + self.assertEqual(EmuDeck._md5s(systems, "psx"), [A, C]) + self.assertFalse(EmuDeck().states(unhashed, "md5")) + + +class ModelKeepsOneFileOneEntry(unittest.TestCase): + def test_size_describes_the_hash_written(self): + fe = NativeFile("boot.bin", "dc/boot.bin", "dc", + platform={"size": 2097152, "md5": A}, truth={"size": 480}) + self.assertEqual(fe.size(), 2097152) + hashed = NativeFile("boot.bin", "dc/boot.bin", "dc", + platform={"size": 2097152, "md5": A}, + truth={"size": 480, "md5": B}) + self.assertEqual(hashed.size(), 480) + + def test_second_core_entry_for_a_declared_file_adds_nothing(self): + scraped = {"systems": {"psx": {"files": [ + {"name": "scph101.bin", "destination": "bios/scph101.bin", "md5": A}, + ]}}} + truth = {"systems": {"psx": {"files": [ + {"name": "scph101.bin", "path": "scph101.bin", "md5": A}, + {"name": "scph101.bin", "path": "scph101.bin", "md5": B}, + ]}}} + systems, report = build_native_model(truth, scraped) + self.assertEqual(report.files_added, 0) + self.assertEqual(len(systems["psx"].files), 1) + + +if __name__ == "__main__": + unittest.main()