From a8fd595608a3227674eb7dfeec31688f1a84d091 Mon Sep 17 00:00:00 2001 From: Nathanael Huffman Date: Mon, 17 Aug 2026 15:36:04 -0400 Subject: [PATCH] Fix grapefruit releaser and some other release bugs --- .github/scripts/resolve-base-commit.py | 135 +++++++++++++++++++++++++ .github/workflows/build.yml | 47 +++++++-- .github/workflows/simulation.yml | 47 +++++++-- tools/fpga_releaser/cli.py | 44 ++++++-- tools/fpga_releaser/config.toml | 2 +- 5 files changed, 244 insertions(+), 31 deletions(-) create mode 100644 .github/scripts/resolve-base-commit.py diff --git a/.github/scripts/resolve-base-commit.py b/.github/scripts/resolve-base-commit.py new file mode 100644 index 00000000..0d655970 --- /dev/null +++ b/.github/scripts/resolve-base-commit.py @@ -0,0 +1,135 @@ +#!/usr/bin/env python3 +"""Pick the git revision BTD should diff the current commit against. + +On a branch the answer is just origin/main. On main it needs to be the last +commit we actually finished building, which is *not* the previous commit: + + - A push can carry several commits. Diffing against HEAD~1 only sees what the + tip commit touched, so anything changed by an earlier commit in the same + push is never built. That is how cosmo_seq's hash engine landed without a + bitstream. + - `concurrency.cancel-in-progress` kills a run when the next push arrives. + Whatever that run was partway through is simply lost, and a per-push diff + has no way to notice. + +Anchoring to the head_sha of the last successful run of this workflow makes both +cases self-healing: a skipped-over or cancelled commit widens the next run's +diff instead of dropping out of it. A failed run is not a valid anchor either -- +its failed targets still need building -- so only successful runs count. + +Writes `base_commit=` to $GITHUB_OUTPUT. An empty value means no usable +base was found and the caller must build everything. +""" + +import argparse +import json +import os +import subprocess +import sys +import urllib.error +import urllib.request + +NULL_SHA = "0" * 40 + + +def git(*args): + return subprocess.run( + ["git", *args], capture_output=True, text=True, check=False + ) + + +def usable(rev, head): + """A base has to exist locally and be an ancestor of HEAD. + + Force-pushes can name a commit that is gone or that lives on an abandoned + line of history; diffing against either produces a changes list full of + spurious reverts. + """ + if not rev or rev == NULL_SHA: + return False + if git("cat-file", "-e", f"{rev}^{{commit}}").returncode != 0: + print(f" {rev[:12]}: not present in this clone") + return False + if git("merge-base", "--is-ancestor", rev, head).returncode != 0: + print(f" {rev[:12]}: not an ancestor of {head}") + return False + return True + + +def last_successful_run_shas(repo, workflow, branch, token, limit=20): + """head_shas of recent successful runs of this workflow, newest first.""" + url = ( + f"https://api.github.com/repos/{repo}/actions/workflows/{workflow}/runs" + f"?branch={branch}&status=success&per_page={limit}" + ) + req = urllib.request.Request(url, headers={"Accept": "application/vnd.github+json"}) + if token: + req.add_header("Authorization", f"Bearer {token}") + try: + with urllib.request.urlopen(req, timeout=30) as resp: + data = json.load(resp) + except (urllib.error.URLError, json.JSONDecodeError, TimeoutError) as e: + # Not fatal: we still have github.event.before to fall back to. + print(f"::warning::Could not query workflow runs ({e}); falling back") + return [] + return [r["head_sha"] for r in data.get("workflow_runs", [])] + + +def resolve(args): + if args.ref != f"refs/heads/{args.branch}": + # On a branch we want every target the branch touches, not just what + # the last push touched, so origin/main is the right base. + return "origin/main" + + print(f"Looking for the last successful {args.workflow} run on {args.branch}") + for sha in last_successful_run_shas( + args.repo, args.workflow, args.branch, args.token + ): + if sha == args.head: + # A re-run of the current commit. Nothing new to build. + print(f" {sha[:12]}: is HEAD, using it") + return sha + if usable(sha, args.head): + print(f"Using last successful build at {sha[:12]}") + return sha + + print("::warning::No usable successful run found; falling back to event.before") + if usable(args.event_before, args.head): + print(f"Using push base {args.event_before[:12]}") + return args.event_before + + print("::warning::No usable base commit; all targets will be built") + return "" + + +def main(): + parser = argparse.ArgumentParser() + parser.add_argument("--ref", required=True, help="github.ref") + parser.add_argument("--event-before", default="", help="github.event.before") + parser.add_argument("--workflow", required=True, help="workflow filename") + parser.add_argument("--repo", required=True, help="owner/name") + parser.add_argument("--branch", default="main") + parser.add_argument("--head", default="HEAD") + args = parser.parse_args() + args.token = os.environ.get("GITHUB_TOKEN", "") + + # Resolve HEAD once so ancestry checks and the re-run comparison both work + # against a full sha. + rev_parse = git("rev-parse", args.head) + if rev_parse.returncode != 0: + print(f"::error::Cannot resolve {args.head}: {rev_parse.stderr.strip()}") + return 1 + args.head = rev_parse.stdout.strip() + + base = resolve(args) + print(f"base_commit={base}") + + out = os.environ.get("GITHUB_OUTPUT") + if out: + with open(out, "a") as f: + f.write(f"base_commit={base}\n") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index d119cbaf..da99492c 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -5,6 +5,9 @@ on: concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true +permissions: + contents: read + actions: read # resolve-base-commit.py reads this workflow's run history jobs: detect-changes: runs-on: self-hosted @@ -33,16 +36,31 @@ jobs: echo "/opt/oss-cad-suite-20250211/bin" >> "$GITHUB_PATH" python3 -m pip install --upgrade -r tools/requirements.txt --break-system-packages - - name: Get changed files and base commit + - name: Resolve BTD base commit + id: base + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + python3 .github/scripts/resolve-base-commit.py \ + --ref "${{ github.ref }}" \ + --event-before "${{ github.event.before }}" \ + --workflow build.yml \ + --repo "${{ github.repository }}" + + - name: Get changed files id: changed-files run: | - # On main (post-merge), compare against the previous commit; - # on branches, compare against origin/main to detect all changes. - if [ "${{ github.ref }}" = "refs/heads/main" ]; then - BASE_COMMIT="HEAD~1" - else - BASE_COMMIT="origin/main" + BASE_COMMIT="${{ steps.base.outputs.base_commit }}" + + # An empty base means we could not establish what has already been + # built, so the only safe answer is to build everything. + if [ -z "$BASE_COMMIT" ]; then + : > /tmp/changes.txt + echo "force_all=true" >> "$GITHUB_OUTPUT" + echo "ci_changed=false" >> "$GITHUB_OUTPUT" + exit 0 fi + echo "force_all=false" >> "$GITHUB_OUTPUT" # Generate list of changed files in BTD format: "M path/to/file" # BTD expects Mercurial-like format with status code (M/A/D) and space separator @@ -60,8 +78,6 @@ jobs: head -1 /tmp/changes.txt | od -c fi - echo "base_commit=$BASE_COMMIT" >> "$GITHUB_OUTPUT" - # Check if this workflow or the shared parser script changed. # These aren't buck2 inputs so BTD won't detect them, but they # can affect builds (toolchain versions, env vars, build flags). @@ -72,13 +88,15 @@ jobs: fi - name: Generate base snapshot + if: steps.changed-files.outputs.force_all != 'true' run: | - git checkout ${{ steps.changed-files.outputs.base_commit }} + git checkout ${{ steps.base.outputs.base_commit }} supertd targets //... 2>/dev/null > /tmp/base.jsonl echo "Base snapshot: $(wc -l < /tmp/base.jsonl) targets" git checkout ${{ github.sha }} - name: Generate diff snapshot + if: steps.changed-files.outputs.force_all != 'true' run: | supertd targets //... 2>/dev/null > /tmp/diff.jsonl echo "Diff snapshot: $(wc -l < /tmp/diff.jsonl) targets" @@ -87,6 +105,15 @@ jobs: id: btd continue-on-error: true run: | + # /tmp survives between runs on the self-hosted runners, so never let + # a stale snapshot from a previous run stand in for a skipped step. + if [ "${{ steps.changed-files.outputs.force_all }}" = "true" ]; then + echo "::warning::No usable base commit, will run all targets" + echo '[]' > /tmp/btd_output.json + echo "succeeded=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + # Note: Requires supertd to be installed # Install with: cargo install --git https://github.com/facebookincubator/buck2-change-detector.git supertd echo "Checking for supertd installation..." diff --git a/.github/workflows/simulation.yml b/.github/workflows/simulation.yml index cada24f9..a265f5cd 100644 --- a/.github/workflows/simulation.yml +++ b/.github/workflows/simulation.yml @@ -5,6 +5,9 @@ on: concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true +permissions: + contents: read + actions: read # resolve-base-commit.py reads this workflow's run history jobs: detect-changes: runs-on: self-hosted @@ -30,16 +33,31 @@ jobs: echo "$HOME/.cargo/bin" >> "$GITHUB_PATH" python3 -m pip install --upgrade -r tools/requirements.txt --break-system-packages - - name: Get changed files and base commit + - name: Resolve BTD base commit + id: base + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + python3 .github/scripts/resolve-base-commit.py \ + --ref "${{ github.ref }}" \ + --event-before "${{ github.event.before }}" \ + --workflow simulation.yml \ + --repo "${{ github.repository }}" + + - name: Get changed files id: changed-files run: | - # On main (post-merge), compare against the previous commit; - # on branches, compare against origin/main to detect all changes. - if [ "${{ github.ref }}" = "refs/heads/main" ]; then - BASE_COMMIT="HEAD~1" - else - BASE_COMMIT="origin/main" + BASE_COMMIT="${{ steps.base.outputs.base_commit }}" + + # An empty base means we could not establish what has already been + # tested, so the only safe answer is to run everything. + if [ -z "$BASE_COMMIT" ]; then + : > /tmp/changes.txt + echo "force_all=true" >> "$GITHUB_OUTPUT" + echo "ci_changed=false" >> "$GITHUB_OUTPUT" + exit 0 fi + echo "force_all=false" >> "$GITHUB_OUTPUT" # Generate list of changed files in BTD format: "M path/to/file" # BTD expects Mercurial-like format with status code (M/A/D) and space separator @@ -57,8 +75,6 @@ jobs: head -1 /tmp/changes.txt | od -c fi - echo "base_commit=$BASE_COMMIT" >> "$GITHUB_OUTPUT" - # Check if this workflow or the shared parser script changed. # These aren't buck2 inputs so BTD won't detect them, but they # can affect simulation runs (toolchain versions, env vars). @@ -69,13 +85,15 @@ jobs: fi - name: Generate base snapshot + if: steps.changed-files.outputs.force_all != 'true' run: | - git checkout ${{ steps.changed-files.outputs.base_commit }} + git checkout ${{ steps.base.outputs.base_commit }} supertd targets //... 2>/dev/null > /tmp/base.jsonl echo "Base snapshot: $(wc -l < /tmp/base.jsonl) targets" git checkout ${{ github.sha }} - name: Generate diff snapshot + if: steps.changed-files.outputs.force_all != 'true' run: | supertd targets //... 2>/dev/null > /tmp/diff.jsonl echo "Diff snapshot: $(wc -l < /tmp/diff.jsonl) targets" @@ -84,6 +102,15 @@ jobs: id: btd continue-on-error: true run: | + # /tmp survives between runs on the self-hosted runners, so never let + # a stale snapshot from a previous run stand in for a skipped step. + if [ "${{ steps.changed-files.outputs.force_all }}" = "true" ]; then + echo "::warning::No usable base commit, will run all targets" + echo '[]' > /tmp/btd_output.json + echo "succeeded=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + # Note: Requires supertd to be installed # Install with: cargo install --git https://github.com/facebookincubator/buck2-change-detector.git supertd echo "Checking for supertd installation..." diff --git a/tools/fpga_releaser/cli.py b/tools/fpga_releaser/cli.py index 3dc6452a..bc0fb525 100644 --- a/tools/fpga_releaser/cli.py +++ b/tools/fpga_releaser/cli.py @@ -67,8 +67,12 @@ def main(): sys.exit(1) api = GhApi(owner='oxidecomputer', repo='quartz', token=token) - zip_file = process_gh_build(args, api, project_info.job_name) + zip_file, build_sha = process_gh_build(args, api, project_info.job_name) project_info.add_archive(zip_file) + # Pin the release to the commit CI actually built, not to whatever main + # points at by the time we get around to releasing. + if build_sha is not None: + project_info.add_build_sha(build_sha) # Do build reports timing_passed = project_info.report_timing() @@ -99,7 +103,10 @@ def main(): def process_gh_build(args, api, name: str): - + """ + Return the build archive and the commit sha it was built from (None for a + locally supplied archive, where we have no way to know). + """ # Get build archive zip file # Get the latest artifact from the repo since we didn't specify a zip file if args.zip is None: @@ -107,12 +114,14 @@ def process_gh_build(args, api, name: str): # Download the artifact from github artifact_inf = get_latest_artifact_info(api, name, branch=args.branch) zip_file = download_artifact(api, artifact_inf) + build_sha = artifact_inf["workflow_run"]["head_sha"] else: print("Using local zip file") # Use the zip file from the command line zip_file = zipfile.ZipFile(args.zip) + build_sha = None - return zip_file + return zip_file, build_sha def get_latest_artifact_info(api, fpga_name: str, branch: str = "main") -> dict: @@ -121,18 +130,33 @@ def get_latest_artifact_info(api, fpga_name: str, branch: str = "main") -> dict: """ artifacts = api.actions.list_artifacts_for_repo(name=fpga_name) artifacts = obj2dict(artifacts) - - artifacts = list(filter(lambda x: x["workflow_run"]["head_branch"] == branch, artifacts["artifacts"])) - if len(artifacts) == 0: - print(f"No artifacts found for {fpga_name} on {branch}") - return None - artifacts = sorted(artifacts, key=lambda x: arrow.get(x["created_at"]), reverse=True) - return artifacts[0] + + on_branch = [x for x in artifacts["artifacts"] if x["workflow_run"]["head_branch"] == branch] + if len(on_branch) == 0: + print(f"No artifacts named {fpga_name} found on {branch}") + print("Check that job_name in config.toml matches the CI artifact name") + sys.exit(1) + # GitHub keeps expired artifacts in the listing but refuses to serve them, + # so drop them here rather than failing at download time. + live = [x for x in on_branch if not x["expired"]] + if len(live) == 0: + newest = max(on_branch, key=lambda x: arrow.get(x["created_at"])) + print(f"All {fpga_name} artifacts on {branch} have expired " + f"(most recent: {newest['created_at']})") + print("Re-run the build in CI to get a fresh artifact") + sys.exit(1) + live = sorted(live, key=lambda x: arrow.get(x["created_at"]), reverse=True) + return live[0] def download_artifact(api: GhApi, artifact_inf: dict): print(f"Downloading artifact {artifact_inf['name']} from GH: {artifact_inf['workflow_run']['head_branch']}") r = requests.get(artifact_inf["archive_download_url"], auth=("oxidecomputer", os.getenv("GITHUB_TOKEN", None))) + if r.status_code != 200: + # The body is JSON on error, and handing it to ZipFile just yields a + # confusing BadZipFile, so say what GitHub actually told us. + print(f"Failed to download artifact ({r.status_code}): {r.text}") + sys.exit(1) return zipfile.ZipFile(io.BytesIO(r.content)) diff --git a/tools/fpga_releaser/config.toml b/tools/fpga_releaser/config.toml index 54f541b5..14955463 100644 --- a/tools/fpga_releaser/config.toml +++ b/tools/fpga_releaser/config.toml @@ -22,7 +22,7 @@ builder = "buck2" toolchain = "vivado" [grapefruit] -job_name = "gfruit-image" +job_name = "grapefruit-image" hubris_path = "drv/spartan7-loader/grapefruit" builder = "buck2" toolchain = "vivado"