diff --git a/scripts/export_native.py b/scripts/export_native.py index efa42ef2..98ea7454 100644 --- a/scripts/export_native.py +++ b/scripts/export_native.py @@ -429,26 +429,89 @@ def refresh_cache( return True, [f"{len(wanted)} file(s) fetched"] -@dataclass -class LandedChanges: - """What an export changed in the platform's file, field by field.""" - - applied: list[str] = field(default_factory=list) - filled: list[str] = field(default_factory=list) - resized: list[str] = field(default_factory=list) - unsized: list[str] = field(default_factory=list) - cleared: list[str] = field(default_factory=list) - requirements: int = 0 - - 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") return f"{number} {noun if number == 1 else plural}" +@dataclass +class LandedChanges: + """What an export changed in the platform's file, field by field. + + `records` holds each change with its value before and after, so a + maintainer can check it against the diff. The lists of labels are read + from it, for the summary and the page. + """ + + records: list[dict] = field(default_factory=list) + + def _labels(self, kind: str) -> list[str]: + return [ + f"{r['system']}/{r['name']} {r['field']}" + for r in self.records if r["kind"] == kind + ] + + @property + def applied(self) -> list[str]: + return self._labels("hash_corrected") + + @property + def filled(self) -> list[str]: + return self._labels("hash_filled") + + @property + def cleared(self) -> list[str]: + return self._labels("hash_cleared") + + @property + def resized(self) -> list[str]: + return [label.removesuffix(" size") for label in self._labels("size_corrected")] + + @property + def unsized(self) -> list[str]: + return [label.removesuffix(" size") for label in self._labels("size_removed")] + + @property + def requirements(self) -> int: + return sum(r["kind"] == "requirement_corrected" for r in self.records) + + +def _change( + kind: str, entry: object, field_name: str | None, old: object, new: object +) -> dict: + return { + "kind": kind, + "system": entry.native_system, + "name": entry.name, + "destination": entry.destination, + "field": field_name, + "old": old, + "new": new, + } + + +def _platform_value(entry: object, field_name: str) -> str: + return ",".join(_hash_values(entry.platform or {}, field_name)) + + +def _added_fields(exporter: object, entry: object) -> dict: + """The fields an addition is written with, as the render chooses them.""" + carried = exporter.carries() + written: dict[str, object] = { + name: value + for name in HASH_FIELDS + if name in carried and (value := exporter.written(entry, name)) + } + if "size" in carried and entry.size() is not None: + written["size"] = entry.size() + if "required" in carried: + written["required"] = entry.required + return written + + def landed_changes(exporter: object, systems: dict) -> LandedChanges: - """The changes the written file carries. + """The changes the written file carries, with their values. 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 @@ -456,26 +519,39 @@ def landed_changes(exporter: object, systems: dict) -> LandedChanges: did not do. A hash written where the platform left none changes the check from existence to content, so it is counted too. The truth taking over an entry also rewrites its size and drops the platform's hashes it - does not declare: both are in the diff. + does not declare: both are in the diff. An addition carries the fields + it is written with as its new value. """ changes = LandedChanges() + records = changes.records for system in systems.values(): for entry in system.files: - label = f"{entry.native_system}/{entry.name}" + if entry.platform is None: + if entry.name and exporter.lands(entry): + records.append(_change( + "added", entry, None, None, _added_fields(exporter, entry) + )) + continue for field_name in entry.corrections: if not exporter.states(entry, field_name): continue if field_name == "required": - changes.requirements += 1 + records.append(_change( + "requirement_corrected", entry, "required", + bool(entry.platform.get("required")), entry.required, + )) else: - changes.applied.append(f"{label} {field_name}") - changes.filled.extend( - f"{label} {field_name}" + records.append(_change( + "hash_corrected", entry, field_name, + _platform_value(entry, field_name), + exporter.written(entry, field_name), + )) + records.extend( + _change("hash_filled", entry, field_name, None, + exporter.written(entry, field_name)) for field_name in entry.filled if exporter.fills(entry, field_name) ) - if entry.platform is None: - continue theirs = entry.platform.get("size") ours = entry.size() if ( @@ -484,7 +560,9 @@ def landed_changes(exporter: object, systems: dict) -> LandedChanges: and _single_size(theirs) not in (None, ours) and exporter.states(entry, "size") ): - changes.resized.append(label) + records.append(_change( + "size_corrected", entry, "size", _single_size(theirs), ours + )) elif ( theirs and ours is None @@ -493,9 +571,12 @@ def landed_changes(exporter: object, systems: dict) -> LandedChanges: ): # The truth took the entry over without a size of its own: # System.dat's neocdz.zip line went out without size 953078. - changes.unsized.append(label) - changes.cleared.extend( - f"{label} {field_name}" + records.append(_change( + "size_removed", entry, "size", _single_size(theirs), None + )) + records.extend( + _change("hash_cleared", entry, field_name, + _platform_value(entry, field_name), None) for field_name in HASH_FIELDS if _hash_values(entry.platform, field_name) and not entry.hashes(field_name) @@ -503,7 +584,10 @@ def landed_changes(exporter: object, systems: dict) -> LandedChanges: ) # A value the render rewrote though the model reads it as the # platform's own (RomM's sha1 with a leading space). - changes.applied.extend(getattr(exporter, "normalized", [])) + records.extend( + _change("hash_corrected", entry, field_name, old, new) + for entry, field_name, old, new in getattr(exporter, "normalized_values", []) + ) return changes @@ -661,6 +745,7 @@ def export_platform( "not_added": refused, "not_kept": lost, }, + "changes": changes.records, "hashes_corrected": applied, "hashes_filled": filled, "sizes_corrected": resized, diff --git a/scripts/exporter/base_exporter.py b/scripts/exporter/base_exporter.py index c8875836..31056015 100644 --- a/scripts/exporter/base_exporter.py +++ b/scripts/exporter/base_exporter.py @@ -180,6 +180,17 @@ class BaseExporter(ABC): return "" return fe.hash(field_name) + def written(self, fe: NativeFile, field_name: str) -> str: + """The value the render writes for one hash field, comma-joined. + + The report gives each change its value before and after, so it reads + the value the way the render chooses it: one md5 where the format + checks one, every accepted value where it lists them. + """ + if field_name == self.SINGLE_HASH_FIELD: + return self.single_value(fe, field_name) + return ",".join(fe.hashes(field_name)) + def states(self, fe: NativeFile, field_name: str) -> bool: """Whether the written file carries this entry's corrected field. diff --git a/scripts/exporter/misterfpga_exporter.py b/scripts/exporter/misterfpga_exporter.py index ed5da927..7869a336 100644 --- a/scripts/exporter/misterfpga_exporter.py +++ b/scripts/exporter/misterfpga_exporter.py @@ -64,6 +64,10 @@ class Exporter(BaseExporter): return False return super().states(fe, field_name) + def written(self, fe: NativeFile, field_name: str) -> str: + """An entry is checked against one md5, the first accepted.""" + return fe.hash(field_name) + def _by_path(self, systems: dict[str, NativeSystem]) -> dict[str, object]: indexed: dict[str, object] = {} for system in systems.values(): diff --git a/scripts/exporter/retrodeck_exporter.py b/scripts/exporter/retrodeck_exporter.py index 03694079..346c5052 100644 --- a/scripts/exporter/retrodeck_exporter.py +++ b/scripts/exporter/retrodeck_exporter.py @@ -90,6 +90,12 @@ class Exporter(BaseExporter): return False return super().writable(fe, require) + def written(self, fe: NativeFile, field_name: str) -> str: + """md5 is a list of every accepted dump, sha256 a single value.""" + if field_name == "sha256": + return fe.hash(field_name) + return super().written(fe, field_name) + def states(self, fe: NativeFile, field_name: str) -> bool: if id(fe) in getattr(self, "_appended", ()): return False diff --git a/scripts/exporter/romm_exporter.py b/scripts/exporter/romm_exporter.py index d6427d7d..2786ae3b 100644 --- a/scripts/exporter/romm_exporter.py +++ b/scripts/exporter/romm_exporter.py @@ -113,6 +113,7 @@ class Exporter(BaseExporter): } self._first_addition = {} self.normalized: list[str] = [] + self.normalized_values: list[tuple[NativeFile, str, str, str]] = [] for system in sorted(systems.values(), key=lambda s: s.native_id): for fe in sorted(system.files, key=lambda f: f.name): key = (fe.native_system, fe.name) @@ -159,6 +160,10 @@ class Exporter(BaseExporter): does not keep.""" return bool(_hash_values(fe.platform or {}, name)) and not fe.hashes(name) + def written(self, fe: NativeFile, field_name: str) -> str: + """A record holds one value per field, the one record_hash names.""" + return fe.record_hash(field_name) + def fills(self, fe: NativeFile, field_name: str) -> bool: return self.states(fe, field_name) and self.record_fills(fe, field_name) @@ -179,6 +184,7 @@ class Exporter(BaseExporter): and name not in fe.corrections ): self.normalized.append(f"{fe.native_system}/{fe.name} {name}") + self.normalized_values.append((fe, name, str(theirs), value)) entry[field] = value def validate( diff --git a/scripts/exporter/systemdat_exporter.py b/scripts/exporter/systemdat_exporter.py index 042a6357..a42aa6c8 100644 --- a/scripts/exporter/systemdat_exporter.py +++ b/scripts/exporter/systemdat_exporter.py @@ -154,6 +154,10 @@ class Exporter(BaseExporter): ) return lines + def written(self, fe: NativeFile, field_name: str) -> str: + """A record holds one value per field, the one record_hash names.""" + return fe.record_hash(field_name) + def fills(self, fe: NativeFile, field_name: str) -> bool: return self.states(fe, field_name) and self.record_fills(fe, field_name) diff --git a/scripts/siteexports.py b/scripts/siteexports.py index 51906782..e378295b 100644 --- a/scripts/siteexports.py +++ b/scripts/siteexports.py @@ -27,6 +27,60 @@ CHANGE_KINDS = ( ) +# Report key, then the kind each line of `changes` carries. +CHANGE_RECORDS = { + "hashes_corrected": "hash_corrected", + "hashes_filled": "hash_filled", + "sizes_corrected": "size_corrected", + "sizes_removed": "size_removed", + "hashes_cleared": "hash_cleared", +} + + +def _value(value: object) -> str: + return f"`{value}`" + + +def describe_change(change: dict) -> str: + """One change in words, with the value before and after it.""" + label = f"`{change['system']}/{change['name']}` {change['field']}" + kind, old, new = change["kind"], change["old"], change["new"] + if kind in ("hash_corrected", "size_corrected"): + return f"{label} {_value(old)} becomes {_value(new)}" + if kind == "hash_filled": + return f"{label} {_value(new)} added" + if kind == "requirement_corrected": + return f"`{change['system']}/{change['name']}` becomes {'required' if new else 'optional'}" + return f"{label} {_value(old)} no longer listed" + + +def change_items(report: dict, key: str) -> list[str]: + """The lines a kind of change shows. A report written before values + were recorded names the entries only.""" + kind = CHANGE_RECORDS.get(key) + changes = report.get("changes") + if kind and changes is not None: + return [describe_change(c) for c in changes if c["kind"] == kind] + items = report.get(key) or [] + return [str(i) if key == "withheld" else f"`{i}`" for i in items] + + +def requirement_lines(report: dict) -> list[str]: + """Each required flag aligned with the code, and what it is now.""" + items = [ + describe_change(c) for c in report.get("changes") or [] + if c["kind"] == "requirement_corrected" + ] + if not items: + return [] + return [ + f'??? note "Required flags aligned with the code ({len(items)})"', + "", + *[f" - {item}" for item in items], + "", + ] + + def _plural(count: int, noun: str, plural: str = "") -> str: return f"{count:,} {noun if count == 1 else plural or noun + 's'}" @@ -241,14 +295,15 @@ def generate_exports_page(reports: list[dict], names: dict[str, str]) -> str: (name, url), = originals.items() lines.extend([f"Based on [`{name}`]({url}).", ""]) + lines.extend(requirement_lines(report)) for key, title in CHANGE_KINDS: - items = report.get(key) or [] + items = change_items(report, key) if not items: continue lines.extend([ f'??? note "{title} ({len(items)})"', "", - *[f" - {item}" if key == "withheld" else f" - `{item}`" for item in items], + *[f" - {item}" for item in items], "", ]) return "\n".join(lines) diff --git a/tests/test_export_report.py b/tests/test_export_report.py index c976dbec..77db124d 100644 --- a/tests/test_export_report.py +++ b/tests/test_export_report.py @@ -8,6 +8,7 @@ file from and no list to check the diff against. from __future__ import annotations import json +import re import sys import tempfile import unittest @@ -157,6 +158,182 @@ class ThePageAddressesMaintainersAsPeers(unittest.TestCase): self.assertNotIn(word, page) +UPSTREAM_CACHE = ROOT / ".cache" / "upstream-native" +_DAT_ROM = re.compile(r"^\trom \( name (\"[^\"]+\"|\S+) (.*) \)$") +_DAT_FIELDS = {"size": "size", "crc": "crc32", "md5": "md5", "sha1": "sha1"} +_HASH_KINDS = { + "hash_corrected", "hash_filled", "hash_cleared", "size_corrected", "size_removed", +} + + +def _norm(value: object) -> str | None: + """A value as the frontend reads it: a list split on commas, each value + stripped, so the layout of a wrapped attribute is not a change.""" + if value in (None, ""): + return None + return ",".join(v.strip() for v in str(value).casefold().split(",")) + + +def _parse_dat(text: str) -> dict[tuple[str, str], dict[str, str]]: + """Each rom line of a System.dat, keyed by its section and rom name.""" + roms: dict[tuple[str, str], dict[str, str]] = {} + section = "" + for line in text.splitlines(): + if line.startswith('\tcomment "'): + section = line.split('"')[1] + continue + match = _DAT_ROM.match(line) + if not match: + continue + tokens = match.group(2).split() + fields = { + _DAT_FIELDS[tokens[i]]: tokens[i + 1] + for i in range(0, len(tokens) - 1, 2) if tokens[i] in _DAT_FIELDS + } + roms.setdefault((section, match.group(1).strip('"')), fields) + return roms + + +def _parse_es_bios(text: str) -> dict[tuple[str, str], dict[str, str]]: + """Each bios element of an es_bios.xml, keyed by system and path.""" + from common import parse_untrusted_xml + + root = parse_untrusted_xml(text) + elements: dict[tuple[str, str], dict[str, str]] = {} + for system in root.iter("system"): + for bios in system.iter("bios"): + elements.setdefault((system.get("platform", ""), bios.get("path", "")), { + "md5": bios.get("md5", ""), + "required": "false" if bios.get("mandatory") == "false" else "true", + }) + return elements + + +class ReportedChangesAreTheDiff(unittest.TestCase): + """The report gives each change its value before and after. Read back + from the original and the updated file, the two must agree entry by + entry, or a maintainer checks the diff against a list that differs.""" + + def _run(self, platform: str, filename: str) -> tuple[list[dict], str, str, dict]: + from common import load_platform_config, native_layer + from exporter import discover_exporters + from exporter.baseline import build_native_model + + original_path = UPSTREAM_CACHE / platform / filename + if not original_path.is_file(): + self.skipTest(f"no cached original for {platform}") + config = load_platform_config(platform, str(ROOT / "platforms")) + truth = export_native.current_truth(platform, config, str(ROOT / "platforms")) + scraped = native_layer(config) + systems, report = build_native_model(truth, scraped) + exporter = discover_exporters()[platform]() + original = original_path.read_text(encoding="utf-8") + originals = { + p.name: p.read_text(encoding="utf-8") + for p in (UPSTREAM_CACHE / platform).iterdir() if p.is_file() + } + produced = exporter.render(systems, report, originals, scraped)[filename] + changes = export_native.landed_changes(exporter, systems).records + return changes, original, produced, {"exporter": exporter, "systems": systems} + + @staticmethod + def _diff(before: dict, after: dict) -> tuple[set, set]: + changed = { + (key, name, _norm(before[key].get(name)), _norm(after[key].get(name))) + for key in before.keys() & after.keys() + for name in before[key].keys() | after[key].keys() + if _norm(before[key].get(name)) != _norm(after[key].get(name)) + } + return changed, set(after) - set(before) + + def test_system_dat(self): + changes, original, produced, ctx = self._run("retroarch", "System.dat") + exporter = ctx["exporter"] + rom_name = { + (fe.native_system, fe.name, fe.destination): exporter._rom_name(fe) + for system in ctx["systems"].values() for fe in system.files + } + changed, added = self._diff(_parse_dat(original), _parse_dat(produced)) + reported = { + ((c["system"], rom_name[(c["system"], c["name"], c["destination"])]), + c["field"], _norm(c["old"]), _norm(c["new"])) + for c in changes if c["kind"] in _HASH_KINDS + } + self.assertTrue(changed, "nothing changed, the comparison proves nothing") + self.assertEqual(reported, changed) + reported_added = { + (c["system"], rom_name[(c["system"], c["name"], c["destination"])]) + for c in changes if c["kind"] == "added" + } + self.assertEqual(reported_added, added) + + def test_recalbox(self): + changes, original, produced, ctx = self._run("recalbox", "es_bios.xml") + exporter = ctx["exporter"] + path_of = { + (fe.native_system, fe.name, fe.destination): exporter._path(fe) + for system in ctx["systems"].values() for fe in system.files + } + changed, added = self._diff(_parse_es_bios(original), _parse_es_bios(produced)) + reported = { + ((c["system"], path_of[(c["system"], c["name"], c["destination"])]), + c["field"], _norm(c["old"]), _norm(c["new"])) + for c in changes + if c["kind"] in _HASH_KINDS or c["kind"] == "requirement_corrected" + } + self.assertTrue(changed, "nothing changed, the comparison proves nothing") + self.assertEqual(reported, changed) + reported_added = { + (c["system"], path_of[(c["system"], c["name"], c["destination"])]) + for c in changes if c["kind"] == "added" + } + self.assertEqual(reported_added, added) + + +class ThePageShowsEachValueBeforeAndAfter(unittest.TestCase): + """A list of names gave a maintainer nothing to check a correction + against: each line now says what the value was and what it becomes.""" + + def setUp(self): + def change(kind: str, field: str | None, old: object, new: object) -> dict: + return {"kind": kind, "system": "psx", "name": "scph5501.bin", + "destination": "scph5501.bin", "field": field, + "old": old, "new": new} + + self.report = _report( + "recalbox", ["es_bios.xml"], + hashes_corrected=["psx/scph5501.bin md5"], requirements_corrected=1, + changes=[ + change("hash_corrected", "md5", "5626" + "0" * 28, "c523" + "1" * 28), + change("requirement_corrected", "required", True, False), + change("hash_filled", "sha1", None, "f" * 40), + ], + ) + self.page = generate_exports_page([self.report], {}) + + def test_a_corrected_hash_shows_both_values(self): + self.assertIn( + f" - `psx/scph5501.bin` md5 `5626{'0' * 28}` becomes `c523{'1' * 28}`", + self.page, + ) + + def test_a_requirement_names_the_entry_and_its_value(self): + self.assertIn('??? note "Required flags aligned with the code (1)"', self.page) + self.assertIn(" - `psx/scph5501.bin` becomes optional", self.page) + + def test_a_filled_hash_shows_its_value(self): + self.assertIn(f" - `psx/scph5501.bin` sha1 `{'f' * 40}` added", self.page) + + def test_no_arrow_glyph(self): + for glyph in (chr(0x2192), "->", "=>"): + self.assertNotIn(glyph, self.page) + + def test_the_report_carries_every_change(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('"changes": changes.records,', body) + + class RemovalCandidatesNeedAReadingNoCoreAnswers(unittest.TestCase): """An entry the truth does not pair can still be read: by an alias, by its content, under a directory a core walks. Of 2776 such entries, 1189 diff --git a/wiki/tools.md b/wiki/tools.md index 312d0cd1..d5cc06b8 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -271,6 +271,12 @@ state is reported rather than written: EmuDeck's arrays are shared between emulators its file does not name, so a hash is corrected in place and never added. +`exports/.json` lists every change under `changes`, each with +its kind, entry, field and the value before and after (an addition carries +the fields it is written with). The exports page shows both values, and a +test reads the original and the updated System.dat and es_bios.xml back to +check that the list is the diff. + No entry the platform declares is removed. One the truth does not pair is looked for in the profiles of every core the platform runs: by name or alias, by the basename or tail of a `path:`, by any hash (archive members