diff --git a/scripts/dedup.py b/scripts/dedup.py index 585ca504..d59f0e59 100644 --- a/scripts/dedup.py +++ b/scripts/dedup.py @@ -28,7 +28,7 @@ from collections import defaultdict from pathlib import Path sys.path.insert(0, os.path.dirname(__file__)) -from common import compute_hashes +from common import compute_hashes, write_text_atomic DEFAULT_BIOS_DIR = "bios" @@ -168,6 +168,25 @@ def deduplicate(bios_dir: str, dry_run: bool = False) -> dict: Returns dict of {sha1: {"canonical": path, "removed": [paths], "aliases": [names]}} """ + # Beside the scanned tree, where get_mame_clone_map() reads it for + # the real layout. A path relative to the working directory let a + # scan of a fixture tree rewrite the repository's own map. + clone_path = os.path.join( + os.path.dirname(os.path.abspath(bios_dir)), "_mame_clones.json" + ) + # Read before anything is removed: the map is cumulative, and a run that + # stopped on an unreadable one after deleting clones would lose them. + try: + with open(clone_path) as f: + previous = json.load(f) + except FileNotFoundError: + previous = {} + except json.JSONDecodeError as e: + raise SystemExit( + f"{clone_path} is not valid JSON ({e}); restore it from git " + "before deduplicating again" + ) from e + sha1_groups = scan_duplicates(bios_dir) results = {} total_removed = 0 @@ -253,23 +272,12 @@ def deduplicate(bios_dir: str, dry_run: bool = False) -> dict: # Write MAME clone mapping if mame_clones: - # Beside the scanned tree, where get_mame_clone_map() reads it for - # the real layout. A path relative to the working directory let a - # scan of a fixture tree rewrite the repository's own map. - clone_path = os.path.join( - os.path.dirname(os.path.abspath(bios_dir)), "_mame_clones.json" - ) # 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 # therefore erases every mapping an earlier run recorded, and the # canonical zip stops answering to the names it stands in for. The # older entries stay valid: their canonical file is still here. if not dry_run: - try: - with open(clone_path) as f: - previous = json.load(f) - except (FileNotFoundError, json.JSONDecodeError): - previous = {} if isinstance(previous, dict): merged = dict(previous) merged.update(mame_clones) @@ -281,8 +289,7 @@ def deduplicate(bios_dir: str, dry_run: bool = False) -> dict: f"{sum(len(v['clones']) for v in mame_clones.values())} clones" ) else: - with open(clone_path, "w") as f: - json.dump(mame_clones, f, indent=2, sort_keys=True) + write_text_atomic(clone_path, json.dumps(mame_clones, indent=2, sort_keys=True)) print(f"\nWrote MAME clone map: {clone_path}") print( f" {len(mame_clones)} canonical ZIPs with " diff --git a/scripts/generate_truth.py b/scripts/generate_truth.py index 2dc815a0..38cc0d66 100644 --- a/scripts/generate_truth.py +++ b/scripts/generate_truth.py @@ -21,6 +21,7 @@ from common import ( load_platform_config, load_target_config, require_yaml, + write_text_atomic, yaml_load, ) from truth import generate_platform_truth @@ -129,14 +130,17 @@ def main(argv: list[str] | None = None) -> None: ) out_path = os.path.join(output_dir, f"{name}.yml") - with open(out_path, "w") as f: + # Whole or not at all: export_native and diff_truth read this file, + # and a run stopped midway left a truncated model behind. + write_text_atomic( + out_path, yaml.dump( result, - f, default_flow_style=False, sort_keys=False, allow_unicode=True, - ) + ), + ) n_systems = len(result.get("systems", {})) n_files = sum( diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index 9e19e73c..b9854954 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -2471,6 +2471,39 @@ class TheZipIndexCacheKnowsItsInputs(unittest.TestCase): ziptools._zip_contents_cache = None +class GeneratedModelsAreWrittenWhole(unittest.TestCase): + """The truth model and the cumulative MAME clone map were written by + truncate-then-write; a corrupt clone map also read as empty and was + then overwritten, losing what earlier runs recorded.""" + + def test_both_writers_replace_their_file_atomically(self): + for module in ("generate_truth.py", "dedup.py"): + source = (ROOT / "scripts" / module).read_text(encoding="utf-8") + with self.subTest(module=module): + self.assertIn("write_text_atomic(", source) + self.assertNotIn('open(out_path, "w")', source) + self.assertNotIn('open(clone_path, "w")', source) + + def test_an_unreadable_clone_map_stops_the_run(self): + from scripts import dedup + + with tempfile.TemporaryDirectory(dir=TMP_ROOT) as directory: + root = Path(directory) + bios = root / "bios" + # A device clone: two archive names, the same ROMs, in MAME's tree. + (bios / "Arcade" / "MAME").mkdir(parents=True) + for name in ("set.zip", "setclone.zip"): + with zipfile.ZipFile(bios / "Arcade" / "MAME" / name, "w") as archive: + archive.writestr("rom.bin", b"rom") + data = (bios / "Arcade" / "MAME" / "set.zip").read_bytes() + (bios / "Arcade" / "MAME" / "setclone.zip").write_bytes(data) + (root / "_mame_clones.json").write_text("<<<<<<< HEAD") + with self.assertRaises(SystemExit): + dedup.deduplicate(str(bios), dry_run=False) + # Stopped before removing anything. + self.assertTrue((bios / "Arcade" / "MAME" / "setclone.zip").exists()) + + class TheDatabaseCacheIsWrittenWhole(unittest.TestCase): """A crash while serializing left a truncated db_cache.json behind, and the next run rehashed the whole collection from scratch."""