diff --git a/scripts/validate_pr.py b/scripts/validate_pr.py index cbc6fa1bd..5a2a0401b 100644 --- a/scripts/validate_pr.py +++ b/scripts/validate_pr.py @@ -24,6 +24,7 @@ import re import shutil import subprocess import sys +import urllib.parse from pathlib import Path sys.path.insert(0, os.path.dirname(__file__)) @@ -72,6 +73,22 @@ LARGE_FILE_ISSUE_URL = ( ) +def large_file_issue_url(result: ValidationResult) -> str: + """The issue form with the values this run computed already filled in. + + GitHub fills an issue form field from the query parameter named by its + id: filename, sha1, md5 and size in missing-bios.yml. + """ + fields = { + "title": f"[BIOS] Missing: {result.filename}", + "filename": result.filename, + "sha1": result.sha1, + "md5": result.md5, + "size": str(result.size), + } + return f"{LARGE_FILE_ISSUE_URL}&{urllib.parse.urlencode(fields)}" + + class ValidationResult: def __init__(self, filepath: str) -> None: self.filepath = filepath @@ -358,14 +375,15 @@ def validate_file( if ext in BLOCKED_EXTENSIONS: result.add_check(False, f"Blocked file extension: {ext}") - if result.size > MAX_FILE_SIZE: + too_large = result.size > MAX_FILE_SIZE + if too_large: # GitHub refuses a pushed file over 100 MiB, so a pull request cannot # carry it. Only a maintainer can add an asset to the release. result.add_check( False, f"File too large for git ({result.size:,} > {MAX_FILE_SIZE:,} bytes). " - "Leave it out of the pull request and open an issue " - f"({LARGE_FILE_ISSUE_URL}) with the hashes above and where the file " + "Leave it out of the pull request and open an issue, its hashes " + f"filled in ({large_file_issue_url(result)}), saying where the file " "can be obtained: a maintainer adds it to the `large-files` release.", ) elif result.size == 0: @@ -449,8 +467,8 @@ def validate_file( result.outcome = "unknown" result.add_warning( "Not recognised: no platform list or emulator profile declares this " - "file. Say in the pull request which emulator reads it, for which " - "system, and where the dump comes from" + f"file. Say in the {'issue' if too_large else 'pull request'} which " + "emulator reads it, for which system, and where the dump comes from" ) normalized = os.path.normpath(relative) @@ -466,7 +484,11 @@ def validate_file( set(), ) expected_folders = sorted(expected) - if normalized.startswith("bios" + os.sep): + if too_large: + # The file stays out of the pull request, so no folder of bios/ is + # the contributor's to choose: the release asset is placed by hand. + pass + elif normalized.startswith("bios" + os.sep): parts = normalized.split(os.sep) here = f"bios/{parts[1]}/{parts[2]}/" if len(parts) >= 4 else "" makers = {folder.split("/")[1] for folder in expected} diff --git a/tests/test_validate_pr_flow.py b/tests/test_validate_pr_flow.py index 0cb7a8667..cab20c4b8 100644 --- a/tests/test_validate_pr_flow.py +++ b/tests/test_validate_pr_flow.py @@ -115,6 +115,30 @@ class OversizedFileAdvice(unittest.TestCase): self.assertIn("open an issue", message) self.assertIn(validate_pr.LARGE_FILE_ISSUE_URL, message) + def test_the_issue_form_is_filled_and_nothing_points_to_the_pr(self): + """The link opened an empty form, and the same report said to put the + file in the pull request and under bios/.""" + import urllib.parse # noqa: PLC0415 + + path = self.tmp / "big.bin" + path.write_bytes(b"x" * 8) + validate_pr.MAX_FILE_SIZE = 4 + result = validate_pr.validate_file( + "big.bin", None, {"sha1": set(), "md5": set(), "names": set()}, + ) + message = next(m for s, m in result.checks if s == "FAIL" and "too large" in m) + url = message.split("(", 2)[2].split(")", 1)[0] + query = urllib.parse.parse_qs(urllib.parse.urlsplit(url).query) + template = (REPO_ROOT / ".github" / "ISSUE_TEMPLATE" / "missing-bios.yml").read_text() + for field, value in (("filename", "big.bin"), ("sha1", result.sha1), + ("md5", result.md5), ("size", "8")): + with self.subTest(field=field): + self.assertIn(f"id: {field}\n", template) + self.assertEqual(query[field], [value]) + others = " ".join(m for _s, m in result.checks) + self.assertNotIn("pull request which", others) + self.assertNotIn("not under bios/", others) + def test_the_linked_issue_template_exists(self): template = validate_pr.LARGE_FILE_ISSUE_URL.rsplit("template=", 1)[1] self.assertTrue(