diff --git a/.github/workflows/ext-link.yml b/.github/workflows/ext-link.yml new file mode 100644 index 0000000000..bb3d2ca27c --- /dev/null +++ b/.github/workflows/ext-link.yml @@ -0,0 +1,155 @@ +# --------------------------------------------------------------------------- +# ext-link — per-PR LINK check for the `perry-ext-*` crates (#7656) +# +# A `perry-runtime` change broke the link of five `perry-ext-*` crates and no +# per-PR gate could have caught it (#7650, fixed in #7655); it surfaced at the +# next tag, days later. +# +# Why `cargo-test`'s scope cannot see this: `ci_test_scope.py` selects a +# reverse-dependency closure, and `_is_fanout_leaf` deliberately keeps +# `perry-ext-*` / `perry-stdlib` OUT of the fan-out. That is correct on its own +# terms — their unit tests are self-contained pure-Rust logic, and re-running +# ~40 crates on every foundational change is the cost the scoping exists to +# avoid. What it misses is that for these crates the coupling is the LINK, not +# the test: they pull in a feature-stripped runtime through `perry-ffi`'s +# `runtime-link` built with `-Wl,-dead_strip`, so a new reference edge inside +# `perry-runtime` can keep alive a chain the stripper had been removing. In +# #7650 that edge was one added call (`pin_object` -> `arena::classify_heap_space`) +# in code that had previously done a raw flag write, and the symptom was +# `Undefined symbols for architecture arm64`. +# +# So this job BUILDS and does not RUN: `cargo test --no-run` links the test +# binaries and stops. Running them would add time and check nothing this does +# not already. +# +# `--release` deliberately: `-dead_strip` is what makes the failure, and it is +# a release-profile behaviour. A dev-profile build links a different set and +# would be green through exactly the regression this exists to catch. +# --------------------------------------------------------------------------- +name: ext-link + +on: + pull_request: + workflow_dispatch: + +concurrency: + # PR runs supersede each other; a manual dispatch is never cancelled. + group: ext-link-${{ github.event.pull_request.number || github.run_id }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +permissions: + contents: read + +jobs: + ext-link: + runs-on: ubuntu-latest + # Measured on an arm64 dev Mac. COLD release target, the five crates from + # #7650 only: 7:24, 7 test binaries. All 38 ext crates with perry-runtime + # already built: 4:10, 216 test binaries. The shared `perry-runtime` release + # build dominates, so covering every ext crate costs LESS than building the + # runtime once — which is why the scope is all of them rather than the five + # that happened to fail in #7650. + # + # The bound is a backstop, not a budget: it has to cover a cold sccache + # building perry-runtime in release from scratch on a shared runner, which + # is far slower than the numbers above, while still cutting a true hang. + timeout-minutes: 120 + env: + RUSTC_WRAPPER: sccache + SCCACHE_GHA_ENABLED: "false" + SCCACHE_DIR: ${{ github.workspace }}/.sccache + SCCACHE_CACHE_SIZE: "8G" + CARGO_INCREMENTAL: "0" + steps: + # This job compiles PR-controlled build scripts; don't leave the workflow + # token in .git/config for them to read. + - uses: actions/checkout@v7 + with: + persist-credentials: false + + # Cheap gate: no toolchain, no cargo, no cache restore. Every step below + # is skipped when the diff cannot change what the archives link. + - name: Compute ext-link scope + id: scope + env: + GH_TOKEN: ${{ github.token }} + run: | + python3 scripts/ci_ext_link_scope.py --self-test + if [ "${{ github.event_name }}" = "pull_request" ]; then + changed_files="$(gh pr view "${{ github.event.pull_request.number }}" \ + --json files --jq '.files[].path')" + else + # Manual dispatch: check everything. + changed_files="crates/perry-runtime/src/lib.rs" + fi + pkgs="$(printf '%s\n' "$changed_files" | python3 scripts/ci_ext_link_scope.py)" + if [ -z "$pkgs" ]; then + echo "Diff cannot change what the ext archives link — nothing to do." + echo "pkgs=" >> "$GITHUB_OUTPUT" + else + echo "Linking $(printf '%s\n' "$pkgs" | wc -l) perry-ext-* crates:" + printf '%s\n' "$pkgs" + { + echo 'pkgs<> "$GITHUB_OUTPUT" + fi + + - name: Install Rust toolchain + if: steps.scope.outputs.pkgs != '' + uses: dtolnay/rust-toolchain@stable + - if: steps.scope.outputs.pkgs != '' + uses: ./.github/actions/setup-llvm22 + + - name: Install sccache + if: steps.scope.outputs.pkgs != '' + uses: mozilla-actions/sccache-action@v0.0.10 + + - name: Cache sccache objects + if: steps.scope.outputs.pkgs != '' + uses: actions/cache@v6 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-${{ runner.os }}-perry-${{ github.job }}-${{ github.run_id }} + restore-keys: | + sccache-${{ runner.os }}-perry- + + - uses: Swatinem/rust-cache@v2 + if: steps.scope.outputs.pkgs != '' + with: + shared-key: "${{ runner.os }}-perry" + save-if: ${{ github.ref == 'refs/heads/main' }} + + - name: Link the ext crates + if: steps.scope.outputs.pkgs != '' + env: + # lld has repeatedly SIGBUS'd large links on the shared runner. + CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUSTFLAGS: "-C linker-features=-lld" + CARGO_PROFILE_TEST_DEBUG: "0" + CARGO_BUILD_JOBS: "1" + PKGS: ${{ steps.scope.outputs.pkgs }} + run: | + # Explicit rather than relying on the runner's default `bash -e`: a + # failing `cargo test --no-run` IS the regression this job exists to + # catch, so its exit status must abort the step under any shell. + set -euo pipefail + args="" + while read -r pkg; do + [ -n "$pkg" ] || continue + args="$args -p $pkg" + done <<< "$PKGS" + + # `--message-format=json` so the count below comes from cargo rather + # than being inferred from exit status. The counting lives in the + # script (`--count-linked`) so this file carries no inline Python — + # a heredoc at column 0 silently breaks the YAML block scalar. + # shellcheck disable=SC2086 + cargo test --release --no-run --message-format=json $args > /tmp/ext-link.json + + linked="$(python3 scripts/ci_ext_link_scope.py --count-linked /tmp/ext-link.json)" + echo "linked test binaries: $linked" + if [ "$linked" -eq 0 ]; then + echo "::error::ext-link linked ZERO test binaries. The gate ran but had no subject — either the scope selected no packages or cargo built nothing. Fix the scope rather than trusting this green." + exit 1 + fi diff --git a/changelog.d/7768-ext-link-gate.md b/changelog.d/7768-ext-link-gate.md new file mode 100644 index 0000000000..c90c43d503 --- /dev/null +++ b/changelog.d/7768-ext-link-gate.md @@ -0,0 +1,15 @@ +### Changed + +- **CI: a per-PR gate now LINKS the `perry-ext-*` crates (#7656).** A `perry-runtime` change broke the link of five of them and no per-PR gate could have caught it (#7650, fixed in #7655) — it surfaced at the next tag, days later. + + `cargo-test`'s scope could not see it, and not by accident: `ci_test_scope.py`'s `_is_fanout_leaf` deliberately keeps `perry-ext-*` / `perry-stdlib` out of the reverse-dependency fan-out, because their unit tests are self-contained pure-Rust logic and re-running ~40 crates on every foundational change is the cost that scoping exists to avoid. That reasoning is right about the tests and silent about the **link**: these crates pull in a feature-stripped runtime through `perry-ffi`'s `runtime-link` built with `-Wl,-dead_strip`, so a new reference edge inside `perry-runtime` can keep alive a chain the stripper had been removing. In #7650 the edge was one added call (`pin_object` → `arena::classify_heap_space`) replacing a raw flag write, and the symptom was `Undefined symbols for architecture arm64`. + + So the new `ext-link` job builds and does not run: `cargo test --release --no-run` links the test binaries and stops. `--release` deliberately — `-dead_strip` is a release-profile behaviour, and a dev-profile build would be green through exactly this regression. The existing fan-out exclusion is left alone. + + **The package list is derived, not written down.** `scripts/ci_ext_link_scope.py` enumerates `crates/perry-ext-*` from the workspace, so a new ext crate is covered the day it lands — the failure mode #7748 had to repair in `ci_e2e_scope.py`, where a hand-maintained map named 3 of 24 suites and nothing could say so. Its self-test additionally asserts the five crates that actually failed in #7650 are still in the derived list, so a rename reports itself instead of quietly shrinking the gate. + + **The job asserts it linked something.** #7656's first requirement, and the failure mode this repo has shipped four times (#6942/#6946, #7024, #7025): a scope rule that selected zero packages would be green forever. The count comes from cargo's own `--message-format=json` artifact records, and zero is a hard failure with an error that says to fix the scope rather than trust the green. + + **Cost, measured, and why the scope is all 38 crates rather than #7650's five.** On an arm64 dev Mac: a COLD release target building only the five took **7:24** (7 test binaries); all **38** ext crates with `perry-runtime` already built took **4:10** (216 test binaries). The shared runtime build dominates, so widening the scope to every ext crate costs less than building the runtime once — the issue's "a wider set is better if it is affordable", answered with a number instead of an argument. + + Not made a required context yet, per CLAUDE.md's corollary — a new gate has never been green, so promoting it immediately blocks every open PR. Run it once, then promote. diff --git a/scripts/ci_ext_link_scope.py b/scripts/ci_ext_link_scope.py new file mode 100755 index 0000000000..e26f30b6df --- /dev/null +++ b/scripts/ci_ext_link_scope.py @@ -0,0 +1,194 @@ +#!/usr/bin/env python3 +"""Decide whether the per-PR `ext-link` gate must run, and over which packages. + +#7656: a `perry-runtime` change broke the LINK of five `perry-ext-*` crates and +no per-PR gate could have caught it (#7650, fixed in #7655). It would have +surfaced at the next tag, days later. + +## Why the existing scope cannot see this + +`ci_test_scope.py` selects a reverse-dependency closure, and `_is_fanout_leaf` +deliberately keeps `perry-ext-*` / `perry-stdlib` OUT of the fan-out. That +exclusion is correct on its own terms — those crates' unit tests are +self-contained pure-Rust logic and re-running ~40 of them on every foundational +change is exactly the cost the scoping exists to avoid. + +What it misses is that for these crates the coupling is not the test, it is the +**link**. They pull in a feature-stripped runtime through `perry-ffi`'s +`runtime-link`, built with `-Wl,-dead_strip`, so any new reference edge inside +`perry-runtime` can keep a chain alive the stripper had been removing. In #7650 +the edge was a single added call (`pin_object` -> `arena::classify_heap_space`) +in code that had previously done a raw flag write. The symptom is +`Undefined symbols for architecture arm64`, not a failing assertion — so +BUILDING the ext crates is the entire check, and running their tests adds +nothing. + +Hence a separate, link-only arm rather than widening the closure: `cargo test +--no-run` builds (and therefore links) the test binaries and stops. + +## Selection + +The gate runs when the diff touches a crate whose object code ends up inside +those archives — `perry-runtime`, `perry-stdlib`, `perry-ffi` — or any +`perry-ext-*` crate directly. + +The package list is DERIVED from the workspace, never hand-listed: a new +`crates/perry-ext-/` is covered the day it lands. A hand-maintained list is +the failure mode #7748 had to repair in `ci_e2e_scope.py`, where the map named +3 of 24 suites and nothing could say so. + +Usage: | python3 scripts/ci_ext_link_scope.py + python3 scripts/ci_ext_link_scope.py --count-linked + python3 scripts/ci_ext_link_scope.py --self-test +""" +import os +import sys + +# A change to any of these can alter what the ext archives' dead-strip pass +# keeps, so they arm the gate. `perry-ffi` is here because it is the crate that +# actually declares `runtime-link`. +LINK_SOURCE_PREFIXES = ( + "crates/perry-runtime/", + "crates/perry-stdlib/", + "crates/perry-ffi/", +) + +EXT_PREFIX = "crates/perry-ext-" + + +def _repo_root() -> str: + return os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + + +def ext_packages(root: str): + """Every `perry-ext-*` workspace member, from the directory listing. + + Derived rather than listed so a new ext crate is covered on the day it + lands. Only directories carrying a `Cargo.toml` count, so a stray path + cannot inject a bogus `-p` argument. + """ + crates = os.path.join(root, "crates") + try: + entries = os.listdir(crates) + except OSError: + return [] + return sorted( + e + for e in entries + if e.startswith("perry-ext-") + and os.path.isfile(os.path.join(crates, e, "Cargo.toml")) + ) + + +def arms(changed) -> bool: + """Does this diff require the ext link check?""" + for path in changed: + path = path.strip() + if not path: + continue + if path.startswith(LINK_SOURCE_PREFIXES) or path.startswith(EXT_PREFIX): + return True + return False + + +def _self_test() -> int: + root = _repo_root() + + cases = [ + (["crates/perry-runtime/src/gc/mod.rs"], True), + (["crates/perry-stdlib/src/lib.rs"], True), + (["crates/perry-ffi/src/async_runtime.rs"], True), + (["crates/perry-ext-http/src/lib.rs"], True), + # Nothing that ends up in the archives. + (["crates/perry-hir/src/lower.rs"], False), + (["crates/perry-codegen/src/expr/mod.rs"], False), + (["docs/src/index.md", "CLAUDE.md"], False), + ([], False), + # One armed path among many is enough. + (["README.md", "crates/perry-runtime/src/value.rs"], True), + ] + for changed, expected in cases: + got = arms(changed) + if got != expected: + print(f"self-test FAILED: {changed} -> {got}, want {expected}", file=sys.stderr) + return 1 + + # The package list must be non-empty and must actually name ext crates. A + # scope rule that selected zero packages would make the gate green forever + # — the failure mode this repo has shipped four times (#6942/#6946, #7024, + # #7025) and the first thing #7656 asks to get right. + pkgs = ext_packages(root) + if len(pkgs) < 5: + print( + f"self-test FAILED: expected the workspace to have several " + f"perry-ext-* crates, found {len(pkgs)}: {pkgs}", + file=sys.stderr, + ) + return 1 + if any(not p.startswith("perry-ext-") for p in pkgs): + print(f"self-test FAILED: non-ext package in list: {pkgs}", file=sys.stderr) + return 1 + # The five that actually failed in #7650 must be covered by the derived + # list — if a rename drops one, this says so instead of quietly shrinking. + for name in ( + "perry-ext-pdf", + "perry-ext-lru-cache", + "perry-ext-node-forge", + "perry-ext-mongodb", + "perry-ext-http", + ): + if name not in pkgs: + print( + f"self-test FAILED: {name} (one of #7650's five) is not in the " + f"derived package list", + file=sys.stderr, + ) + return 1 + + print(f"ci_ext_link_scope self-test: ok ({len(pkgs)} ext crates)") + return 0 + + +def count_linked(path: str) -> int: + """Test binaries cargo reports as linked, from `--message-format=json`. + + THE COUNT IS THE POINT. A scope rule that silently selected zero packages, + or a cargo invocation that built nothing, leaves the gate green forever — + the failure mode this repo has shipped four times (#6942/#6946, #7024, + #7025), and the first thing #7656 asks to get right. The job asserts this is + non-zero, so "nothing threw" is never mistaken for "the subject was live". + """ + import json + + linked = 0 + with open(path, encoding="utf-8", errors="replace") as fh: + for line in fh: + try: + msg = json.loads(line) + except Exception: + continue + if msg.get("reason") == "compiler-artifact" and msg.get("executable"): + linked += 1 + print(f"linked {msg['target']['name']}", file=sys.stderr) + return linked + + +def main() -> int: + if "--self-test" in sys.argv: + return _self_test() + + if "--count-linked" in sys.argv: + path = sys.argv[sys.argv.index("--count-linked") + 1] + print(count_linked(path)) + return 0 + + changed = [line for line in sys.stdin] + if not arms(changed): + return 0 + for pkg in ext_packages(_repo_root()): + print(pkg) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main())