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()