diff --git a/scripts/common.py b/scripts/common.py index 4677c690..98839cb6 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -870,7 +870,29 @@ def archive_entry(archive: str, files: list[dict]) -> dict: def holds_required_members(path: str, members: list[dict]) -> bool: """Whether an archive holds every member a profile requires inside it.""" crcs = zip_member_crcs(path) - return all(m["crc32"] in crcs for m in members if m["required"]) + names = zip_member_names(path) + return all(_member_held(m, crcs, names) for m in members if m["required"]) + + +def contents_members(file_entry: dict) -> list[dict]: + """The members an archive entry's ``contents:`` lists, as ranking keys. + + A member is its crc32 where the profile declares one, its name where the + code opens it by name alone: Flycast reads epr-21329.ic27 in + hod2bios.zip by filename, and a copy without it has no Japan BIOS. + """ + return [ + {"crc32": str(m["crc32"]).lower(), "required": True} + if m.get("crc32") else {"name": str(m["name"]), "required": True} + for m in file_entry.get("contents") or [] + if m.get("crc32") or m.get("name") + ] + + +def _member_held(member: dict, crcs: frozenset[str], names: frozenset[str]) -> bool: + if member.get("crc32"): + return member["crc32"] in crcs + return member["name"].rsplit("/", 1)[-1].casefold() in names def _holding_members(paths: list[str], members: list[dict]) -> list[str]: @@ -881,16 +903,35 @@ def _holding_members(paths: list[str], members: list[dict]) -> list[str]: """ def held(path: str) -> tuple[int, int]: crcs = zip_member_crcs(path) - return ( - sum(1 for m in members if m["required"] and m["crc32"] in crcs), - sum(1 for m in members if m["crc32"] in crcs), - ) + names = zip_member_names(path) + found = [m for m in members if _member_held(m, crcs, names)] + return sum(1 for m in found if m["required"]), len(found) scores = {path: held(path) for path in paths} best = max(scores.values(), default=(0, 0)) return [path for path in paths if scores[path] == best] +def _covering_archives(paths: list[str]) -> list[str]: + """Same-named archives, those holding every ROM each other copy holds. + + Without a declared layout, a primary that is a superset of the other + primaries misses nothing any of them holds. Two copies that each hold a + ROM the other lacks are different revisions, and nothing here can + choose: the order is kept. Index order alone used to decide, and + deleting an unrelated duplicate flipped which copy a core received. + """ + # A variant is served only when no primary is, so the primaries are + # the copies this choice is between. + pool = [path for path in paths if "/.variants/" not in path] or paths + crcs = {path: zip_member_crcs(path) for path in pool} + covering = [ + path for path in pool + if crcs[path] and all(crcs[other] <= crcs[path] for other in pool) + ] + return covering + [path for path in paths if path not in covering] + + def _by_affinity(paths: list[str], file_entry: dict, dest_hint: str) -> list[str]: """Same-named candidates, the likeliest first. @@ -1231,8 +1272,11 @@ def resolve_local_file( if candidates: if zipped_file: candidates = [p for p in candidates if ".zip" in os.path.basename(p)] - if file_entry.get("members"): - candidates = _holding_members(candidates, file_entry["members"]) + members = file_entry.get("members") or contents_members(file_entry) + if members: + candidates = _holding_members(candidates, members) + elif len(candidates) > 1 and name.casefold().endswith(".zip"): + candidates = _covering_archives(candidates) candidates = _by_affinity(candidates, file_entry, dest_hint) # A copy outside another emulator's tree comes first: the # affinity read pcsx2 in bios/Other/armsx2/pcsx2/ and served @@ -2361,6 +2405,7 @@ from ziptools import ( # noqa: E402,F401 _zip_contents_cache, _BOUNDED_RATIO_METHODS, zip_member_crcs, + zip_member_names, ) from artifacts import ( # noqa: E402,F401 write_if_changed, diff --git a/scripts/packextras.py b/scripts/packextras.py index 773dd501..3a36f010 100644 --- a/scripts/packextras.py +++ b/scripts/packextras.py @@ -776,6 +776,8 @@ def _collect_emulator_extras( for identity in PROFILE_IDENTITY_FIELDS: if f.get(identity) not in (None, "", []): extra[identity] = f[identity] + if f.get("contents"): + extra["contents"] = f["contents"] extra["claim_order"] = (2, 0, emu_name) if _take(extra, full_dest, seen_dests, taken): extras.append(extra) @@ -823,6 +825,10 @@ def _report_extra(u: dict, name: str, dest: str) -> dict: } if u.get("members"): extra["members"] = u["members"] + # The archive layout the core opens ranks same-named copies by what + # they hold; without it the first copy indexed won. + if u.get("contents"): + extra["contents"] = u["contents"] # Keep every reproducible identity constraint carried by the source # profile. Some profiles expose CRC32/SHA-256 only through their # validation block; dropping those fields here would let a same-named diff --git a/scripts/verify.py b/scripts/verify.py index 756e3256..a2af70bd 100644 --- a/scripts/verify.py +++ b/scripts/verify.py @@ -638,6 +638,8 @@ def find_undeclared_files( "sha1": f.get("sha1"), "md5": f.get("md5"), } + if f.get("contents"): + entry["contents"] = f["contents"] if duplicate: first.setdefault("other_claims", []).append(entry) continue diff --git a/scripts/ziptools.py b/scripts/ziptools.py index a14d6f0f..9a8cabe4 100644 --- a/scripts/ziptools.py +++ b/scripts/ziptools.py @@ -75,6 +75,27 @@ def _member_crcs(path: str, mtime: float) -> frozenset[str]: return frozenset() +@functools.lru_cache(maxsize=4096) +def _member_names(path: str, mtime: float) -> frozenset[str]: + try: + with zipfile.ZipFile(path) as archive: + return frozenset( + info.filename.rsplit("/", 1)[-1].casefold() + for info in archive.infolist() if not info.is_dir() + ) + except (zipfile.BadZipFile, OSError): + return frozenset() + + +def zip_member_names(path: str) -> frozenset[str]: + """Casefolded base name of each member, read from the central directory.""" + try: + mtime = os.path.getmtime(path) + except OSError: + return frozenset() + return _member_names(path, mtime) + + def zip_member_crcs(path: str) -> frozenset[str]: """CRC-32 of each member, read from the central directory.""" try: diff --git a/tests/test_archive_choice.py b/tests/test_archive_choice.py index 3c6a6774..860f6dbd 100644 --- a/tests/test_archive_choice.py +++ b/tests/test_archive_choice.py @@ -269,5 +269,85 @@ class AnArchiveHashedByItsMembers(unittest.TestCase): self.assertEqual(marks, {(True, "composite"), (False, None)}) +class AnArchiveRankedByWhatItHolds(unittest.TestCase): + """Two primaries answered hod2bios.zip and the first one indexed won: + Flycast received a set without epr-21329.ic27, its Japan BIOS, once an + unrelated duplicate was deleted and the index order flipped.""" + + def setUp(self): + directory = tempfile.mkdtemp(dir=REPO_ROOT / "tmp") + self.addCleanup(shutil.rmtree, directory, True) + self.root = Path(directory) + self.roms = {f"epr-213{n}.ic27": bytes([n]) * 16 for n in (29, 30, 31, 32)} + + def _db(self, copies: list[tuple[str, list[str]]]) -> dict: + files, by_name = {}, {} + for relative, members in copies: + path = self.root / relative + path.parent.mkdir(parents=True, exist_ok=True) + with zipfile.ZipFile(path, "w") as zf: + for member in members: + zf.writestr(member, self.roms.get(member, member.encode())) + sha1 = hashlib.sha1(path.read_bytes()).hexdigest() + files[sha1] = {"path": str(path), "name": "hod2bios.zip", "sha1": sha1} + by_name.setdefault("hod2bios.zip", []).append(sha1) + return {"files": files, "indexes": {"by_name": by_name, "by_md5": {}, + "by_path_suffix": {}, "by_crc32": {}}} + + def _resolve(self, entry: dict, db: dict) -> str: + from common import resolve_local_file + + path, _status = resolve_local_file(entry, db) + return str(Path(path).relative_to(self.root)) + + def test_member_names_the_code_opens_choose_the_copy(self): + db = self._db([("bios/Arcade/hod2bios.zip", ["epr-21330.ic27", "epr-21331.ic27"]), + ("bios/MAME/hod2bios.zip", list(self.roms))]) + entry = {"name": "hod2bios.zip", "contents": [ + {"name": name, "size": 16} for name in ("epr-21331.ic27", "epr-21330.ic27", + "epr-21329.ic27")]} + self.assertEqual(self._resolve(entry, db), "bios/MAME/hod2bios.zip") + + def test_member_crcs_choose_the_copy(self): + db = self._db([("bios/Arcade/hod2bios.zip", ["epr-21330.ic27", "renamed.ic27"]), + ("bios/MAME/hod2bios.zip", ["epr-21330.ic27", "epr-21332.ic27"])]) + entry = {"name": "hod2bios.zip", "contents": [ + {"name": "other-name.ic27", "crc32": format(zlib.crc32(self.roms[name]), "08x")} + for name in ("epr-21330.ic27", "epr-21332.ic27")]} + self.assertEqual(self._resolve(entry, db), "bios/MAME/hod2bios.zip") + + def test_without_a_layout_a_superset_primary_wins(self): + db = self._db([("bios/Arcade/hod2bios.zip", ["epr-21330.ic27"]), + ("bios/MAME/hod2bios.zip", list(self.roms))]) + self.assertEqual(self._resolve({"name": "hod2bios.zip"}, db), + "bios/MAME/hod2bios.zip") + + def test_two_revisions_keep_the_index_order(self): + db = self._db([("bios/Arcade/hod2bios.zip", ["epr-21330.ic27", "epr-21331.ic27"]), + ("bios/MAME/hod2bios.zip", ["epr-21330.ic27", "epr-21332.ic27"])]) + self.assertEqual(self._resolve({"name": "hod2bios.zip"}, db), + "bios/Arcade/hod2bios.zip") + + def test_a_variant_does_not_stop_a_primary_from_covering(self): + db = self._db([("bios/Arcade/hod2bios.zip", ["epr-21330.ic27"]), + ("bios/MAME/hod2bios.zip", list(self.roms)), + ("bios/Arcade/.variants/hod2bios.zip", ["proto.ic27"])]) + self.assertEqual(self._resolve({"name": "hod2bios.zip"}, db), + "bios/MAME/hod2bios.zip") + + def test_every_claim_path_carries_the_layout(self): + from packextras import _report_extra + + contents = [{"name": "epr-21329.ic27", "size": 16}] + extra = _report_extra({"name": "hod2bios.zip", "contents": contents}, + "hod2bios.zip", "dc/hod2bios.zip") + self.assertEqual(extra["contents"], contents) + # The second pass builds its extra from the profile entry itself. + source = (REPO_ROOT / "scripts" / "packextras.py").read_text() + self.assertEqual(source.count('extra["contents"] = '), 2) + report = (REPO_ROOT / "scripts" / "verify.py").read_text() + self.assertIn('entry["contents"] = f["contents"]', report) + + if __name__ == "__main__": unittest.main()