From f24b34c1ef2ba8bfa9ed5c2ad7c867d1ef4debca Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Thu, 5 Nov 2026 19:25:47 +0100 Subject: [PATCH] feat: place recognised files with validate_pr The report named the folder of each file and left the copy to the contributor, one file and one .variants/ name at a time. --place copies each recognised file from outside bios/ to its folder under the name the lists declare, or to the .variants/ slot of a name already held, never over a file. A hash with two names or two folders is not placed. --- CONTRIBUTING.md | 11 ++++--- scripts/generate_readme.py | 11 ++++--- scripts/validate_pr.py | 63 ++++++++++++++++++++++++++++++++++++-- tests/test_validate_pr.py | 21 +++++++++++-- wiki/tools.md | 2 +- 5 files changed, 92 insertions(+), 16 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8c1253272..ae2c25459 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -24,11 +24,12 @@ the most useful contribution. From a clone, the file goes to `bios/Manufacturer/Console/filename`, and `python scripts/validate_pr.py path/to/file` names that folder for a file still -outside `bios/`. Given a folder, such as an old card's BIOS folder, it checks -every file under it. A file over the browser's 25 MB limit goes in that way, -up to the 100 MB GitHub accepts in a push. A larger file stays out of the pull -request: [open an issue](https://github.com/Abdess/retrobios/issues/new?template=missing-bios.yml) with the hashes -`validate_pr.py` prints, and a maintainer adds it to the `large-files` release. +outside `bios/`, and `--place` copies it there under its declared name. Given a +folder, such as an old card's BIOS folder, it checks every file under it. A +file over the browser's 25 MB limit goes in that way, up to the 100 MB GitHub +accepts in a push. A larger file stays out of the pull request: [open an +issue](https://github.com/Abdess/retrobios/issues/new?template=missing-bios.yml) with the hashes `validate_pr.py` prints, and a +maintainer adds it to the `large-files` release. ## Add a platform diff --git a/scripts/generate_readme.py b/scripts/generate_readme.py index 6e1a30e62..006aab519 100644 --- a/scripts/generate_readme.py +++ b/scripts/generate_readme.py @@ -930,11 +930,12 @@ the most useful contribution. From a clone, the file goes to `bios/Manufacturer/Console/filename`, and `python scripts/validate_pr.py path/to/file` names that folder for a file still -outside `bios/`. Given a folder, such as an old card's BIOS folder, it checks -every file under it. A file over the browser's 25 MB limit goes in that way, -up to the 100 MB GitHub accepts in a push. A larger file stays out of the pull -request: [open an issue]({LARGE_FILE_ISSUE_URL}) with the hashes -`validate_pr.py` prints, and a maintainer adds it to the `large-files` release. +outside `bios/`, and `--place` copies it there under its declared name. Given a +folder, such as an old card's BIOS folder, it checks every file under it. A +file over the browser's 25 MB limit goes in that way, up to the 100 MB GitHub +accepts in a push. A larger file stays out of the pull request: [open an +issue]({LARGE_FILE_ISSUE_URL}) with the hashes `validate_pr.py` prints, and a +maintainer adds it to the `large-files` release. ## Add a platform diff --git a/scripts/validate_pr.py b/scripts/validate_pr.py index c1a702816..f44aff8b4 100644 --- a/scripts/validate_pr.py +++ b/scripts/validate_pr.py @@ -21,6 +21,7 @@ import argparse import json import os import re +import shutil import subprocess import sys from pathlib import Path @@ -84,6 +85,9 @@ class ValidationResult: # or profile declares it): no check fails, and neither is a file # the pull request adds to the collection. self.outcome = "" + # Where --place copies the file: one folder and one declared name, or + # the .variants/ slot of a name the collection already holds. + self.destination = "" def add_check(self, passed: bool, message: str) -> None: self.checks.append(("PASS" if passed else "FAIL", message)) @@ -156,7 +160,7 @@ def load_platform_hashes(platforms_dir: str, db: dict | None = None) -> dict: under, read from the other files of that system it holds. """ known = {"sha1": set(), "md5": set(), "sha256": set(), "crc32": set(), - "names": set(), "folders": {}, "sizes": {}, "entries": {}} + "names": set(), "folders": {}, "sizes": {}, "entries": {}, "named": {}} systems_of: dict[str, set[str]] = {} folders_of: dict[str, dict[str, int]] = {} files = (db or {}).get("files", {}) @@ -203,6 +207,9 @@ def load_platform_hashes(platforms_dir: str, db: dict | None = None) -> dict: ) for value in (*sha1s, *md5s): systems_of.setdefault(value, set()).add(system_id) + for value in (*sha1s, *md5s, *sha256s): + if "name" in file_entry: + known["named"].setdefault(value, set()).add(file_entry["name"]) _note_folder(folders_of, system_id, files, by_md5, sha1s, md5s) known["folders"] = _system_homes(systems_of, folders_of) @@ -260,6 +267,7 @@ def load_profile_hashes( """ known: dict[str, dict] = { "sha1": {}, "sha256": {}, "md5": {}, "crc32": {}, "sizes": {}, "folders": {}, + "named": {}, } systems_of: dict[str, set[str]] = {} folders_of: dict[str, dict[str, int]] = {} @@ -283,6 +291,8 @@ def load_profile_hashes( for algo, values in hashes.items(): for value in values: known[algo].setdefault(value, set()).add(emulator) + if isinstance(entry.get("name"), str): + known["named"].setdefault(value, set()).add(entry["name"]) # The entry's own system, or the profile's when it emulates one. system = entry.get("system") or ( declared_systems[0] if len(declared_systems) == 1 else None @@ -474,9 +484,23 @@ def validate_file( pass elif expected_folders: # The contributor asks before forking: say where it goes. + names = { + name + for table in (platform_hashes, profile_hashes) + for value in (result.sha1, result.md5, hashes.get("sha256", "")) + for name in table.get("named", {}).get(value, ()) + } + name = next(iter(names)) if len(names) == 1 else "" + if len(expected_folders) == 1 and name and "/" not in name: + folder = expected_folders[0] + result.destination = ( + f"{folder}.variants/{name}.{result.sha1[:8]}" + if os.path.exists(folder + name) else folder + name + ) result.add_warning( "File is not under bios/ yet. Its system's files are in " + " or ".join(f"`{f}`" for f in expected_folders) + + (f". --place copies it to `{result.destination}`" if result.destination else "") ) elif _variant_home(db, result.filename, result.sha1): result.add_warning( @@ -505,9 +529,11 @@ def validate_file( if (name_known and not (sha1_known or md5_known or other_known or accepted_by) and not by_profile and size_fits): home = _variant_home(db, result.filename) + slot = f"{home}.variants/{result.filename}.{result.sha1[:8]}" + if home and not normalized.startswith("bios" + os.sep): + result.destination = slot result.add_info( - "This may be a valid variant. If accepted, it will be placed in " - f"`{home}.variants/{result.filename}.{result.sha1[:8]}`" + f"This may be a valid variant. If accepted, it will be placed in `{slot}`" ) return result @@ -700,6 +726,28 @@ def _files_to_validate( return files +def place(results: list[ValidationResult]) -> list[str]: + """Copy each placeable file to its destination, never over a file. + + The report named the folder of each file and left 40 copies from a card + to be made by hand, each .variants/ name retyped. The source is copied, + not moved: it is often a card or another tree the contributor keeps. + """ + placed = [] + for r in results: + if not (r.passed and r.destination and not r.outcome): + continue + target = Path(r.destination) + if target.exists(): + print(f"not placed: {target} exists", file=sys.stderr) + continue + target.parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(r.filepath, target) + placed.append(r.destination) + print(f"placed {r.filepath} at {r.destination}", file=sys.stderr) + return placed + + def _load_database_if_any(path: str) -> dict | None: """The database, optional only where the default is meant. @@ -734,6 +782,11 @@ def main() -> None: "--markdown", action="store_true", help="Output as markdown (for PR comments)" ) output.add_argument("--json", action="store_true", help="Output as JSON") + parser.add_argument( + "--place", action="store_true", + help="copy each recognised file from outside bios/ to the folder and name " + "the report gives, or to .variants/ beside the name it varies", + ) args = parser.parse_args() # The database, platforms and profiles are read from the repository @@ -771,6 +824,7 @@ def main() -> None: "file": r.filepath, "passed": r.passed, "outcome": r.outcome or "new", + "destination": r.destination, "sha1": r.sha1, "md5": r.md5, "size": r.size, @@ -816,6 +870,9 @@ def main() -> None: ) print(f" [{marker}] {m}") + if args.place: + place(results) + if not all_passed: sys.exit(1) diff --git a/tests/test_validate_pr.py b/tests/test_validate_pr.py index 04b7a78fa..642420795 100644 --- a/tests/test_validate_pr.py +++ b/tests/test_validate_pr.py @@ -139,9 +139,25 @@ class AFileOnlyACoreReadsIsKnown(_Fixture): ) self.assertIn( ("WARN", "File is not under bios/ yet. Its system's files are in " - "`bios/Maker/Console/`"), + "`bios/Maker/Console/`. --place copies it to `bios/Maker/Console/b.bin`"), result.checks, ) + self.assertEqual(result.destination, "bios/Maker/Console/b.bin") + # A file of that name already there: the copy is a variant. + self._bios_file("bios/Maker/Console/b.bin", b"another") + self.assertEqual( + validate_pr.validate_file("Downloads/b.bin", db, EMPTY_HASHES, known).destination, + f"bios/Maker/Console/.variants/b.bin.{result.sha1[:8]}", + ) + # Copied under the declared name, never over a file, and only once. + card = self._bios_file("card/B.BIN", missing) + named = validate_pr.validate_file(card, db, EMPTY_HASHES, known) + self.assertEqual(validate_pr.place([named]), [named.destination]) + self.assertEqual((self.tmp / named.destination).read_bytes(), missing) + self.assertTrue((self.tmp / card).is_file()) + self.assertEqual(validate_pr.place([named]), []) + unknown = validate_pr.validate_file(self._bios_file("card/x.bin"), db, EMPTY_HASHES, known) + self.assertEqual((unknown.outcome, unknown.destination), ("unknown", "")) placed = validate_pr.validate_file( self._bios_file("bios/Other/Place/b.bin", missing), db, EMPTY_HASHES, known, ) @@ -388,7 +404,8 @@ class PlatformHashLoading(_Fixture): def test_missing_directory_yields_empty_sets(self): known = validate_pr.load_platform_hashes(str(self.tmp / "absent")) self.assertEqual(known, {"sha1": set(), "md5": set(), "sha256": set(), "crc32": set(), - "names": set(), "folders": {}, "sizes": {}, "entries": {}}) + "names": set(), "folders": {}, "sizes": {}, "entries": {}, + "named": {}}) def test_declared_hashes_and_names_are_collected(self): platforms = self.tmp / "platforms" diff --git a/wiki/tools.md b/wiki/tools.md index d691b8917..16173cd99 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -1364,7 +1364,7 @@ Further options, by script: | `romset_recipes.py` | `--dat-recipes PATH`, a snapshot or a directory of them (default `recipes/`) | | `split_pack.py` | `--max-size 1900M`, the ceiling per part (K, M, G, default 2G) | | `torrentzip.py` | `--check ZIP` reports conformance, `--rebuild ZIP -o OUT` normalizes an archive | -| `validate_pr.py` | `--changed` takes the BIOS files the branch changes, a folder named checks every file under it, `--markdown`, `--json` | +| `validate_pr.py` | `--changed` takes the BIOS files the branch changes, a folder named checks every file under it, `--place` copies each recognised file from outside `bios/` to its folder and declared name (or its `.variants/` slot), `--markdown`, `--json` | | `validate_site.py` | `--site-dir` (rendered site, default `website/dist`), `--base` (path the site is served under), `--max-errors N` | | `scraper/coreinfo_scraper.py` | `--compare-db database.json` lists the `.info` firmware the database lacks | | `scraper/mame_hash_scraper.py` | `--release mameNNNN --profile FILE --source-commit SHA --core NAME --built-by PLATFORM` writes the profile of a MAME build a platform compiles. `--devices --listxml FILE` documents in `mame.yml` every device set with dumped ROMs that the release's `-listxml` names (`isdevice="yes"`), with `--dry-run` to preview |