diff --git a/.github/workflows/labels.yml b/.github/workflows/labels.yml index ebaaec842e..299adbcdb8 100644 --- a/.github/workflows/labels.yml +++ b/.github/workflows/labels.yml @@ -28,6 +28,9 @@ jobs: force_label ci 5319e7 '.github' force_label agc 1d76db 'libSceAgc*' force_label shader 1d76db 'core/shader' + force_label 'unblocks title' 0e8a16 'Names a title ID on the Unblocks line' + force_label perf fef2c0 'Performance change' + force_label 'perf numbers missing' e99695 'perf pull request without a before and after summary line' label relinker 5319e7 'core/relinker' label libc fbca04 'libc' diff --git a/.github/workflows/pr-labels.yml b/.github/workflows/pr-labels.yml new file mode 100644 index 0000000000..6d96cce3f8 --- /dev/null +++ b/.github/workflows/pr-labels.yml @@ -0,0 +1,35 @@ +name: PR labels + +on: + pull_request_target: + branches: [main] + types: [opened, edited, reopened, synchronize] + schedule: + - cron: '45 * * * *' + workflow_dispatch: + +permissions: {} + +concurrency: + group: pr-labels-${{ github.event.pull_request.number || 'all' }} + +jobs: + labels: + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: read + pull-requests: write + issues: write + steps: + - uses: actions/checkout@v7 + - env: + GH_TOKEN: ${{ github.token }} + GITHUB_REPOSITORY: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + if [ -n "$PR_NUMBER" ]; then + python3 -I tools/pr_labels.py --pr "$PR_NUMBER" + else + python3 -I tools/pr_labels.py --all + fi diff --git a/CMakeLists.txt b/CMakeLists.txt index e652e28cff..39e489e674 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -119,6 +119,8 @@ if(BUILD_TESTING) ${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_pr_overlap_dependencies.py) add_test(NAME pr_overlap_tools COMMAND ${Python3_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_pr_overlap.py) + add_test(NAME pr_labels COMMAND ${Python3_EXECUTABLE} + ${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_pr_labels.py) add_test(NAME progress_history COMMAND ${Python3_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_progress_history.py) add_test(NAME progress_report COMMAND ${Python3_EXECUTABLE} diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1f7568da8d..e862604893 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -58,12 +58,12 @@ The [pull request template](.github/pull_request_template.md) is the checklist f - A pull request that conflicts with `main` and has no activity for 7 days is labeled `stale` with a reminder. It is not closed. - If a pull request needs another one first, say so on the checklist line (`Depends on: #N`; `none` otherwise). - If a pull request fixes a reported issue, write `Fixes #N` on its checklist line (`none` otherwise). GitHub closes the issue on merge, and the `fix error` label is added within the hour. -- If a pull request fixes a stop or a wrong result seen on a title, name it on the checklist line with the title ID and its [Game bring-up](https://github.com/boykopovar/AnyPS5/discussions/categories/game-bring-up) discussion (`Unblocks: PPSA01325 #3232`; `none` otherwise). Reviewers review these first. +- If a pull request fixes a stop or a wrong result seen on a title, name it on the checklist line with the title ID and its [Game bring-up](https://github.com/boykopovar/AnyPS5/discussions/categories/game-bring-up) discussion (`Unblocks: PPSA01325 #3232`; `none` otherwise). Reviewers review these first. Such a pull request gets the `unblocks title` label. - For GPU changes, list each tested GPU's vendor, model, driver and OS on the checklist line. CI runs GPU tests only on lavapipe, so reviewers may ask for a run on another vendor before merging. - Fixes a title needs go to `main` as pull requests. A Game bring-up discussion lists the open pull requests and local changes the title still needs; update the list when one is merged, closed or replaced. - The PR overlap comment lists open pull requests that change the same lines or exports; agree with their authors which one goes first and declare it with `Depends on`. - Run the tests before opening the pull request and describe what was tested (OS, title or homebrew). -- A pull request that claims a change in speed shows the summary lines of `main` and of the change, from the same machine and scene, and attaches the traces, captured as the [Performance report](.github/ISSUE_TEMPLATE/performance.yml) issue form describes. A slow scene is reported with the same form. +- A pull request that claims a change in speed shows the summary lines of `main` and of the change, from the same machine and scene, and attaches the traces, captured as the [Performance report](.github/ISSUE_TEMPLATE/performance.yml) issue form describes. A slow scene is reported with the same form. A pull request titled `perf(...)` or `perf:` is labeled `perf`, and `perf numbers missing` until its description holds two valid summary lines with different commits. - Changes to the AGC driver, the shader recompiler or VideoOut name each GPU the GPU tests ran on (vendor, model, driver, OS) on the `GPUs tested` checklist line. CI runs these tests only on lavapipe, and hardware drivers fail where lavapipe and other vendors pass, so a reviewer can ask for a run on another vendor before merging. - Investigation notes, reports, screenshots and logs go in the pull request, not in the repository. Images for documentation go in the [gist](https://gist.github.com/boykopovar/0e53f2e1426f29ecd41e3b51540b8a90) comments. - Say whether the change was written with AI assistance. The author of the pull request is responsible for every line of it. diff --git a/tools/pr_labels.py b/tools/pr_labels.py new file mode 100644 index 0000000000..0ca6f4724b --- /dev/null +++ b/tools/pr_labels.py @@ -0,0 +1,153 @@ +import argparse +import json +import os +import re +import subprocess +import sys +import time +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) +import performance_compare + +UNBLOCKS = re.compile(r'Unblocks:[ \t]*([^\r\n]*(?:\r?\n[ \t]*[-*][ \t]+(?!\[[ xX]\])[^\r\n]*)*)') +TITLE_ID = re.compile(r"(?" +UNBLOCKS_LABEL = "unblocks title" +PERF_LABEL = "perf" +MISSING_LABEL = "perf numbers missing" +MANAGED = {UNBLOCKS_LABEL, PERF_LABEL, MISSING_LABEL} +WRITE_PAUSE = 1.0 +COMMENT = (f"{MARKER}\nThis pull request is labeled `perf`, so it needs the summary line of a capture on `main` and one with the change, " + "from the same title, scene and machine, as the Performance report form asks. " + "Please paste both `aps5-summary.jsonl` lines, attach the two traces and add the output of `tools/trace_digest.py`.") + + +def gh(*args): + return subprocess.run(["gh", *args], capture_output=True, text=True, encoding="utf-8", check=True).stdout + + +def repository(): + return os.environ.get("GITHUB_REPOSITORY") or gh("repo", "view", "--json", "nameWithOwner", "--jq", ".nameWithOwner").strip() + + +def strip_comments(body): + return re.sub(r"", "", body or "", flags=re.S) + + +def title_ids(body): + match = UNBLOCKS.search(strip_comments(body)) + return set(TITLE_ID.findall(match.group(1))) if match else set() + + +def is_perf(title): + return bool(PERF.match(title or "")) + + +def has_numbers(body): + body = strip_comments(body) + candidates = [line.strip() for line in body.splitlines()] + [block.strip() for block in FENCE.findall(body)] + commits = set() + for text in candidates: + if not text.startswith("{"): + continue + try: + summary = json.loads(text) + if isinstance(summary, dict): + commits.add(performance_compare.validate(summary)["commit"]) + except (ValueError, RecursionError): + continue + return len(commits) >= 2 + + +def labels_for(pr): + wanted = set() + if title_ids(pr["body"]): + wanted.add(UNBLOCKS_LABEL) + if is_perf(pr["title"]): + wanted.add(PERF_LABEL) + if not has_numbers(pr["body"]): + wanted.add(MISSING_LABEL) + current = set(pr["labels"]) + return wanted - current, (MANAGED & current) - wanted + + +def decode_pages(out): + decoder = json.JSONDecoder() + offset = 0 + items = [] + while offset < len(out): + while offset < len(out) and out[offset].isspace(): + offset += 1 + if offset == len(out): + break + page, offset = decoder.raw_decode(out, offset) + if not isinstance(page, list): + raise ValueError("gh api --paginate returned a non-array page") + items.extend(page) + return items + + +def pull(data): + return {"number": data["number"], "title": data["title"], "body": data["body"] or "", + "labels": [label["name"] for label in data["labels"]], "state": data["state"]} + + +def commented(repo, number): + out = gh("api", "--paginate", f"repos/{repo}/issues/{number}/comments", + "--jq", f'[.[] | select(.body | contains("{MARKER}"))] | length') + return sum(int(count) for count in out.split()) + + +def process(repo, pr, dry_run, comment=True): + if pr["state"] != "open": + return set(), set(), False + add, remove = labels_for(pr) + wants_comment = comment and (MISSING_LABEL in add or (MISSING_LABEL in pr["labels"] and MISSING_LABEL not in remove)) + post = wants_comment and not commented(repo, pr["number"]) + if not dry_run: + if add or remove: + args = ["pr", "edit", str(pr["number"]), "-R", repo] + if add: + args += ["--add-label", ",".join(sorted(add))] + if remove: + args += ["--remove-label", ",".join(sorted(remove))] + gh(*args) + if post: + gh("api", "-X", "POST", f"repos/{repo}/issues/{pr['number']}/comments", "-f", f"body={COMMENT}") + return add, remove, post + + +def main(): + parser = argparse.ArgumentParser(description="Label pull requests from their title and template fields.") + which = parser.add_mutually_exclusive_group(required=True) + which.add_argument("--pr", type=int, help="label one pull request") + which.add_argument("--all", action="store_true", help="label every open pull request") + parser.add_argument("--dry-run", action="store_true", help="print the planned changes and write nothing") + args = parser.parse_args() + repo = repository() + if args.pr is not None: + prs = [pull(json.loads(gh("api", f"repos/{repo}/pulls/{args.pr}")))] + else: + prs = [pull(data) for data in decode_pages(gh("api", "--paginate", f"repos/{repo}/pulls?state=open&base=main&per_page=100"))] + failed = [] + for pr in prs: + try: + add, remove, post = process(repo, pr, args.dry_run, comment=args.pr is not None) + except subprocess.CalledProcessError as error: + failed.append(pr["number"]) + print(f"cannot label #{pr['number']}: {(error.stderr or '').strip()}", file=sys.stderr) + continue + if add or remove or post: + plan = [f"+{label}" for label in sorted(add)] + [f"-{label}" for label in sorted(remove)] + (["comment"] if post else []) + print(f"{'would change' if args.dry_run else 'changed'} #{pr['number']}: {' '.join(plan)}") + if args.all and not args.dry_run: + time.sleep(WRITE_PAUSE) + if failed: + sys.exit(f"{len(failed)} pull requests not labeled: {', '.join(f'#{number}' for number in failed)}") + + +if __name__ == "__main__": + main() diff --git a/tools/tests/test_pr_labels.py b/tools/tests/test_pr_labels.py new file mode 100644 index 0000000000..73a0ccd559 --- /dev/null +++ b/tools/tests/test_pr_labels.py @@ -0,0 +1,181 @@ +import copy +import json +import sys +import unittest +from pathlib import Path +from unittest.mock import patch + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) +import pr_labels + +SUMMARY = { + "commit": "abc123", "dirty": False, "env": {"APS5_TRACE_FRAMES": "600:4"}, + "gpu": "GPU", "driver": "Driver 1", "os": "OS 1", "resolution": "1920x1080", + "window": {"spec": "frames 600:4", "first_present": 600, "ms": 80.0}, "presents": 4, + "frame_ms": {"p50": 20.0, "p90": 40.0, "p99": 40.0, "max": 40.0}, + "gpu_busy_ms_per_present": 5.0, "worker_busy_pct": {"queue worker 0x0": 50.0}, + "top_phases_ms_per_frame": {"Driver.Packet": 12.0, "Driver.Present": 2.0}, + "trace_dropped": False, "trace": "aps5-trace-abc123-7.json.gz", +} +TEMPLATE = ("- [ ] Unblocks: \n" + "- [ ] Depends on: none\n") + + +def summary(commit): + record = copy.deepcopy(SUMMARY) + record["commit"] = commit + return json.dumps(record) + + +def pr(title="fix: x", body="", labels=()): + return {"number": 7, "title": title, "body": body, "labels": list(labels), "state": "open"} + + +class TitleIdTests(unittest.TestCase): + def test_forms(self): + cases = [ + (TEMPLATE, set()), + ("- [x] Unblocks: PPSA01325 #3232\n", {"PPSA01325"}), + ("- [ ] Unblocks: **PPSA01325 #3232, CUSA12345 #1**", {"PPSA01325", "CUSA12345"}), + ("- [ ] Unblocks: none\nSeen on PPSA01325.\n", set()), + ("- [ ] Unblocks:\n - PPSA01325 #3232\n - PPSA00001\n- [ ] Depends on: none\n", {"PPSA01325", "PPSA00001"}), + ("- [ ] Unblocks:\n- [ ] Depends on: PPSA01325\n", set()), + ("- [ ] Unblocks: \n", set()), + ("- [x] Unblocks: PPSA01325_00 #3232\n", {"PPSA01325"}), + ("- [x] Unblocks: PPSA013250, XPPSA01325\n", set()), + ("", set()), + ] + for body, expected in cases: + with self.subTest(body=body): + self.assertEqual(pr_labels.title_ids(body), expected) + + def test_none_body(self): + self.assertEqual(pr_labels.title_ids(None), set()) + + +class PerfTests(unittest.TestCase): + def test_titles(self): + for title, expected in [("perf(agc): x", True), ("perf: x", True), ("perf!: x", True), ("perf(agc)!: x", True), + ("fix(perf): x", False), ("feat: perf", False), ("performance: x", False)]: + with self.subTest(title=title): + self.assertEqual(pr_labels.is_perf(title), expected) + + +class NumbersTests(unittest.TestCase): + def test_counts(self): + cases = [ + ("", False), + (summary("a"), False), + (summary("a") + "\n" + summary("b"), True), + ("before\n" + summary("a") + "\nafter\n" + summary("b") + "\n", True), + (summary("a") + "\n" + summary("a"), False), + (summary("a") + "\n{not json", False), + ("{}\n{}", False), + ("```json\n" + json.dumps(json.loads(summary("a")), indent=2) + "\n```\n```json\n" + summary("b") + "\n```", True), + ("\n" + summary("b"), False), + (summary("a") + "\r\n" + summary("b") + "\r\n", True), + (summary("a") + "\n{" + "[" * 100000 + "\n" + summary("b"), True), + ] + for body, expected in cases: + with self.subTest(body=body[:40]): + self.assertEqual(pr_labels.has_numbers(body), expected) + + def test_invalid_summary_not_counted(self): + broken = json.loads(summary("b")) + broken["presents"] = 0 + self.assertFalse(pr_labels.has_numbers(summary("a") + "\n" + json.dumps(broken))) + + +class LabelTests(unittest.TestCase): + def test_labels(self): + self.assertEqual(pr_labels.labels_for(pr()), (set(), set())) + self.assertEqual(pr_labels.labels_for(pr("perf: x", TEMPLATE)), ({"perf", "perf numbers missing"}, set())) + both = TEMPLATE + summary("a") + "\n" + summary("b") + self.assertEqual(pr_labels.labels_for(pr("perf: x", both, ["perf", "perf numbers missing"])), (set(), {"perf numbers missing"})) + self.assertEqual(pr_labels.labels_for(pr("fix: x", "Unblocks: PPSA01325", ["perf", "other"])), ({"unblocks title"}, {"perf"})) + self.assertEqual(pr_labels.labels_for(pr("fix: x", "Unblocks: none", ["unblocks title", "other"])), (set(), {"unblocks title"})) + + +class ProcessTests(unittest.TestCase): + def run_process(self, pull, comments=0, dry_run=False, comment=True): + calls = [] + + def fake(*args): + calls.append(args) + return "" + + with patch.object(pr_labels, "gh", side_effect=fake), patch.object(pr_labels, "commented", return_value=comments) as counted: + result = pr_labels.process("o/r", pull, dry_run, comment) + self.comment_lookups = counted.call_count + return result, calls + + def test_backfill_labels_without_commenting(self): + result, calls = self.run_process(pr("perf: x", TEMPLATE), comment=False) + self.assertEqual(result, ({"perf", "perf numbers missing"}, set(), False)) + self.assertEqual(calls, [("pr", "edit", "7", "-R", "o/r", "--add-label", "perf,perf numbers missing")]) + self.assertEqual(self.comment_lookups, 0) + + def test_no_edit_when_labels_match(self): + result, calls = self.run_process(pr("perf: x", TEMPLATE, ["perf", "perf numbers missing"]), comments=1) + self.assertEqual(result, (set(), set(), False)) + self.assertEqual(calls, []) + + def test_edit_and_single_comment(self): + result, calls = self.run_process(pr("perf: x", TEMPLATE)) + self.assertEqual(result, ({"perf", "perf numbers missing"}, set(), True)) + self.assertEqual(calls[0], ("pr", "edit", "7", "-R", "o/r", "--add-label", "perf,perf numbers missing")) + self.assertIn("-f", calls[1]) + self.assertIn(pr_labels.MARKER, calls[1][-1]) + self.assertEqual(len(calls), 2) + + def test_comment_not_posted_twice(self): + result, calls = self.run_process(pr("perf: x", TEMPLATE, ["perf", "perf numbers missing"]), comments=1) + self.assertFalse(result[2]) + result, calls = self.run_process(pr("perf: x", TEMPLATE), comments=1) + self.assertEqual(len(calls), 1) + + def test_comment_restored_for_labeled_pr_without_one(self): + result, calls = self.run_process(pr("perf: x", TEMPLATE, ["perf", "perf numbers missing"])) + self.assertTrue(result[2]) + self.assertEqual(len(calls), 1) + + def test_numbers_present_removes_label_keeps_comment(self): + body = summary("a") + "\n" + summary("b") + result, calls = self.run_process(pr("perf: x", body, ["perf", "perf numbers missing"]), comments=1) + self.assertEqual(result, (set(), {"perf numbers missing"}, False)) + self.assertEqual(calls, [("pr", "edit", "7", "-R", "o/r", "--remove-label", "perf numbers missing")]) + + def test_dry_run_writes_nothing(self): + result, calls = self.run_process(pr("perf: x", TEMPLATE), dry_run=True) + self.assertEqual(result[2], True) + self.assertEqual(calls, []) + + def test_closed_pull_request_ignored(self): + pull = pr("perf: x", TEMPLATE) + pull["state"] = "closed" + self.assertEqual(self.run_process(pull)[1], []) + + +class MainTests(unittest.TestCase): + def test_failed_pull_request_does_not_stop_the_others(self): + listed = [{"number": number, "title": "perf: x", "body": "", "labels": [], "state": "open"} for number in (1, 2)] + edited = [] + + def fake(*args): + if args[0] == "api": + return json.dumps(listed) + edited.append(args[2]) + if args[2] == "1": + raise pr_labels.subprocess.CalledProcessError(1, "gh", stderr="HTTP 403") + return "" + + with patch.object(pr_labels, "gh", side_effect=fake), patch.object(pr_labels, "WRITE_PAUSE", 0), \ + patch.object(sys, "argv", ["pr_labels.py", "--all"]), patch.dict(pr_labels.os.environ, {"GITHUB_REPOSITORY": "o/r"}): + with self.assertRaises(SystemExit) as raised: + pr_labels.main() + self.assertEqual(edited, ["1", "2"]) + self.assertIn("#1", str(raised.exception.code)) + + +if __name__ == "__main__": + unittest.main()