From e5b8cede928789cdeea249d4c2e2654695f141ed Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Sat, 17 Oct 2026 02:20:25 +0200 Subject: [PATCH] fix: agree on missing files in system packs The emulator pack recorded only loose files as a path's occupant, so a path an archive or data tree had filled was reported missing even with the same bytes in the ZIP, and an archive several profiles ask for was counted once per profile. verify registered neither, and credited one profile's file at a path holding another's. Both now record archives and tree members in pack order; --system snk-neogeo-mvs went from 113 missing in the pack and 51 in verify to 59 on both sides. --- scripts/generate_pack.py | 26 ++++++++++--- scripts/verify.py | 52 ++++++++++++++++++++++--- tests/test_system_pack_parity.py | 65 ++++++++++++++++++++++++++++++++ 3 files changed, 133 insertions(+), 10 deletions(-) create mode 100644 tests/test_system_pack_parity.py diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index ec6175fb..a5aae92e 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -1176,11 +1176,14 @@ def _pack_data_tree( seen_destinations: set[str], seen_parents: set[str], seen_lower: set[str], + packed_at: dict[str, str] | None = None, ) -> int: """Write one data directory into an emulator pack, under its shared lock. A refresh swaps the tree by rename; without the lock a walk half done - read the old tree and wrote files of the new one. + read the old tree and wrote files of the new one. ``packed_at`` records + each member, so a later profile asking for the same bytes there is + served rather than reported missing. """ added = 0 with cache_lock(local_cache, shared=True): @@ -1197,6 +1200,8 @@ def _pack_data_tree( _register_path(full, seen_destinations, seen_parents) seen_lower.add(full.lower()) _add_pack_member(zf, src, full) + if packed_at is not None: + packed_at[full.lower()] = src added += 1 return added @@ -1294,6 +1299,15 @@ def generate_emulator_pack( # Loose file packed at each folded destination, to tell a profile asking # for the same file there from one asking for other bytes. packed_at: dict[str, str] = {} + # A destination is missing once, however many profiles ask for it: + # verify --emulator counts by destination. + missing_at: set[str] = set() + + def report_missing(name: str, destination: str) -> None: + if destination not in missing_at: + missing_at.add(destination) + missing_files.append(name) + data_dir_notices: list[str] = [] data_registry = load_data_dir_registry( platforms_dir or os.path.join(os.path.dirname(__file__), "..", "platforms") @@ -1348,9 +1362,10 @@ def generate_emulator_pack( seen_destinations.add(archive_dest) _register_path(archive_dest, seen_destinations, seen_parents) seen_lower.add(archive_dest.lower()) + packed_at[archive_dest.lower()] = local_path total_files += 1 else: - missing_files.append(archive_name) + report_missing(archive_name, archive_dest) # Pack individual files (skip archived ones). The entries of one # profile on one destination are the releases the code accepts @@ -1456,7 +1471,7 @@ def generate_emulator_pack( ): # The file already at this path, asked for twice. continue - missing_files.append(alternatives[0]["name"]) + report_missing(alternatives[0]["name"], dest) continue if chosen is None: @@ -1465,7 +1480,7 @@ def generate_emulator_pack( elif all(fe.get("unsourceable") for fe in alternatives): unsourceable_files.append(alternatives[0]["name"]) else: - missing_files.append(alternatives[0]["name"]) + report_missing(alternatives[0]["name"], dest) continue fe, local_path, how = chosen @@ -1535,7 +1550,8 @@ def generate_emulator_pack( if prefix: dd_dest = f"{prefix}/{dd_dest}" if dd_dest else prefix total_files += _pack_data_tree( - zf, local_cache, dd_dest, seen_destinations, seen_parents, seen_lower + zf, local_cache, dd_dest, seen_destinations, seen_parents, seen_lower, + packed_at, ) # Remove empty ZIP (no files packed and no missing = nothing to ship) diff --git a/scripts/verify.py b/scripts/verify.py index bddfc2ef..2aef7ddf 100644 --- a/scripts/verify.py +++ b/scripts/verify.py @@ -1466,9 +1466,28 @@ def verify_emulator( sha1_by_path = { entry.get("path"): sha1 for sha1, entry in db.get("files", {}).items() } + # Destination -> (file packed there, its profile, the path as written). + # Archives and data trees take paths too, before and after the loose + # files of their profile, as the pack writes them. placed: dict[str, tuple[str | None, str, str]] = {} placed_files: set[str] = set() placed_parents: set[str] = set() + content_ids: dict[str, str | None] = {} + + def content_id(path: str | None) -> str | None: + if not path: + return None + if path not in content_ids: + content_ids[path] = sha1_by_path.get(path) or ( + compute_hashes(path)["sha1"] if os.path.isfile(path) else None + ) + return content_ids[path] + + def occupy(dest: str, path: str | None, owner: str) -> None: + if placement_block(dest, placed_files, set(placed), placed_parents, True): + return + placed[dest.lower()] = (path, owner, dest) + _register_path(dest, placed_files, placed_parents) for emu_name, profile in sorted(selected): files = expand_directory_entries( filter_files_by_mode(profile.get("files", []), standalone), @@ -1559,6 +1578,8 @@ def verify_emulator( archive, profile.get("archive_prefix", ""), profile.get("pack_structure"), standalone, ) + if local_path: + occupy(dest, local_path, emu_name) dest_to_name[dest] = archive cur = result["status"] prev = file_status.get(dest) @@ -1690,20 +1711,22 @@ def verify_emulator( ) block_reason = "" if block and held is not None: - other_sha1, owner, other_dest = placed.get(dest.lower(), (None, "", "")) + other_path, owner, other_dest = placed.get(dest.lower(), (None, "", "")) + same = content_id(held) is not None and content_id(held) == content_id( + other_path + ) if block == "path_conflict": block_reason = "a packed file occupies its directory" elif block == "case_twin": block_reason = ( f"packed only as {other_dest}, from {owner}" - if sha1_by_path.get(held) == other_sha1 + if same else f"the pack holds {owner}'s {other_dest} at this path" ) - elif sha1_by_path.get(held) != other_sha1: + elif not same: block_reason = f"the pack holds {owner}'s {other_dest} at this path" elif not block and packed is not None: - placed[dest.lower()] = (sha1_by_path.get(packed), emu_name, dest) - _register_path(dest, placed_files, placed_parents) + occupy(dest, packed, emu_name) for result, required, hle in alternatives: if block_reason and result["status"] != Status.MISSING: result["status"] = Status.MISSING @@ -1726,6 +1749,25 @@ def verify_emulator( ) > _SEVERITY_ORDER.get(prev_sev, 0): file_severity[dest] = sev + # The profile's data trees follow its files in the pack. + for dd in filter_files_by_mode(profile.get("data_directories", []), standalone): + cache = (data_registry or {}).get(dd.get("ref", ""), {}).get("local_cache", "") + if not cache or not os.path.isdir(cache): + continue + dd_dest = dd.get("destination", "") + structure = profile.get("pack_structure") + prefix = ( + structure.get("standalone" if standalone else "libretro", "") + if structure else "" + ) + if prefix: + dd_dest = f"{prefix}/{dd_dest}" if dd_dest else prefix + for root, _dirs, names in os.walk(cache): + for fname in names: + src = os.path.join(root, fname) + rel = os.path.relpath(src, cache).replace(os.sep, "/") + occupy(f"{dd_dest}/{rel}" if dd_dest else rel, src, emu_name) + counts = { Severity.OK: 0, Severity.INFO: 0, diff --git a/tests/test_system_pack_parity.py b/tests/test_system_pack_parity.py new file mode 100644 index 00000000..cc4401e6 --- /dev/null +++ b/tests/test_system_pack_parity.py @@ -0,0 +1,65 @@ +"""A system pack and verify --system name the same missing files. + +The pack registered only loose files as occupants: a destination an earlier +profile's archive or data tree had filled was reported missing even when +the bytes were the same and in the ZIP, and an archive several profiles ask +for was counted once per profile. verify, in turn, never registered archives +or trees and credited a profile's file at a path holding another profile's. +`--system snk-neogeo-mvs` said 113 missing where verify said 51. +""" + +from __future__ import annotations + +import json +import re +import shutil +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[1] + + +def _run(*argv: str) -> str: + result = subprocess.run( + [sys.executable, *argv], cwd=REPO_ROOT, capture_output=True, text=True, + timeout=1800, + ) + if result.returncode != 0: + raise AssertionError(result.stdout + result.stderr) + return result.stdout + + +class SystemPackMatchesVerify(unittest.TestCase): + SYSTEMS = ("snk-neogeo-mvs", "msx") + + def setUp(self): + if not (REPO_ROOT / "database.json").exists(): + self.skipTest("database.json is not generated") + self.out = Path(tempfile.mkdtemp(dir=REPO_ROOT / "tmp")) + self.addCleanup(shutil.rmtree, self.out, True) + + def test_the_same_files_are_missing(self): + for system in self.SYSTEMS: + with self.subTest(system=system): + packed = _run( + "scripts/generate_pack.py", "--system", system, "--offline", + "--output-dir", str(self.out / system), + ) + report = json.loads(_run("scripts/verify.py", "--system", system, "--json")) + pack_missing = sorted(re.findall(r"^ MISSING: (\S+)", packed, re.M)) + # One per destination, as the pack reports them; an entry the + # collection cannot hold is counted apart on both sides. + verify_missing = sorted({ + (d["destination"], d["name"]) for d in report["details"] + if d["status"] == "missing" and not d.get("unsourceable") + }) + verify_missing = sorted(name for _dest, name in verify_missing) + self.assertTrue(pack_missing or verify_missing) + self.assertEqual(pack_missing, verify_missing) + + +if __name__ == "__main__": + unittest.main()