diff --git a/scripts/fileless_audit.py b/scripts/fileless_audit.py index f91e58cd..5baf2720 100644 --- a/scripts/fileless_audit.py +++ b/scripts/fileless_audit.py @@ -26,7 +26,7 @@ sys.path.insert(0, os.path.dirname(__file__)) import upstream from common import suggest_names -from profile_sync import collect_citations, select_views, split_source_ref +from profile_sync import collect_citations, ref_role, select_views, split_source_ref from safeparse import yaml_load # Ways a program asks for somewhere to read from. @@ -45,7 +45,7 @@ def cited_paths(document: dict) -> list[str]: seen: list[str] = [] for citation in collect_citations(document): for part in split_source_ref(citation.ref): - path = part.path + path = ref_role(part.path)[1] if path.endswith(SOURCE_SUFFIX) and path not in seen: seen.append(path) return seen diff --git a/scripts/generate_site.py b/scripts/generate_site.py index 4c240595..18124b0b 100644 --- a/scripts/generate_site.py +++ b/scripts/generate_site.py @@ -2755,6 +2755,12 @@ def _render_emulator_file( for scope, checks in validation.items(): details.append(f"Validation ({scope}): {', '.join(checks)}") if source_ref: + asker = requested_by(profile, source_ref) + if asker: + # A profile reading a libretro core and the original emulator + # cited both trees alike: readers could not tell which code asks + # for the file. + details.append(f"Asked for by: {asker}") details.append( f"Source: {_source_ref_markdown(profile, source_ref)}" ) @@ -4869,6 +4875,7 @@ from sitewrite import ( # noqa: E402,F401 ) from sitesources import ( # noqa: E402,F401 link_prose_citations, + requested_by, revision_markdown, _repo_pins, _forge_sources, diff --git a/scripts/mame_ref_audit.py b/scripts/mame_ref_audit.py index c7c89bdb..c770bdba 100644 --- a/scripts/mame_ref_audit.py +++ b/scripts/mame_ref_audit.py @@ -30,7 +30,8 @@ import upstream from safeparse import yaml_load from scraper.mame_parser import _MACHINE_MACROS, strip_comments -DRIVER_REF = re.compile(r"^(src/[\w./+-]+):(\d+)$") +# An optional role word names the repository the lines are in. +DRIVER_REF = re.compile(r"^(?:(source|upstream) )?(src/[\w./+-]+):(\d+)$") @dataclass(frozen=True) @@ -39,6 +40,10 @@ class Finding: path: str cited: int declared: int + role: str = "" + + def ref(self, line: int) -> str: + return f"{self.role} {self.path}:{line}" if self.role else f"{self.path}:{line}" def declares(line: str, set_name: str) -> bool: @@ -57,11 +62,14 @@ def audit( """Findings, plus counts of refs that agree and of unjudgeable ones.""" with open(os.path.join(emulators_dir, f"{name}.yml"), encoding="utf-8") as handle: document = yaml_load(handle) or {} - repo = upstream.parse_repo(str(document.get("source") or "")) - pin = str(document.get("source_commit") or "") - if repo is None or not pin: + trees = { + role: (upstream.parse_repo(str(document.get(role) or "")), + str(document.get(f"{role}_commit") or "")) + for role in ("source", "upstream") + } + if trees["source"][0] is None or not trees["source"][1]: return [], 0, 0 - sources: dict[str, list[str] | None] = {} + sources: dict[tuple[str, str], list[str] | None] = {} findings: list[Finding] = [] agreed = unjudged = 0 for entry in document.get("files", []) or []: @@ -69,10 +77,14 @@ def audit( match = DRIVER_REF.match(str(entry.get("source_ref") or "")) if not (entry_name.endswith(".zip") and match): continue - path, cited = match.group(1), int(match.group(2)) - if path not in sources: - sources[path] = upstream.fetch_file(repo, pin, path, cache_dir, offline) - lines = sources[path] + role, path, cited = match.group(1) or "", match.group(2), int(match.group(3)) + repo, pin = trees[role or "source"] + if repo is None or not pin: + unjudged += 1 + continue + if (role, path) not in sources: + sources[(role, path)] = upstream.fetch_file(repo, pin, path, cache_dir, offline) + lines = sources[(role, path)] if lines is None: continue set_name = entry_name[:-4] @@ -85,7 +97,7 @@ def audit( # declares the name twice and only a reader can choose. unjudged += 1 continue - findings.append(Finding(set_name, path, cited, found[0])) + findings.append(Finding(set_name, path, cited, found[0], role)) return findings, agreed, unjudged @@ -97,14 +109,14 @@ def rewrite(name: str, emulators_dir: str, findings: list[Finding]) -> int: written = 0 for finding in findings: anchor = f"- name: {finding.set_name}.zip" - old = f'source_ref: "{finding.path}:{finding.cited}"' + old = f'source_ref: "{finding.ref(finding.cited)}"' start = text.find(anchor) if start < 0: continue at = text.find(old, start) if at < 0: continue - new = f'source_ref: "{finding.path}:{finding.declared}"' + new = f'source_ref: "{finding.ref(finding.declared)}"' text = text[:at] + new + text[at + len(old):] written += 1 if written: diff --git a/scripts/profile_sync.py b/scripts/profile_sync.py index b857f8fe..f4f1190b 100644 --- a/scripts/profile_sync.py +++ b/scripts/profile_sync.py @@ -38,10 +38,12 @@ from sourceref import ( # noqa: F401 (re-exported for callers and tests) _LOCATED_RE, _REF_RE, _SPLIT_RE, + REPO_ROLES, RefPart, _trim_prose, parse_source_ref, ref_prose_text, + ref_role, source_ref_values, split_outside_parentheses, split_source_ref, @@ -213,6 +215,9 @@ def strip_repo_word( """ if " " not in path: return path + role, rest = ref_role(path) + if role: + return rest head, _, tail = path.partition(" ") if head.lower() in known and tail and "/" not in head and "." not in head: tail = tail.strip() @@ -1503,8 +1508,14 @@ def build_report( if (repo := upstream.parse_repo(url)) is not None ) refs = [] + # The repository a part says its lines are in (`upstream src/a.c:5`). + cited_roles: dict[str, str] = {} for citation in collect_citations(profile, text): if citation.kind == "ref": + for p in split_source_ref(citation.ref): + role, _bare = ref_role(p.path) + if role: + cited_roles.setdefault(strip_repo_word(p.path, known), role) entry = citation.entry display = str(entry.get("name") or "") or citation.field if citation.label: @@ -1707,10 +1718,13 @@ def build_report( return 2 return 1 if line.strip() else 0 + # A part that names its repository is looked up there first. + wanted = cited_roles.get(path, "") + ordered = sorted(views, key=lambda v: _view_role(v) != wanted) if wanted else views best = None for candidate in candidates: for sha_of in (lambda v: v.pin, lambda v: v.head): - for view in views: + for view in ordered: found = fetch_from(view.repo, sha_of(view), candidate) if found is None: continue @@ -1721,6 +1735,16 @@ def build_report( continue candidate = target rank = score(found) + if rank == 3 and not wanted and _view_role(view) == "source": + # The port carries a copy of the emulator: when the + # original holds the same value on the cited line, + # the code asking for the file is the emulator's. + for other in views: + if _view_role(other) != "upstream": + continue + theirs = fetch_from(other.repo, sha_of(other), candidate) + if theirs is not None and score(theirs) == 3: + return other, candidate if rank == 3: return view, candidate if best is None or rank > best[0]: @@ -1937,6 +1961,11 @@ def build_report( result = verify_at_pin( part, fetch(PIN, part.path, part.start, tokens), tokens, hashes ) + # Checked at its own revision, the part still lies in one tree: + # the report names it, as a comparison does. + owner, _actual = resolve_path(part.path, part.start, tuple(tokens)) + if result.repo is None and owner is not primary: + result = replace(result, repo=owner.repo.slug) else: result = anchor_across_views(part, tokens) # A forge that refused the request said nothing about the file: @@ -2603,6 +2632,68 @@ def blocks_the_pin(report: ProfileReport, accept_changed: bool = False) -> int: return blocked +def _view_role(view: RepoView) -> str: + """source or upstream: the profile field a repository comes from.""" + return "upstream" if view.field == "upstream" else "source" + + +ATTRIBUTED_STATUSES = ("ANCHORED", "SHIFTED", "RENAMED", "MOVED", "CHANGED") + + +def attribute_refs(path: Path, report: ProfileReport, profile: dict) -> list[str]: + """Open each cited path with the role of the repository its lines are in. + + A profile reading a libretro port and the original emulator cited both + trees the same way, and a reader could not tell whether the core or the + emulator asked for the file. The role is the one this comparison found + the lines in, so a word already written that disagrees is corrected. + Parts whose lines were found nowhere keep what they have. + """ + if len(report.repos) < 2: + return [] + role_of: dict[str, str] = {} + for field_name, _mode, url in declared_repositories(profile): + repo = upstream.parse_repo(url) + if repo is not None: + role_of.setdefault(repo.slug, "source" if field_name == "source_mirror" + else field_name) + if len(set(role_of.values())) < 2: + return [] + primary = role_of.get(report.repos[0], "source") + text = path.read_text(encoding="utf-8") + applied: list[str] = [] + cursor = 0 + for entry in report.entries: + if entry.kind != "ref" or entry.name.endswith("]"): + continue + if not _is_rewritable(entry.source_ref): + continue + rendered = [] + for part in entry.parts: + raw = part.part.raw or _original_part(part) + role, bare = ref_role(raw) + written_path = bare.startswith(part.part.path) + owner = role_of.get(part.repo or "", primary) if part.repo else primary + if part.status in ATTRIBUTED_STATUSES and written_path and owner != role: + rendered.append(f"{owner} {bare}") + else: + rendered.append(raw) + new_ref = ", ".join(rendered) + if new_ref == entry.source_ref: + continue + try: + text, index = replace_field_line( + text, "source_ref", entry.source_ref, new_ref, cursor + ) + except YamlWriteError: + continue + cursor = index + 1 + applied.append(f"{entry.source_ref} -> {new_ref}") + if applied: + path.write_text(text, encoding="utf-8") + return applied + + def rebase_refs( path: Path, report: ProfileReport, accept_changed: bool = False ) -> list[str]: @@ -3368,6 +3459,12 @@ def build_parser() -> argparse.ArgumentParser: help="rewrite shifted refs for HEAD and move source_commit with them, " "all or nothing", ) + parser.add_argument( + "--attribute-refs", + action="store_true", + help="open each cited path with source or upstream, the repository its " + "lines are in, for profiles that declare both", + ) parser.add_argument( "--bump-commit", action="store_true", @@ -3504,6 +3601,18 @@ def _apply_writes( return if args.backfill_commits: _backfill(args, name, profile, report, path) + if getattr(args, "attribute_refs", False): + if report.skipped: + return + with tempfile.TemporaryDirectory() as scratch: + target = Path(scratch) / path.name + target.write_bytes(path.read_bytes()) + verb = "would attribute " if args.dry_run else "" + for line in attribute_refs(target, report, profile): + print(f"{name}: {verb}{line}") + if not args.dry_run: + path.write_bytes(target.read_bytes()) + return if not (args.rebase_refs or args.bump_commit) or _stale_prose_blocks(name, path, args): return with tempfile.TemporaryDirectory() as scratch: @@ -3716,6 +3825,16 @@ def _refuse_flag_combinations(args: argparse.Namespace) -> None: file=sys.stderr, ) raise SystemExit(1) + if args.attribute_refs and ( + args.rebase_refs or args.bump_commit or args.backfill_commits + or args.realign_prose + ): + print( + "--attribute-refs runs alone: it rewrites the refs the report was " + "built from.", + file=sys.stderr, + ) + raise SystemExit(1) if args.realign_prose and ( args.rebase_refs or args.bump_commit or args.backfill_commits ): @@ -3786,7 +3905,7 @@ def main() -> None: writes = ( args.backfill_commits or args.rebase_refs or args.bump_commit - or args.realign_prose + or args.realign_prose or args.attribute_refs ) if writes and not args.dry_run and not args.force: _refuse_dirty_tree(args.emulators_dir) diff --git a/scripts/sitesources.py b/scripts/sitesources.py index 3ca75ddb..24613f8a 100644 --- a/scripts/sitesources.py +++ b/scripts/sitesources.py @@ -7,6 +7,7 @@ from __future__ import annotations from siterender import _by_mode import re +from sourceref import ref_role from sourceref import source_ref_values from sourceref import split_source_ref import upstream @@ -118,6 +119,42 @@ def revision_markdown(profile: dict) -> str: return ", ".join(links) +def role_label(profile: dict, role: str) -> str: + """What a reader calls the repository a role names.""" + if role == "upstream": + return "upstream emulator" + return "libretro core" if "libretro" in str(profile.get("type", "")) else "this build" + + +def role_sources(profile: dict, label: str = "") -> dict[str, tuple[upstream.Repo, str]]: + """The forge repository and pin behind each role a citation can name.""" + found: dict[str, tuple[upstream.Repo, str]] = {} + for field in ("source", "upstream"): + for value, pin, _fallback in _repo_pins(profile, field, label): + repo = upstream.parse_repo(value) + if repo is not None and pin and field not in found: + found[field] = (repo, pin) + return found + + +def requested_by(profile: dict, value: str | dict | None) -> str: + """Whose code the citations of an entry are in, said in words.""" + roles = { + ref_role(part.path)[0] + for _label, refs in source_ref_values(value) + for part in split_source_ref(refs) + } - {""} + if not roles or not _two_repositories(profile): + return "" + names = [role_label(profile, role) for role in ("source", "upstream") if role in roles] + return " and ".join(names) + + +def _two_repositories(profile: dict) -> bool: + source, upstream_url = profile.get("source"), profile.get("upstream") + return bool(source and upstream_url and source != upstream_url) + + def _source_ref_markdown(profile: dict, value: str | dict | None) -> str: """Render source_ref values as pinned links when their forge is known. @@ -132,8 +169,16 @@ def _source_ref_markdown(profile: dict, value: str | dict | None) -> str: path_re = re.compile(r"^[A-Za-z0-9_.@/+~-]+$") for label, refs in source_ref_values(value): candidates = _forge_sources(profile, label) + by_role = role_sources(profile, label) rendered: list[str] = [] + current_role = "" for part in split_source_ref(refs): + role, bare = ref_role(part.path) + if bare != part.path or role: + current_role = role + elif part.raw and not part.raw[:1].isdigit(): + current_role = "" + part = type(part)(bare, part.start, part.end, part.raw) display = part.path if part.start is not None: display += f":{part.start}" @@ -146,17 +191,23 @@ def _source_ref_markdown(profile: dict, value: str | dict | None) -> str: # broke 470 links. Two repositories and no tree to read: no link. selected: tuple[upstream.Repo, str] | None = None real_path = part.path - if path_re.fullmatch(part.path) and len(candidates) == 1: - selected = candidates[0] + if path_re.fullmatch(part.path): + if current_role: + # Only the repository the part names, or no link at all. + selected = by_role.get(current_role) + elif len(candidates) == 1: + selected = candidates[0] + tag = (f" ({role_label(profile, current_role)})" + if current_role and _two_repositories(profile) else "") if selected is None: - rendered.append(f"`{display}`") + rendered.append(f"`{display}`{tag}") else: repo, pin = selected url = _source_permalink( repo, pin, real_path, part.start, part.end ) - rendered.append(f"[`{display}`]({url})") + rendered.append(f"[`{display}`]({url}){tag}") text = ", ".join(rendered) if rendered else f"`{refs}`" if label: diff --git a/scripts/sourceref.py b/scripts/sourceref.py index 53c63750..e7f60bbc 100644 --- a/scripts/sourceref.py +++ b/scripts/sourceref.py @@ -16,6 +16,22 @@ _ANNOTATION_RE = re.compile(r"\s*\([^)]*\)?") _LOCATED_RE = re.compile(r"^[^\s:]+:\d+(?:-\d+)?$") +# A part opening with one of these words says which declared repository its +# lines are in: `upstream src/bios.c:40` is the original emulator, +# `source libretro.c:120` the repository the profile names as its source, +# usually the libretro port. A reader could not tell which code asked for +# the file when a profile declares both. +REPO_ROLES = ("source", "upstream") + + +def ref_role(path: str) -> tuple[str, str]: + """The role word a cited path opens with, and the path without it.""" + word, _, rest = path.partition(" ") + if word in REPO_ROLES and rest.strip(): + return word, rest.strip() + return "", path + + @dataclass(frozen=True) class RefPart: path: str diff --git a/tests/test_profile_sync.py b/tests/test_profile_sync.py index 79eaf67d..60e09cfe 100644 --- a/tests/test_profile_sync.py +++ b/tests/test_profile_sync.py @@ -4288,5 +4288,57 @@ class ARecaleKeepsTheRepositoryWord(unittest.TestCase): self.assertEqual(profile_sync._rendered_part(result), expected) +class EachCitationNamesTheCodeThatAsks(unittest.TestCase): + """A profile reading a libretro core and the original emulator cited both + trees alike, and readers could not tell which code asked for a file.""" + + setUp = TestBuildReport.setUp + tearDown = TestBuildReport.tearDown + MD5 = "0123456789abcdef0123456789abcdef" + TEXT = ( + 'emulator: T\n' + 'source: "https://github.com/libretro/a5200"\n' + 'upstream: "https://github.com/alekmaul/a5200"\n' + 'source_commit: "pins"\n' + 'upstream_commit: "pinu"\n' + 'files:\n' + ' - name: 5200.rom\n' + f' md5: {MD5}\n' + ' source_ref: "{ref}"\n' + ) + + def _attribute(self, ref: str) -> str: + trees = { + "libretro/a5200": {"emu/memory.c": ["x", f"rom {self.MD5}"], + "libretro.c": [f"load {self.MD5}"]}, + "alekmaul/a5200": {"emu/memory.c": ["y", f"rom {self.MD5}"]}, + } + profile_sync.upstream.fetch_file = ( + lambda repo, sha, path, cache_dir, offline=False: + trees.get(repo.slug, {}).get(path) + ) + profile_sync.upstream.list_tree = ( + lambda repo, sha, cache_dir, offline=False: (sorted(trees[repo.slug]), False) + ) + profile_sync.upstream.resolve_head = ( + lambda repo, cache_dir, offline=False, branch=None: + "pins" if repo.owner == "libretro" else "pinu" + ) + path = Path(self.dir) / "t.yml" + path.write_text(self.TEXT.replace("{ref}", ref), encoding="utf-8") + profile = yaml.safe_load(path.read_text(encoding="utf-8")) + with contextlib.redirect_stderr(io.StringIO()): + report = build_report("t", profile, self.dir, text=path.read_text()) + profile_sync.attribute_refs(path, report, profile) + return yaml.safe_load(path.read_text(encoding="utf-8"))["files"][0]["source_ref"] + + def test_emulator_code_the_port_carries_is_the_emulators(self): + self.assertEqual(self._attribute("emu/memory.c:2, libretro.c:1"), + "upstream emu/memory.c:2, source libretro.c:1") + + def test_a_written_role_is_followed(self): + self.assertEqual(self._attribute("source emu/memory.c:2"), "source emu/memory.c:2") + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_site_exports.py b/tests/test_site_exports.py index c149f0b6..0213d3e8 100644 --- a/tests/test_site_exports.py +++ b/tests/test_site_exports.py @@ -728,5 +728,33 @@ class ProseCitationsLinkToThePin(unittest.TestCase): self.assertEqual(link_prose_citations(two, "src/a.c:5"), "src/a.c:5") +class ACitationSaysWhoseCodeItIs(unittest.TestCase): + """Readers could not tell whether the libretro core or the emulator asked + for a file: both trees were cited alike, and left unlinked.""" + + PROFILE = {"type": "libretro", "source": "https://github.com/libretro/a5200", + "upstream": "https://github.com/alekmaul/a5200", + "source_commit": "a" * 40, "upstream_commit": "b" * 40} + + def test_each_part_links_to_its_repository_with_its_label(self): + from sitesources import _source_ref_markdown, requested_by + + ref = "source libretro.c:5, upstream emu/memory.c:46, 50-52" + text = _source_ref_markdown(self.PROFILE, ref) + self.assertIn("libretro/a5200/blob/" + "a" * 40 + "/libretro.c#L5) (libretro core)", text) + self.assertIn("alekmaul/a5200/blob/" + "b" * 40 + "/emu/memory.c#L50-L52) (upstream emulator)", + text) + self.assertEqual(requested_by(self.PROFILE, ref), "libretro core and upstream emulator") + self.assertEqual(requested_by(self.PROFILE, "upstream emu/memory.c:46"), + "upstream emulator") + + def test_a_role_without_a_pin_links_nowhere_else(self): + from sitesources import _source_ref_markdown + + profile = {**self.PROFILE, "upstream_commit": ""} + self.assertEqual(_source_ref_markdown(profile, "upstream emu/memory.c:46"), + "`emu/memory.c:46` (upstream emulator)") + + if __name__ == "__main__": unittest.main() diff --git a/wiki/profiling.md b/wiki/profiling.md index 02eb8bc6..25493204 100644 --- a/wiki/profiling.md +++ b/wiki/profiling.md @@ -401,7 +401,7 @@ partitions), and `game_count`. | `has_builtin` | true if the core falls back to a built-in copy when the file is absent | | `config_key` | configuration key the emulator exposes to point at the file | | `load_from` | directory the core reads the file from when it is not the system directory | -| `source_ref` | source file and line number (e.g. `boot.cpp:42`), read at the profile's `source_commit` when set. A dict splits `standalone` from `libretro` when they diverge | +| `source_ref` | source file and line number (e.g. `boot.cpp:42`), read at the profile's `source_commit` when set. A dict splits `standalone` from `libretro` when they diverge. In a profile that declares both `source` and `upstream`, each cited path opens with the one its lines are in: `source libretro.c:120` for the port's own code, `upstream emu/memory.c:46` for the emulator's, including the copy a port carries. `profile_sync --attribute-refs` writes these words from where the lines are found, and the site labels each citation with them | | `path` | destination path relative to system directory | | `standalone_path` | where the standalone build reads the file, under its own data folder, when it differs from `path` (the libretro destination). Packs for a platform running the emulator standalone use it | | `on_mismatch` | what the code does when a declared check fails: `reject` (default) or `warn` and run with it, for every check or check by check (`{size: warn, md5: reject}`) |