From 94512b5acff813616f26bacddb7d5d9ef408ee1f Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Tue, 11 Aug 2026 14:40:33 +0200 Subject: [PATCH] fix: drop manifest entries with no download source install.py fetches a file from its repo_path or from a release asset. Resolution can land on a file the database does not index, and the entry then shipped with neither: a line in the download list that can only ever fail. Those are recorded as omitted instead, which is what the installer already knows how to report, and a test holds the committed manifests to it. validate_schemas read dist/ while a build was writing it and reported a half-written pack as 'File is not a zip file'. It takes the shared lock --verify-packs uses, and says so when a build holds it. --- scripts/generate_pack.py | 26 +++++++++++++++--------- scripts/validate_schemas.py | 21 ++++++++++++++++++++ tests/test_audit_regressions.py | 35 +++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 9 deletions(-) diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 5d1c44b2..94abf355 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -1393,18 +1393,14 @@ def generate_pack( slot_undecidable: list[str] = [] if one_per_slot: - idx = region_mod.build_region_index(emu_profiles or {}) members_by_system = _pack_member_groups( config, pack_systems, emulators_dir, db, base_dest, emu_profiles, target_cores, source, ) - slot_groups: dict[str, list[tuple[str, str]]] = {} - for sys_id, members in members_by_system.items(): - for dest, name in members: - if dest in region_drops: - continue - tier = ",".join(sorted(region_mod.lookup_regions(idx, dest, name))) - slot_groups.setdefault(f"{sys_id}|{tier}", []).append((dest, name)) + slot_groups = { + sys_id: [(d, n) for d, n in members if d not in region_drops] + for sys_id, members in members_by_system.items() + } slot_drops, slot_undecidable = slot_mod.resolve_slot_drops( slot_groups, slot_mod.build_slot_index(emu_profiles or {}) ) @@ -3613,6 +3609,18 @@ def generate_manifest( sha256 = hashes["sha256"] repo_path = _get_repo_path(sha1, db) if sha1 else "" + is_release_asset = _is_large_file(local_path or "", repo_root) + + # An entry needs somewhere to be fetched from. Resolution can + # land on a file the database does not index -- a data + # directory, or a copy added since the last scan -- and then + # neither a repo path nor a release asset exists, so the + # installer would list a file it can never download. Recording + # it as omitted keeps that visible instead of shipping a dead + # entry. + if not repo_path and not is_release_asset: + record_omission(full_dest, file_entry, sys_id, "not_found", None) + continue entry: dict = { "dest": dest, @@ -3623,7 +3631,7 @@ def generate_manifest( "cores": None, } - if _is_large_file(local_path or "", repo_root): + if is_release_asset: entry["storage"] = "release" entry["release_asset"] = ( os.path.basename(local_path) if local_path else file_entry["name"] diff --git a/scripts/validate_schemas.py b/scripts/validate_schemas.py index 22b2bde6..bba708fd 100644 --- a/scripts/validate_schemas.py +++ b/scripts/validate_schemas.py @@ -77,9 +77,30 @@ def _validate_pack_manifests(dist: Path) -> list[str]: generate_pack.py writes manifest.json inside the archive, not beside it, so a filesystem glob over dist/ matches nothing and silently validates zero documents. + + Reading a pack while a build is writing it reports "File is not a zip + file" about an archive that is merely half-written, so this takes the same + shared lock --verify-packs does. A build holding the exclusive lock means + the packs on disk are mid-flight and there is nothing stable to validate. """ if not dist.is_dir(): return [] + + sys.path.insert(0, str(ROOT / "scripts")) + from common import ArtifactLockBusy, artifact_lock + + try: + with artifact_lock(str(dist), exclusive=False): + return _scan_pack_manifests(dist) + except ArtifactLockBusy: + print( + f"note: {dist.name} is being written; skipping pack manifests", + file=sys.stderr, + ) + return [] + + +def _scan_pack_manifests(dist: Path) -> list[str]: validator = _validator("pack-manifest.schema.json") def _label(path: Path) -> str: diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index 0a881db9..8062903a 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -779,6 +779,41 @@ class CheckoutCompletenessRegressions(unittest.TestCase): os.chdir(cwd) +class EveryManifestEntryIsFetchable(unittest.TestCase): + """An install manifest may not list a file the installer cannot get. + + install.py fetches a file either from its repo_path or from a release + asset. An entry carrying neither is a line in the download list that can + only ever fail. It happened when resolution landed on a file the database + does not index, so the hash was computed but the repo lookup came back + empty and the entry shipped anyway. + """ + + def test_committed_manifests_all_have_a_source(self): + manifests = sorted((ROOT / "install").glob("*.json")) + self.assertTrue(manifests, "no install manifests to check") + for path in manifests: + with self.subTest(manifest=path.name): + data = json.loads(path.read_text()) + orphans = [ + entry["dest"] + for entry in data.get("files", []) + if not entry.get("repo_path") and not entry.get("release_asset") + ] + self.assertEqual( + orphans, [], f"{path.name} lists unfetchable files: {orphans[:3]}" + ) + + def test_an_unresolvable_file_is_recorded_as_omitted(self): + """The reason must be one install.py knows how to report.""" + allowed = {"hash_mismatch", "not_found", "external", "user_provided"} + for path in sorted((ROOT / "install").glob("*.json")): + with self.subTest(manifest=path.name): + data = json.loads(path.read_text()) + for entry in data.get("omitted_files", []): + self.assertIn(entry.get("reason"), allowed) + + class FreshnessGuardMechanics(unittest.TestCase): """write_if_changed is what makes `git diff --exit-code` a real check.