diff --git a/scripts/scraper/_hash_merge.py b/scripts/scraper/_hash_merge.py index 067a30e0..6952f21b 100644 --- a/scripts/scraper/_hash_merge.py +++ b/scripts/scraper/_hash_merge.py @@ -18,6 +18,7 @@ from common import yaml_load _MAME_RELEASE_RE = re.compile(r"^0\.\d+") +_MAMEDEV = "https://github.com/mamedev/mame" def merge_mame_profile( @@ -45,6 +46,11 @@ def merge_mame_profile( # never built from. if _MAME_RELEASE_RE.match(str(profile.get("core_version") or "")): profile["core_version"] = hashes.get("version", profile.get("core_version")) + # The refs about to be written are line numbers of that release's + # mamedev/mame tree: the pin moves with them, or the profile cites + # lines its declared revision does not hold. + if profile.get("upstream") == _MAMEDEV and hashes.get("commit"): + profile["upstream_commit"] = hashes["commit"] files = profile.get("files", []) bios_zip, non_bios = _split_files(files, lambda f: f.get("category") == "bios_zip") @@ -380,6 +386,7 @@ def _backup_and_write(path: str, data: dict) -> None: original = p.read_text(encoding="utf-8") patched = _patch_core_version(original, data.get("core_version", "")) + patched = _patch_scalar(patched, "upstream_commit", data.get("upstream_commit", "")) patched = _patch_bios_entries(patched, data.get("files", [])) patched = _append_new_entries(patched, data.get("files", []), original) @@ -388,13 +395,16 @@ def _backup_and_write(path: str, data: dict) -> None: def _patch_core_version(text: str, version: str) -> str: """Replace core_version value in-place.""" - if not version: - return text - import re + return _patch_scalar(text, "core_version", version) + +def _patch_scalar(text: str, key: str, value: str) -> str: + """Replace a top-level scalar in-place, quoted; absent value, no change.""" + if not value: + return text return re.sub( - r"^(core_version:\s*).*$", - rf'\g<1>"{version}"', + rf"^({re.escape(key)}:\s*).*$", + rf'\g<1>"{value}"', text, count=1, flags=re.MULTILINE, diff --git a/scripts/scraper/mame_hash_scraper.py b/scripts/scraper/mame_hash_scraper.py index ad8a7849..4f4534e2 100644 --- a/scripts/scraper/mame_hash_scraper.py +++ b/scripts/scraper/mame_hash_scraper.py @@ -86,17 +86,25 @@ def _run_git( ) -def _sparse_clone() -> None: +def _sparse_clone(tag: str) -> None: + """Clone the release the data will be labelled with, not master. + + The version comes from the latest release; cloning master gave line + numbers and hashes of a later revision under that release's label, and + the profiles cited lines no declared revision holds. + """ if _CLONE_DIR.exists(): shutil.rmtree(_CLONE_DIR) _CLONE_DIR.parent.mkdir(parents=True, exist_ok=True) - log.info("sparse cloning mamedev/mame into %s", _CLONE_DIR) + log.info("sparse cloning mamedev/mame %s into %s", tag, _CLONE_DIR) _run_git( [ "clone", "--depth", "1", + "--branch", + tag, "--filter=blob:none", "--sparse", _REPO_URL, @@ -110,7 +118,12 @@ def _sparse_clone() -> None: def _get_version() -> str: - """The latest MAME release, from the GitHub API. + """The latest MAME release version, such as 0.289.""" + return _parse_version_tag(_get_release_tag()) + + +def _get_release_tag() -> str: + """The latest MAME release tag, from the GitHub API. version.cpp is generated at build time, not in the repo. A failed lookup raises: "unknown" was cached for a day and written as core_version. @@ -131,7 +144,7 @@ def _get_version() -> str: raise RuntimeError(f"cannot read the MAME release from {url}: {exc}") from exc if not tag: raise RuntimeError(f"no tag_name in {url}") - return _parse_version_tag(tag) + return tag def _parse_version_tag(tag: str) -> str: @@ -251,9 +264,10 @@ def _fetch_hashes(force: bool) -> dict[str, Any]: return cache # type: ignore[return-value] try: - _sparse_clone() + tag = _get_release_tag() + _sparse_clone(tag) bios_sets = parse_mame_source_tree(str(_CLONE_DIR)) - version = _get_version() + version = _parse_version_tag(tag) commit = _get_commit() data: dict[str, Any] = { diff --git a/tests/test_hash_merge.py b/tests/test_hash_merge.py index e4698140..c2565285 100644 --- a/tests/test_hash_merge.py +++ b/tests/test_hash_merge.py @@ -311,6 +311,64 @@ class TestMameMerge(unittest.TestCase): self.assertEqual(entry["source_ref"], "src/mame/neogeo/neogeo.cpp:2432") +class TheMamePinMovesWithItsRefs(unittest.TestCase): + """The refs the merge writes are line numbers of the scraped release: + leaving upstream_commit on an older revision made the profile cite lines + its declared revision does not hold (pgm.cpp:5548 is a comment there).""" + + def _merged_text(self, **profile_overrides) -> str: + with tempfile.TemporaryDirectory() as td: + p = Path(td) + profile_path = _write_yaml( + p / "mame.yml", _make_mame_profile(**profile_overrides) + ) + hashes_path = _write_json( + p / "hashes.json", _make_mame_hashes(commit="f" * 40) + ) + merge_mame_profile(profile_path, hashes_path, write=True) + return Path(profile_path).read_text(encoding="utf-8") + + def test_the_upstream_pin_follows_the_release(self): + text = self._merged_text( + upstream="https://github.com/mamedev/mame", upstream_commit="a" * 40 + ) + written = yaml.safe_load(text) + self.assertEqual(written["upstream_commit"], "f" * 40) + self.assertEqual(written["core_version"], "0.286") + + def test_a_fork_keeps_its_own_pin(self): + text = self._merged_text( + upstream="https://github.com/example/fork", upstream_commit="a" * 40 + ) + self.assertEqual(yaml.safe_load(text)["upstream_commit"], "a" * 40) + + +class TheScraperClonesTheReleaseItLabels(unittest.TestCase): + """Cloning master and labelling it with the latest release tag gave + master's line numbers and hashes under the release's name.""" + + def test_clone_and_version_come_from_one_tag(self): + from unittest import mock + + from scripts.scraper import mame_hash_scraper as scraper + + calls: list[list[str]] = [] + with mock.patch.object(scraper, "_get_release_tag", return_value="mame0289"), \ + mock.patch.object(scraper, "_run_git", + side_effect=lambda args, cwd=None: calls.append(args)), \ + mock.patch.object(scraper, "parse_mame_source_tree", return_value={}), \ + mock.patch.object(scraper, "_get_commit", return_value="c" * 40), \ + mock.patch.object(scraper, "_write_cache"), \ + mock.patch.object(scraper, "_cleanup"), \ + mock.patch.object(scraper, "_load_cache", return_value={}), \ + mock.patch.object(scraper, "_is_stale", return_value=True): + data = scraper._fetch_hashes(force=True) + clone = next(args for args in calls if args[0] == "clone") + self.assertEqual(clone[clone.index("--branch") + 1], "mame0289") + self.assertEqual(data["version"], "0.289") + self.assertEqual(data["commit"], "c" * 40) + + class TestFbneoMerge(unittest.TestCase): """Tests for merge_fbneo_profile."""