From 0d3a2b87edc731877dff8bf41067f8121eeb4dfc Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Mon, 5 Oct 2026 02:51:13 +0200 Subject: [PATCH] fix: name homonymous release assets by path --- scripts/check_release_assets.py | 54 +++++++----- scripts/generate_db.py | 2 +- scripts/largefiles.py | 109 +++++++++++++++++++++++- tests/test_large_file_cache.py | 144 ++++++++++++++++++++++++++++++++ wiki/release-process.md | 15 +++- 5 files changed, 297 insertions(+), 27 deletions(-) diff --git a/scripts/check_release_assets.py b/scripts/check_release_assets.py index 5dc6e263..2afa52c5 100644 --- a/scripts/check_release_assets.py +++ b/scripts/check_release_assets.py @@ -21,21 +21,23 @@ import os import re import subprocess import sys +from collections.abc import Iterable sys.path.insert(0, os.path.dirname(__file__)) from common import load_database -from largefiles import LARGE_FILES_RELEASE, LARGE_FILES_REPO +from largefiles import ( + LARGE_FILES_RELEASE, + LARGE_FILES_REPO, + asset_names, + registered_paths, +) Finding = tuple[str, str, int, int | None] def expected_assets(db: dict, gitignore_text: str) -> dict[str, int]: """Map each gitignored database path to the size the manifests carry.""" - ignored = { - line.strip() - for line in gitignore_text.splitlines() - if line.strip().startswith("bios/") - } + ignored = set(registered_paths(gitignore_text)) return { entry["path"]: entry["size"] for entry in db.get("files", {}).values() @@ -43,31 +45,37 @@ def expected_assets(db: dict, gitignore_text: str) -> dict[str, int]: } -def _asset_names(path: str) -> list[str]: - """Names the release may publish a file under. +def _spellings(name: str) -> list[str]: + """Names the release may publish an asset under. GitHub rewrites spaces to dots in asset names, so a file whose name carries spaces is published under the dotted form. """ - name = os.path.basename(path) return [name, name.replace(" ", ".")] if " " in name else [name] def compare( - expected: dict[str, int], assets: dict[str, int], notes_current: bool = True + expected: dict[str, int], + assets: dict[str, int], + notes_current: bool = True, + registered: Iterable[str] | None = None, ) -> list[Finding]: """Files the release lacks or serves at another size, sorted by kind. - A release description that no longer matches what render_notes() would - write is a finding of its own: the page is what a reader checks a - download against. + *registered* is every path .gitignore lists; an asset name depends on + which other paths share its basename, so it defaults to *expected* only + when the caller has nothing wider. A release description that no longer + matches what render_notes() would write is a finding of its own: the + page is what a reader checks a download against. """ findings: list[Finding] = [] if not notes_current: findings.append(("notes", "release description", 0, None)) + names = asset_names([*(registered or ()), *expected]) for path, size in sorted(expected.items()): published = next( - (assets[name] for name in _asset_names(path) if name in assets), None + (assets[name] for name in _spellings(names[path]) if name in assets), + None, ) if published is None: findings.append(("missing", path, size, None)) @@ -150,17 +158,12 @@ def render_notes( sections already written by hand are kept by name. """ known = parse_notes(previous) - ignored = { - line.strip() - for line in gitignore_text.splitlines() - if line.strip().startswith("bios/") - } + names = asset_names(registered_paths(gitignore_text)) indexed: dict[str, tuple[str, str]] = {} for sha1, entry in db.get("files", {}).items(): path = entry.get("path", "") - if path in ignored: - name = os.path.basename(path) - for candidate in (name, name.replace(" ", ".")): + if path in names: + for candidate in _spellings(names[path]): indexed[candidate] = (sha1, path) rows: dict[str, list[tuple[int, str]]] = {section: [] for section in SECTIONS} @@ -307,7 +310,12 @@ def main() -> int: if args.notes: with open(args.notes, "w", encoding="utf-8") as handle: handle.write(rendered) - findings = compare(expected, assets, notes_current=rendered.strip() == body.strip()) + findings = compare( + expected, + assets, + notes_current=rendered.strip() == body.strip(), + registered=registered_paths(gitignore_text), + ) if args.json: print(json.dumps( diff --git a/scripts/generate_db.py b/scripts/generate_db.py index 50ff45d7..5f5a451e 100644 --- a/scripts/generate_db.py +++ b/scripts/generate_db.py @@ -307,7 +307,7 @@ def _preserve_large_file_entries(files: dict, db_path: str) -> int: if path not in large_files.values() and name not in large_files: continue cached = fetch_large_file( - name, + path if path in large_files.values() else name, expected_sha1=entry.get("sha1", ""), expected_md5=entry.get("md5", ""), ) diff --git a/scripts/largefiles.py b/scripts/largefiles.py index 7178a55e..efba6a73 100644 --- a/scripts/largefiles.py +++ b/scripts/largefiles.py @@ -7,9 +7,14 @@ from __future__ import annotations import contextlib import os +import re import tempfile import urllib.error +import urllib.parse import urllib.request +from collections import Counter +from collections.abc import Iterable +from pathlib import Path from hashing import compute_hashes @@ -17,6 +22,83 @@ from hashing import compute_hashes LARGE_FILES_RELEASE = "large-files" LARGE_FILES_REPO = "Abdess/retrobios" LARGE_FILES_CACHE = ".cache/large" +GITIGNORE = Path(__file__).resolve().parent.parent / ".gitignore" + +_UNSAFE_ASSET_CHARS = re.compile(r"[^A-Za-z0-9._-]+") + + +def registered_paths(gitignore_text: str) -> list[str]: + """The bios/ paths .gitignore lists, which are the release assets.""" + return [ + line.strip() + for line in gitignore_text.splitlines() + if line.strip().startswith("bios/") + ] + + +def load_registered_paths(gitignore: str | Path = GITIGNORE) -> list[str]: + try: + return registered_paths(Path(gitignore).read_text(encoding="utf-8")) + except FileNotFoundError: + return [] + + +def asset_names(registered: Iterable[str]) -> dict[str, str]: + """Map each registered path to the name of its release asset. + + A path whose basename no other registered path shares is published + under that basename. A path whose basename is shared is published under + its location below bios/, segments joined by "--" and every character + outside [A-Za-z0-9._-] replaced by "_": + bios/Id Software/Wolfenstein Enemy Territory/etmain/pak0.pk3 becomes + Id_Software--Wolfenstein_Enemy_Territory--etmain--pak0.pk3. The name + depends on the path only, never on the bytes, so rebuilding a file + keeps its asset. + """ + paths = sorted(set(registered)) + counts = Counter(os.path.basename(p) for p in paths) + names: dict[str, str] = {} + for registered_path in paths: + base = os.path.basename(registered_path) + if counts[base] == 1: + names[registered_path] = base + continue + rel = registered_path.removeprefix("bios/") + names[registered_path] = "--".join( + _UNSAFE_ASSET_CHARS.sub("_", part) for part in rel.split("/") + ) + owners: dict[str, str] = {} + for registered_path, name in names.items(): + # GitHub publishes a space as a dot, so both spellings are taken. + for spelling in {name, name.replace(" ", ".")}: + other = owners.setdefault(spelling, registered_path) + if other != registered_path: + raise ValueError( + f"release asset {spelling!r} named by both {other!r} " + f"and {registered_path!r}" + ) + return names + + +def asset_name(path: str, registered: Iterable[str]) -> str: + """The release asset name of *path* among the *registered* paths.""" + return asset_names([*registered, path])[path] + + +def asset_candidates(name: str, registered: Iterable[str]) -> list[str]: + """Assets that may hold *name*, a registered path or a bare file name. + + A bare name shared by several registered paths has one asset per path; + the caller's hash tells them apart. + """ + registered = list(registered) + if "/" in name: + return [asset_name(name, registered)] + names = asset_names(registered) + matches = [ + names[p] for p in sorted(names) if os.path.basename(p) == name + ] + return matches or [name] def fetch_large_file( @@ -26,8 +108,33 @@ def fetch_large_file( expected_md5: str = "", *, offline: bool = False, + registered: Iterable[str] | None = None, +) -> str | None: + """Return a verified cached large file, downloading it only when allowed. + + *name* is a registered bios/ path or a bare file name; asset_names() + turns it into the asset to fetch, and a bare name shared by several + registered paths tries each of their assets until one verifies. + """ + if registered is None: + registered = load_registered_paths() + for asset in asset_candidates(name, registered): + cached = _fetch_asset( + asset, dest_dir, expected_sha1, expected_md5, offline=offline + ) + if cached: + return cached + return None + + +def _fetch_asset( + name: str, + dest_dir: str, + expected_sha1: str, + expected_md5: str, + *, + offline: bool, ) -> str | None: - """Return a verified cached large file, downloading it only when allowed.""" cached = os.path.join(dest_dir, name) # Between the existence test and the hash, a concurrent run can drop the # same stale entry: the file is gone by the time this one reads it, and diff --git a/tests/test_large_file_cache.py b/tests/test_large_file_cache.py index e0f3b84e..9b247197 100644 --- a/tests/test_large_file_cache.py +++ b/tests/test_large_file_cache.py @@ -291,5 +291,149 @@ class PreservedLargeFileEntries(unittest.TestCase): self.assertEqual(files["b" * 40]["path"], "/cache/large/FW.PUP") +class ReleaseAssetNames(unittest.TestCase): + """One registered path, one asset name, the same for every consumer. + + Every consumer named an asset by the file's basename, so two collection + files sharing one (`etmain/pak0.pk3` and `demomain/pak0.pk3`) could not + both be published: the second upload replaced the first and every + fetch received whichever was there. + """ + + ET = "bios/Id Software/Wolfenstein Enemy Territory/etmain/pak0.pk3" + RTCW = "bios/Id Software/Return to Castle Wolfenstein/demomain/pak0.pk3" + PUP = "bios/Sony/PS3/PS3UPDAT.PUP" + REGISTERED = [ET, RTCW, PUP] + + def test_a_shared_basename_gets_two_distinct_names(self): + names = largefiles.asset_names(self.REGISTERED) + self.assertNotEqual(names[self.ET], names[self.RTCW]) + self.assertNotIn("pak0.pk3", (names[self.ET], names[self.RTCW])) + for name in (names[self.ET], names[self.RTCW]): + self.assertNotIn("/", name) + self.assertNotIn(" ", name) + self.assertTrue(name.endswith("pak0.pk3")) + + def test_the_name_depends_on_the_path_not_on_the_set_order(self): + forward = largefiles.asset_names(self.REGISTERED) + backward = largefiles.asset_names(list(reversed(self.REGISTERED))) + self.assertEqual(forward, backward) + + def test_a_basename_used_once_keeps_its_name(self): + self.assertEqual( + largefiles.asset_name(self.PUP, self.REGISTERED), "PS3UPDAT.PUP" + ) + self.assertEqual( + largefiles.asset_names(["bios/Arcade/MAME/MAME 0.174 Arcade XML.dat"]), + {"bios/Arcade/MAME/MAME 0.174 Arcade XML.dat": "MAME 0.174 Arcade XML.dat"}, + ) + + def test_the_published_register_has_no_two_paths_on_one_name(self): + registered = largefiles.registered_paths( + (REPO_ROOT / ".gitignore").read_text(encoding="utf-8") + ) + names = largefiles.asset_names(registered) + self.assertEqual(len(set(names.values())), len(registered)) + basenames = [os.path.basename(p) for p in registered] + for registered_path, name in names.items(): + if basenames.count(os.path.basename(registered_path)) == 1: + self.assertEqual(name, os.path.basename(registered_path)) + + def test_manifest_checker_and_fetcher_agree_on_a_path(self): + import check_release_assets + import generate_pack + + expected = largefiles.asset_names(self.REGISTERED) + gitignore = "\n".join(["tmp/", *self.REGISTERED]) + "\n" + + with tempfile.TemporaryDirectory() as root: + Path(root, ".gitignore").write_text(gitignore, encoding="utf-8") + generate_pack._GITIGNORE_ENTRIES = None + try: + manifest = { + p: generate_pack._release_asset_name(os.path.join(root, p), root) + for p in self.REGISTERED + } + finally: + generate_pack._GITIGNORE_ENTRIES = None + self.assertEqual(manifest, expected) + + sizes = {self.ET: 228138631, self.RTCW: 122757192, self.PUP: 10} + published = {expected[p]: size for p, size in sizes.items()} + self.assertEqual( + check_release_assets.compare( + sizes, published, registered=self.REGISTERED + ), + [], + ) + # The old basename-only asset must not satisfy either pak. + self.assertEqual( + {f[1] for f in check_release_assets.compare( + sizes, {"pak0.pk3": 228138631, "PS3UPDAT.PUP": 10}, + registered=self.REGISTERED, + )}, + {self.ET, self.RTCW}, + ) + db = {"files": { + "a" * 40: {"path": self.ET, "size": sizes[self.ET]}, + "b" * 40: {"path": self.RTCW, "size": sizes[self.RTCW]}, + "c" * 40: {"path": self.PUP, "size": sizes[self.PUP]}, + }} + body = check_release_assets.render_notes( + db, gitignore, published, "", {}, {}, {} + ) + self.assertIn(f"[{expected[self.ET]}]", body) + self.assertIn("a" * 40, body) + self.assertIn("b" * 40, body) + self.assertNotIn("## Not indexed", body) + + requested: list[str] = [] + + def fake_urlopen(req, timeout=None): + requested.append(req.full_url.rsplit("/", 1)[1]) + raise urllib.error.URLError("offline") + + self._urlopen = largefiles.urllib.request.urlopen + largefiles.urllib.request.urlopen = fake_urlopen + try: + with tempfile.TemporaryDirectory() as cache: + largefiles.fetch_large_file( + self.RTCW, dest_dir=cache, registered=self.REGISTERED + ) + self.assertEqual( + requested, + [largefiles.urllib.parse.quote(expected[self.RTCW])], + ) + requested.clear() + largefiles.fetch_large_file( + "pak0.pk3", dest_dir=cache, registered=self.REGISTERED + ) + self.assertEqual( + sorted(requested), + sorted( + largefiles.urllib.parse.quote(expected[p]) + for p in (self.ET, self.RTCW) + ), + ) + finally: + largefiles.urllib.request.urlopen = self._urlopen + + def test_a_shared_basename_is_fetched_by_the_asset_its_hash_names(self): + names = largefiles.asset_names(self.REGISTERED) + with tempfile.TemporaryDirectory() as cache: + Path(cache, names[self.ET]).write_bytes(PAYLOAD_A) + Path(cache, names[self.RTCW]).write_bytes(PAYLOAD_B) + got = largefiles.fetch_large_file( + "pak0.pk3", + dest_dir=cache, + expected_sha1=hashlib.sha1(PAYLOAD_B).hexdigest(), + offline=True, + registered=self.REGISTERED, + ) + self.assertEqual(got, os.path.join(cache, names[self.RTCW])) + # The cache entry of the other asset is a valid file, not stale. + self.assertTrue(Path(cache, names[self.ET]).exists()) + + if __name__ == "__main__": unittest.main() diff --git a/wiki/release-process.md b/wiki/release-process.md index c99f0f5d..f6d462c3 100644 --- a/wiki/release-process.md +++ b/wiki/release-process.md @@ -123,12 +123,24 @@ deletes version-tagged releases). `large-files` into `.cache/large/` and copies them to their expected paths before pack generation. +**Asset name.** A gitignored path whose file name no other gitignored path +shares is published under that file name. When two paths share one, each is +published under its location below `bios/`, segments joined by `--` and any +character outside `A-Za-z0-9._-` replaced by `_` +(`Id_Software--Wolfenstein_Enemy_Territory--etmain--pak0.pk3`). +`asset_names()` in `scripts/largefiles.py` is the only place that rule lives; +the manifests, the fetcher and `check_release_assets.py` all read it. + **Upload.** To add or update a large file: ```bash gh release upload large-files "bios/Sony/PS3/PS3UPDAT.PUP#PS3UPDAT.PUP" ``` +The text after `#` is only a display label: the asset takes the uploaded +file's own name. A path whose asset name differs from its file name is +uploaded from a copy carrying the asset name. + **Local cache.** `generate_pack.py` calls `fetch_large_file()` which downloads from the release and caches in `.cache/large/` for subsequent runs. @@ -136,8 +148,7 @@ from the release and caches in `.cache/large/` for subsequent runs. from the manifest size, and the manifest size is that of the local file. A file rebuilt locally after its upload therefore fails every install until it is uploaded again (`--clobber`). `python scripts/check_release_assets.py` -compares every gitignored `bios/` path in the database with the asset of the -same name, and the release page with the one it renders from the collection; +compares every gitignored `bios/` path in the database with its asset, and the release page with the one it renders from the collection; the online pipeline runs it as step 2b2. To refresh the page: ```bash