diff --git a/scripts/export_native.py b/scripts/export_native.py index b198dd10..3491ce67 100644 --- a/scripts/export_native.py +++ b/scripts/export_native.py @@ -36,6 +36,7 @@ from artifacts import file_lock, write_bytes_atomic, write_if_changed, write_tex import yaml from common import ( + build_zip_contents_index, platform_arg, list_registered_platforms, load_data_dir_registry, @@ -50,6 +51,7 @@ from common import ( from exporter import discover_exporters from exporter.baseline import HASH_FIELDS, _hash_values, _single_size, build_native_model from exporter.candidates import removal_candidates +from exporter.doubled import doubled_destinations from truth import generate_platform_truth DEFAULT_CACHE = ".cache/upstream-native" @@ -436,6 +438,24 @@ def refresh_cache( return True, [f"{len(wanted)} file(s) fetched"] +@functools.cache +def _collection(db_path: str) -> tuple[dict, dict]: + """The database and its archive index, read once for every platform.""" + db = load_database(db_path) + return db, build_zip_contents_index(db) + + +def doubled_in(scraped: dict | None, platforms_dir: str) -> list[dict]: + """The destinations the platform's own list declares for two files.""" + if not scraped: + return [] + db, zip_contents = _collection(TRUTH_DB) + return doubled_destinations( + scraped, load_emulator_profiles("emulators"), db, zip_contents, + load_data_dir_registry(platforms_dir), + ) + + def _count(number: int, noun: str, plural: str = "") -> str: """"1 hash", "2 hashes": a count with its noun in the right number.""" plural = plural or noun + ("es" if noun.endswith(("sh", "ch", "s", "x")) else "s") @@ -764,6 +784,8 @@ def export_platform( systems, scraped, load_emulator_profiles("emulators"), load_data_dir_registry(platforms_dir), ) + # Reported, never rewritten: both declarations stay as the platform wrote them. + doubled = doubled_in(scraped, platforms_dir) messages.append(summary) if unwritten: messages.append("not in the file: " + ", ".join(unwritten)) @@ -774,6 +796,11 @@ def export_platform( + (f" ({open_question} undetermined)" if open_question else "") + ", listed in the report" ) + if doubled: + messages.append( + f"{_count(len(doubled), 'destination')} declared twice for two files" + ", listed in the report" + ) messages.extend(f"withheld: {reason}" for reason in withheld) for heading, labels in ( ("hash updated", applied), @@ -807,6 +834,7 @@ def export_platform( "withheld": list(withheld), "found_by_content": report.labels, "removal_candidates": candidates, + "doubled": doubled, "invalid": issues, }) return True, messages diff --git a/scripts/exporter/doubled.py b/scripts/exporter/doubled.py new file mode 100644 index 00000000..e0e33720 --- /dev/null +++ b/scripts/exporter/doubled.py @@ -0,0 +1,137 @@ +"""Destinations a platform's own list declares twice, for two files. + +One path holds one file, so on an install one of the two checks always +fails. RetroDECK declares bios/disk.rom for the PC-88 core and for XRoar, +with two md5 that name two different ROMs. slots.find_collisions finds +these. The export keeps both entries and names, beside each declaration, +the emulators whose profile reads the file it declares. +""" + +from __future__ import annotations + +import os +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from common import ( + contents_members, + holds_required_members, + platform_layouts, + resolve_platform_cores, + standalone_set, +) +from exporter.baseline import HASH_FIELDS, _hash_values +from slots import _accepted_by_all, _lists_hashes, find_collisions +from validation import opens_by_name, outside_gap_scope + + +def _names(entry: dict) -> set[str]: + """The file names a profile entry is opened under.""" + names = {entry.get("name"), *(entry.get("aliases") or [])} + for key in ("path", "standalone_path"): + if entry.get(key): + names.add(str(entry[key]).rsplit("/", 1)[-1]) + return {str(n).casefold() for n in names if n} + + +def _size_fits(entry: dict, local: str) -> bool: + """Whether the file's size is one the entry states, or none is stated.""" + try: + size = os.path.getsize(local) + except OSError: + return False + declared = entry.get("size") + sizes = declared if isinstance(declared, list) else [declared] + sizes = [s for s in sizes if isinstance(s, int)] + if sizes and size not in sizes: + return False + if isinstance(entry.get("min_size"), int) and size < entry["min_size"]: + return False + return not (isinstance(entry.get("max_size"), int) and size > entry["max_size"]) + + +def _reads(entry: dict, by_name: bool, local: str, db: dict) -> bool: + """Whether a profile entry describes the file at local. + + An archive is read by its members, a dump by its hashes. A name the code + opens with no hash to check is read by any copy of a size it accepts. A + name that is only a label (a core identifying files by content) reads + nothing by name. + """ + if entry.get("contents"): + return holds_required_members(local, contents_members(entry)) + if _lists_hashes(entry): + return _accepted_by_all(local, [entry], db) + return by_name and _size_fits(entry, local) + + +def _hashes(entry: dict) -> dict[str, list[str]]: + found = {f: _hash_values(entry, f) for f in HASH_FIELDS} + return {f: values for f, values in found.items() if values} + + +def doubled_destinations( + scraped: dict | None, + profiles: dict[str, dict], + db: dict, + zip_contents: dict | None = None, + data_registry: dict | None = None, +) -> list[dict]: + """Each destination the platform declares twice for two files. + + Every declaration is listed with its system, component, hashes and the + file it resolves to, and the emulators the platform runs whose profile + reads that file there, with the profile's source_ref. Nothing is + removed from the platform's file. + """ + if not scraped: + return [] + collisions = find_collisions(scraped, db, zip_contents, data_registry) + if not collisions: + return [] + standalone = standalone_set(scraped) + named: dict[str, list[tuple[str, dict, dict, bool]]] = {} + for key in sorted(resolve_platform_cores(scraped, profiles)): + profile = profiles[key] + if profile.get("type") in ("launcher", "alias"): + continue + for layout, is_standalone in platform_layouts(key, profile, standalone): + for entry in layout.get("files") or []: + if not isinstance(entry, dict) or outside_gap_scope(entry, is_standalone): + continue + by_name = opens_by_name(entry, layout, is_standalone) + for name in _names(entry): + named.setdefault(name, []).append((key, layout, entry, by_name)) + + doubled = [] + for collision in collisions: + base = collision.destination.rsplit("/", 1)[-1].casefold() + declarations = [] + for entry, local in collision.declarations: + readers: list[dict] = [] + for key, layout, profile_entry, by_name in named.get(base, []): + if not local or not _reads(profile_entry, by_name, local, db): + continue + reader = { + "emulator": layout.get("emulator") or key, + "profile": key, + "entry": profile_entry.get("name", ""), + "source_ref": profile_entry.get("source_ref", ""), + } + if reader not in readers: + readers.append(reader) + declarations.append({ + "system": entry.get("native_system") or "", + "component": entry.get("component") or "", + "name": entry.get("name", ""), + "hashes": _hashes(entry), + "file": local, + "readers": readers, + }) + doubled.append({ + "destination": collision.destination, + "declarations": declarations, + }) + return doubled diff --git a/scripts/siteexports.py b/scripts/siteexports.py index f8c7a7c2..d6be34f4 100644 --- a/scripts/siteexports.py +++ b/scripts/siteexports.py @@ -156,6 +156,49 @@ def candidate_lines(candidates: list[dict]) -> list[str]: return lines +def _declaration(declaration: dict) -> str: + """One declaration of a doubled destination: who declares it, its hash, + and the emulators that read the file it names.""" + where = f"`{declaration['system']}`" + if declaration.get("component"): + where += f" (`{declaration['component']}`)" + parts = [where] + [ + f"{field} {_value(values[0] if len(values) == 1 else values)}" + for field, values in (declaration.get("hashes") or {}).items() + ] + readers = declaration.get("readers") or [] + read = ( + "read by " + ", ".join( + r["emulator"] + (f" (`{r['source_ref']}`)" if r.get("source_ref") else "") + for r in readers + ) + if readers else "no emulator this platform runs is known to read this file" + ) + return " ".join(parts) + f": {read}" + + +def doubled_lines(doubled: list[dict]) -> list[str]: + """The destinations the original declares twice, for two files. + + One path holds one file, so one of the two checks fails on every + install. Both entries stay in the updated file. + """ + if not doubled: + return [] + count = _plural(len(doubled), "destination") + lines = [ + f"{count} of the original {'is' if len(doubled) == 1 else 'are'} declared " + "twice, for two different files. One path holds one file, so one of the " + "two checks fails on every install. Both entries stay in the updated file.", + "", + ] + for item in doubled: + lines.append(f"- `{item['destination']}`") + lines.extend(f" - {_declaration(d)}" for d in item["declarations"]) + lines.append("") + return lines + + def load_export_reports(directory: str = EXPORTS_DIR) -> list[dict]: """Every platform report the export wrote, by platform name.""" root = Path(directory) @@ -266,6 +309,7 @@ def generate_exports_page(reports: list[dict], names: dict[str, str]) -> str: "", ]) lines.extend(candidate_lines(report.get("removal_candidates") or [])) + lines.extend(doubled_lines(report.get("doubled") or [])) if report.get("invalid"): lines.extend(markup.aside( "warning", "The updated file does not pass the platform's own checks", diff --git a/scripts/slots.py b/scripts/slots.py index f2478ac4..5002ce86 100644 --- a/scripts/slots.py +++ b/scripts/slots.py @@ -675,6 +675,8 @@ class Collision: destination: str resolved: list[str] + # Each declaration with the file it resolved to, in the platform's order. + declarations: list[tuple[dict, str | None]] = field(default_factory=list) def _collision_json(collision: Collision) -> dict: @@ -714,6 +716,7 @@ def find_collisions( if len(entries) < 2: continue resolved = [] + declarations = [] for entry in entries: local, _ = resolve_local_file( entry, @@ -722,6 +725,7 @@ def find_collisions( dest_hint=entry.get("destination", ""), data_dir_registry=data_dir_registry, ) + declarations.append((entry, local)) if local and local not in resolved: resolved.append(local) if len(resolved) > 1 and not _same_file_family(resolved): @@ -730,7 +734,9 @@ def find_collisions( # also names the first one's file. if any(_accepted_by_all(path, entries, db) for path in resolved): continue - collisions.append(Collision(destination=key, resolved=resolved)) + collisions.append( + Collision(destination=key, resolved=resolved, declarations=declarations) + ) return collisions diff --git a/tests/test_export_report.py b/tests/test_export_report.py index 26c25fc4..5ebf2283 100644 --- a/tests/test_export_report.py +++ b/tests/test_export_report.py @@ -202,17 +202,23 @@ class TheRunReportStatesWhatTheFileCarries(unittest.TestCase): {"name": "a.bin", "destination": "a.bin", "native_system": "psx", "md5": "a" * 32}, ]}}} + doubled = {"destination": "a.bin", "declarations": []} with tempfile.TemporaryDirectory(dir=ROOT / "tmp") as scratch, \ mock.patch.object(export_native, "load_inputs", return_value=(truth, scraped)), \ mock.patch.object(export_native, "unpinned", return_value=[]), \ - mock.patch.object(export_native, "removal_candidates", return_value=[]): + mock.patch.object(export_native, "removal_candidates", return_value=[]), \ + mock.patch.object(export_native, "doubled_in", return_value=[doubled]): ok, messages = export_native.export_platform( "plat", Plain, Path(scratch), Path(scratch) / "out", str(ROOT / "platforms"), Path(scratch) / "cache", False, ) self.assertTrue(ok, messages) report = json.loads((Path(scratch) / "out" / "plat.json").read_text()) + self.assertEqual(report["doubled"], [doubled]) + self.assertIn( + "1 destination declared twice for two files, listed in the report", messages + ) self.assertEqual( report["summary"], "1 kept, 1 file added, 1 hash updated to the emulator source", @@ -542,5 +548,115 @@ class RemovalCandidatesNeedAReadingNoCoreAnswers(unittest.TestCase): self.assertIn('"removal_candidates": candidates,', body) +class ADestinationDeclaredTwiceIsNamed(unittest.TestCase): + """RetroDECK aims a PC-88 disk ROM and a CoCo disk ROM at one + bios/disk.rom, each with its own md5. One path holds one file, so one + check fails on every install. The export keeps both entries and names + the contradiction, with the emulators that read each file.""" + + PC88 = b"\x01" * 2048 + COCO = b"\x02" * 8194 + + def setUp(self): + import hashlib # noqa: PLC0415 + + self._tmp = tempfile.TemporaryDirectory(dir=ROOT / "tmp") + root = Path(self._tmp.name) + files, by_md5, by_name = {}, {}, {} + self.md5 = {} + for label, data, rel in ( + ("pc88", self.PC88, "bios/NEC/PC-88/N88SUB.ROM"), + ("coco", self.COCO, "bios/Tandy/CoCo/disk.rom"), + ): + path = root / rel + path.parent.mkdir(parents=True) + path.write_bytes(data) + sha1 = hashlib.sha1(data).hexdigest() + md5 = hashlib.md5(data).hexdigest() + self.md5[label] = md5 + files[sha1] = {"path": str(path), "name": path.name, "md5": md5, + "sha1": sha1, "crc32": "", "size": len(data)} + by_md5[md5] = sha1 + by_name.setdefault(path.name, []).append(sha1) + self.db = {"files": files, "indexes": { + "by_md5": by_md5, "by_name": by_name, "by_crc32": {}, "by_path_suffix": {}, + }} + self.scraped = {"cores": ["pcemu", "cocoemu"], "systems": { + "nec-pc-88": {"files": [ + {"name": "disk.rom", "destination": "bios/disk.rom", + "md5": self.md5["pc88"], "native_system": "pc88", + "component": "retroarch"}, + ]}, + "coco": {"files": [ + {"name": "disk.rom", "destination": "bios/disk.rom", + "md5": self.md5["coco"], "native_system": "coco", + "component": "xroar"}, + ]}, + }} + self.profiles = { + "pcemu": {"emulator": "PC Emu", "type": "libretro", "systems": ["nec-pc-88"], + "files": [{"name": "n88sub.rom", "aliases": ["disk.rom"], + "size": 2048, "source_ref": "libretro.c:120"}]}, + "cocoemu": {"emulator": "CoCo Emu", "type": "libretro", "systems": ["coco"], + "files": [{"name": "disk11.rom", "source_ref": "xroar.c:714"}]}, + } + + def tearDown(self): + self._tmp.cleanup() + + def _doubled(self) -> list[dict]: + from exporter.doubled import doubled_destinations # noqa: PLC0415 + + return doubled_destinations(self.scraped, self.profiles, self.db) + + def test_each_declaration_is_named_with_its_reader(self): + doubled = self._doubled() + self.assertEqual([d["destination"] for d in doubled], ["bios/disk.rom"]) + pc88, coco = doubled[0]["declarations"] + self.assertEqual((pc88["system"], pc88["component"]), ("pc88", "retroarch")) + self.assertEqual(pc88["hashes"], {"md5": [self.md5["pc88"]]}) + self.assertTrue(pc88["file"].endswith("bios/NEC/PC-88/N88SUB.ROM")) + self.assertEqual(pc88["readers"], [{ + "emulator": "PC Emu", "profile": "pcemu", "entry": "n88sub.rom", + "source_ref": "libretro.c:120", + }]) + # A size the profile states rules out the CoCo ROM, and XRoar's own + # profile never opens disk.rom. + self.assertEqual((coco["system"], coco["component"]), ("coco", "xroar")) + self.assertEqual(coco["hashes"], {"md5": [self.md5["coco"]]}) + self.assertEqual(coco["readers"], []) + + def test_the_platform_entries_are_left_as_written(self): + import copy # noqa: PLC0415 + + before = copy.deepcopy(self.scraped) + self._doubled() + self.assertEqual(self.scraped, before) + + def test_a_destination_both_hashes_accept_is_not_doubled(self): + self.scraped["systems"]["coco"]["files"][0]["md5"] = ( + f"{self.md5['coco']},{self.md5['pc88']}" + ) + self.assertEqual(self._doubled(), []) + + def test_the_run_counts_it_and_the_report_carries_it(self): + source = (ROOT / "scripts" / "export_native.py").read_text(encoding="utf-8") + body = source[source.index("def export_platform("):source.index("def write_report(")] + self.assertIn('"doubled": doubled,', body) + self.assertIn("declared twice for two files", body) + + def test_the_page_names_each_declaration(self): + page = generate_exports_page( + [_report("retrodeck", ["a.json"], doubled=self._doubled())], {}) + self.assertIn("1 destination of the original is declared twice", page) + self.assertIn("- `bios/disk.rom`", page) + self.assertIn( + f" - `pc88` (`retroarch`) md5 `{self.md5['pc88']}`: read by PC Emu " + "(`libretro.c:120`)", page) + self.assertIn( + f" - `coco` (`xroar`) md5 `{self.md5['coco']}`: no emulator this " + "platform runs is known to read this file", page) + + if __name__ == "__main__": unittest.main() diff --git a/wiki/tools.md b/wiki/tools.md index 5d5fa17e..4c00d968 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -350,6 +350,14 @@ profile (`common.unresolved_platform_cores`) or when the entry is a ROM set and a core of the system opens sets its profile does not list. BizHawk and MiSTer resolve no core, so nothing is examined and nothing is listed. +A destination the platform's own list declares twice, for two different +files, is written under `doubled` (`slots.find_collisions` finds it). One +path holds one file, so one of the two checks fails on every install. +Each declaration carries its system, component, hashes and the file it +resolves to, with the emulators the platform runs whose profile reads +that file: a member list it holds, a hash it matches, or a name and a +size it fits. Both entries stay in the updated file. + MAME and FBNeo entries are `.zip` ROM sets: their meaningful hashes belong to the files inside the archive, so the exported DAT lists those entries without a container sha1. Anyone submitting the DAT upstream should mention