fix: name homonymous release assets by path

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(

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", ""),
         )

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

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

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