From 2f92aaee6cf368aaebf03adfd430dfdca5d24884 Mon Sep 17 00:00:00 2001 From: jyviko <2501648+jyviko@users.noreply.github.com> Date: Mon, 19 Oct 2026 10:42:20 -0400 Subject: [PATCH] CI: one tested rule for code changes; review fixes From the review of this PR: - What counts as code lives only in scripts/ci/affected.py now: nothing selected and not full means no code changed, on pull requests and pushes to main alike. The untested bash copy in Select checks disagreed with it. Root files and folders a crate embeds now select that crate (the licence texts and NOTICE: the engine; skills/: the MCP server's tests), where before a change to them ran no test at all. Other workflows and the bots' scripts select nothing. - A push to main skips the native jobs only when nothing a build reads changed since the last push to main whose run passed. Comparing with the push just before was unsafe: a newer push cancels a waiting run, so that commit may never have been tested. - Fail fast applies to pull requests only. On main, nightly and Run workflow the test build and the shards run regardless of the repository checks, so the action matrix and property tests that hang off Build tests keep their signal. Doc tests (main and nightly only now) no longer wait for the checks. - tests/robustness/ turns on the goldens, so a change to goldens.json runs the seed tests. - tests/oracle/ no longer selects the kernel, which never reads it. - The workflow header and CONTRIBUTING describe what main and pull requests now run. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01QWhMwdqJYQKQq8RCLSL1LT --- .github/workflows/ci.yml | 98 +++++++++++++++++-------------------- CONTRIBUTING.md | 10 ++-- scripts/ci/affected.py | 47 ++++++++++++------ scripts/ci/test_affected.py | 18 ++++++- 4 files changed, 99 insertions(+), 74 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6084d20c7f..bfea2f6a02 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,27 +2,27 @@ # PRs test what they touch; main and nightly test everything. On a pull request the "Select # checks" job maps the changed files to workspace packages (scripts/ci/affected.py), adds the # packages that depend on them, and Clippy, the test archive and the test shards cover only -# those. Changes to the lockfile, root manifest, toolchain/cargo/nextest config, .github/, -# vendor/, tests/, scripts/ or any file no package owns select the whole workspace, as do -# pushes to main, the nightly run and Run workflow. +# those. Test data under tests/, vendored crates under vendor/ and root files a crate embeds +# select the packages that read them. Changes to the lockfile, root manifest, +# toolchain/cargo/nextest config, this workflow, scripts/ or any file no package owns select the +# whole workspace, as do pushes to main that change code, the nightly run and Run workflow. +# Documentation, packaging, the other workflows and the GitHub bots' scripts select nothing: +# no native job runs for them, on pull requests or on main. # Tests are built once and run in shards. A few heavy tests (see .config/nextest.toml) are # skipped on pull requests and run on pushes to main and nightly. The Windows jobs (D3D12 shaders # and app startup), the reference tests and the doc tests run on main and nightly only, not on # pull requests (the fast lane, in Select checks); a break there is fixed forward on main. Only -# main saves the build caches; pull requests restore them. The test build, the shards and the doc -# tests start only after the format and repository checks pass. -# The merge queue runs the full suite on exactly the commit it then puts on main, so a push to -# main that a passing merge-queue run already tested runs only the repository checks. A push that -# skipped the queue runs everything when it changes code; one that changes only documentation, -# packaging, other workflows or the GitHub bots' scripts (the same rule as pull requests, -# `touches_code` in Select checks) runs only the repository checks. Test data under tests/ and -# vendored crates under vendor/ select the packages that read them (scripts/ci/affected.py). +# main saves the build caches; pull requests restore them. On pull requests the test build and the +# shards start only after the format and repository checks pass. +# A push to main runs only the repository checks when a passing merge-queue run tested that very +# commit, or when nothing a build reads changed since the last push to main whose run passed. +# Otherwise it runs everything. # Heavy checks wait for review: a pull request without the `approved-for-ci` label runs only # Select checks, the format and repository checks and a quick compile. Adding the label (after # review) runs everything above, and every later push to the PR does too, except a push that only # merges main into a change that already passed (scripts/ci/main_merge.py): it carries that -# result and runs the repository checks. Pushes to main, merge -# groups, the nightly run and Run workflow are not pull requests and always run the full suite. +# result and runs the repository checks. Merge groups, the nightly run and Run workflow are not +# pull requests and always run the full suite. # GitHub cannot filter `labeled` events by label name, so any other label starts a run whose jobs # are all skipped; it gets its own concurrency group (below) so it never cancels the real run. # Cross-compilation is opt-in through Run workflow; release builds remain in release.yml. @@ -130,7 +130,6 @@ jobs: BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} PUSH_SHA: ${{ github.sha }} - BEFORE_SHA: ${{ github.event.before }} REPO: ${{ github.repository }} APPROVED: ${{ contains(github.event.pull_request.labels.*.name, 'approved-for-ci') }} GH_TOKEN: ${{ github.token }} @@ -141,42 +140,38 @@ jobs: windows=false full=true packages= - # Whether any of the NUL-separated paths on stdin is something a build or test reads. - # Pull requests and pushes to main share it, so a docs-only change runs no native job - # on either. - touches_code() { - local path - while IFS= read -r -d '' path; do - case "$path" in - .github/workflows/ci.yml) return 0 ;; - *.rs|Cargo.toml|Cargo.lock|*/Cargo.toml|.cargo/*|rust-toolchain*) return 0 ;; - docs/*|*.md|.github/*|packaging/*|LICENSE*|NOTICE|THIRD-PARTY-LICENSES.txt) ;; - # The GitHub bots' scripts: no Rust, tested in this job (Claim and triage bot tests). - scripts/claim/*|scripts/triage/*) ;; - *) return 0 ;; - esac - done - return 1 + # Whether the NUL-separated paths in $1 reach any code: scripts/ci/affected.py selects + # something or the whole workspace. Documentation, packaging, other workflows and the bots' + # scripts select nothing. Pull requests and pushes to main share it. + select_packages() { + python3 -I scripts/ci/affected.py --paths "$1" > "$RUNNER_TEMP/ci-affected" + cat "$RUNNER_TEMP/ci-affected" + full=$(sed -n 's/^full=//p' "$RUNNER_TEMP/ci-affected") + packages=$(sed -n 's/^packages=//p' "$RUNNER_TEMP/ci-affected") } if [ "$EVENT_NAME" = push ]; then # The queue lands the very commit its merge_group run tested. If that run passed, # testing the push again proves nothing new; otherwise (a merge that skipped the # queue, or the API cannot answer) test everything. - # curl and grep only: Select checks of a push runs on the self-hosted runner. + # The API through curl and grep: Select checks of a push runs on the self-hosted runner. tested=$(curl -fsS -H "Authorization: Bearer $GH_TOKEN" -H "Accept: application/vnd.github+json" \ "https://api.github.com/repos/$REPO/actions/workflows/ci.yml/runs?event=merge_group&head_sha=$PUSH_SHA&status=success&per_page=1" \ | grep -o '"total_count": *[0-9]*' | grep -o '[0-9]*$' || echo 0) if [ "${tested:-0}" -gt 0 ]; then echo "The merge queue already tested $PUSH_SHA: repository checks only." - elif [ -n "$BEFORE_SHA" ] && [ "${BEFORE_SHA#0000000}" = "$BEFORE_SHA" ] \ - && git cat-file -e "$BEFORE_SHA^{commit}" 2>/dev/null \ - && git diff --no-renames --name-only -z "$BEFORE_SHA" "$PUSH_SHA" > "$RUNNER_TEMP/ci-paths" \ - && ! touches_code < "$RUNNER_TEMP/ci-paths"; then - # Nothing a build reads changed (documentation, packaging, other workflows, the bots' - # scripts): the code on main is what the last push already tested. - echo "$BEFORE_SHA..$PUSH_SHA changes no code: repository checks only." + elif tested=$(curl -fsS -H "Authorization: Bearer $GH_TOKEN" -H "Accept: application/vnd.github+json" \ + "https://api.github.com/repos/$REPO/actions/workflows/ci.yml/runs?event=push&branch=main&status=success&per_page=1" \ + | grep -o '"head_sha": *"[0-9a-f]*"' | head -n 1 | grep -o '[0-9a-f]\{40\}') \ + && [ -n "$tested" ] && git merge-base --is-ancestor "$tested" "$PUSH_SHA" \ + && git diff --no-renames --name-only -z "$tested" "$PUSH_SHA" > "$RUNNER_TEMP/ci-paths" \ + && select_packages "$RUNNER_TEMP/ci-paths" && [ "$full" != true ] && [ -z "$packages" ]; then + # Nothing a build reads changed since the last push to main whose run passed (the + # push before may never have run: a newer push cancels a waiting one). + echo "$tested..$PUSH_SHA changes no code: repository checks only." else # Code changed (or the push can't be compared): main runs everything. + full=true + packages= native=true windows=true fi @@ -190,7 +185,7 @@ jobs: case "$path" in crates/kernel/*|vendor/truck-*) kernel=true ;; esac # What can change a robustness golden: the kernel and the crates a design is built with. case "$path" in - crates/kernel/*|crates/engine/*|crates/doc/*|crates/sketch/*|crates/geom/*|vendor/truck-*) goldens=true ;; + crates/kernel/*|crates/engine/*|crates/doc/*|crates/sketch/*|crates/geom/*|vendor/truck-*|tests/robustness/*) goldens=true ;; esac # What the Windows D3D12 jobs exercise: the GPU crate and its shaders, the app and # its startup, the viewport's wgpu glue, the vendored egui-wgpu, their scripts. @@ -199,20 +194,16 @@ jobs: tools/windows-*.ps1|.github/workflows/ci.yml|rust-toolchain*|.cargo/*) windows=true ;; esac done < "$RUNNER_TEMP/ci-paths" - if touches_code < "$RUNNER_TEMP/ci-paths"; then native=true; fi # A lockfile change that moves the graphics stack (wgpu, naga, egui, winit, windows). if [ "$windows" = false ] && grep -qxz 'Cargo.lock' "$RUNNER_TEMP/ci-paths" \ && git diff "$BASE_SHA...$HEAD_SHA" -- Cargo.lock \ | grep -Eq '^[ +-]name = "(wgpu[a-z_-]*|naga|egui[a-z_-]*|eframe|winit|windows[a-z_-]*|d3d12)"'; then windows=true fi - # Pull requests build and test the touched packages and their dependents. - python3 -I scripts/ci/affected.py --paths "$RUNNER_TEMP/ci-paths" > "$RUNNER_TEMP/ci-affected" - cat "$RUNNER_TEMP/ci-affected" - full=$(sed -n 's/^full=//p' "$RUNNER_TEMP/ci-affected") - packages=$(sed -n 's/^packages=//p' "$RUNNER_TEMP/ci-affected") - # Code changed but nothing was selected: be safe and test everything. - if [ "$native" = true ] && [ "$full" != true ] && [ -z "$packages" ]; then full=true; fi + # Pull requests build and test the touched packages and their dependents; nothing + # selected means no code changed. + select_packages "$RUNNER_TEMP/ci-paths" + if [ "$full" = true ] || [ -n "$packages" ]; then native=true; fi # The goldens are solidcraft-engine tests: its tests must be in the archive. if [ "$goldens" = true ] && [ "$full" != true ]; then case " $packages " in *" solidcraft-engine "*) ;; *) packages="$packages solidcraft-engine" ;; esac @@ -361,12 +352,14 @@ jobs: build-tests: name: Build tests - # Fail fast: the test archive (and so the shards) waits for the repository checks, the most - # common cheap failure; heavy jobs used to run on to the end beside a failed format check. + # Fail fast on pull requests: the test archive (and so the shards) waits for the repository + # checks, the most common cheap failure; heavy jobs used to run on to the end beside a failed + # format check. Main, nightly and Run workflow build regardless, so the action matrix and the + # property tests that hang off this job keep their signal. needs: [changes, checks] # The self-hosted shards (main and the merge queue) build from their warm build directories # instead: downloading the 3 GB archive took them longer than building. - if: needs.changes.outputs.native == 'true' && needs.changes.outputs.local != 'true' + if: ${{ !cancelled() && needs.changes.result == 'success' && needs.changes.outputs.native == 'true' && needs.changes.outputs.local != 'true' && (github.event_name != 'pull_request' || needs.checks.result == 'success') }} runs-on: ${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository && (endsWith(format('{0}', github.event.pull_request.number), '7') || endsWith(format('{0}', github.event.pull_request.number), '8') || endsWith(format('{0}', github.event.pull_request.number), '9'))) && 'self-hosted-untrusted' || 'ubuntu-24.04' }} # Fork builds compile inside gVisor on shared boxes; give them room to finish and upload. timeout-minutes: ${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository && (endsWith(format('{0}', github.event.pull_request.number), '7') || endsWith(format('{0}', github.event.pull_request.number), '8') || endsWith(format('{0}', github.event.pull_request.number), '9'))) && 90 || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository) && 60 || 45 }} @@ -413,7 +406,7 @@ jobs: tests: name: Native Linux tests (${{ matrix.shard }}/4) needs: [changes, checks, build-tests] - if: ${{ !cancelled() && needs.changes.outputs.native == 'true' && needs.checks.result == 'success' && (needs.changes.outputs.local == 'true' || needs.build-tests.result == 'success') }} + if: ${{ !cancelled() && needs.changes.outputs.native == 'true' && (github.event_name != 'pull_request' || needs.checks.result == 'success') && (needs.changes.outputs.local == 'true' || needs.build-tests.result == 'success') }} runs-on: ${{ needs.changes.outputs.local == 'true' && 'self-hosted-fast' || ((github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository && (endsWith(format('{0}', github.event.pull_request.number), '7') || endsWith(format('{0}', github.event.pull_request.number), '8') || endsWith(format('{0}', github.event.pull_request.number), '9'))) && 'self-hosted-untrusted' || 'ubuntu-24.04') }} timeout-minutes: 60 strategy: @@ -599,8 +592,7 @@ jobs: doc-tests: name: Doc tests - # After the repository checks pass (see Build tests). - needs: [changes, checks] + needs: changes if: needs.changes.outputs.doctests == 'true' runs-on: ${{ needs.changes.outputs.local == 'true' && 'self-hosted-fast' || 'ubuntu-24.04' }} timeout-minutes: 45 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index aeb2b314f7..b490d3bbc8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -183,10 +183,12 @@ depend on wall-clock timing. **Ready pull requests test what they touch.** CI maps changed files to workspace packages and runs Clippy, the test build and tests for those and their dependents. The [package selector](scripts/ci/affected.py) and [CI workflow](.github/workflows/ci.yml) define -which changes require native checks; documentation, packaging and bot-script-only PRs can run -repository checks only. Workspace-wide changes, merge-queue groups, nightly runs and Run -workflow select the full workspace. A push to `main` reuses a successful merge-queue run for -the same commit; otherwise it runs the full suite. +which changes require native checks; documentation, packaging, other-workflow and bot-script-only +changes run the repository checks only, on pull requests and on `main`. Workspace-wide changes, +merge-queue groups, nightly runs and Run workflow select the full workspace. A push to `main` +reuses a successful merge-queue run for the same commit, and runs only the repository checks +when no code changed since the last push to `main` that passed; otherwise it runs the full +suite. Doc tests, reference tests and the Windows jobs run on `main` and nightly only. **CI shards the tests.** One job builds them into a cargo-nextest archive; two shards run on pull requests and four during full validation. Every test runs in its own process: don't rely diff --git a/scripts/ci/affected.py b/scripts/ci/affected.py index b826086c6f..513db3c85b 100644 --- a/scripts/ci/affected.py +++ b/scripts/ci/affected.py @@ -13,24 +13,37 @@ Prints and optionally appends to $GITHUB_OUTPUT: A changed file maps to the package whose directory holds it. The packages that depend on those (directly or not, dev-dependencies included) are added. Test data under tests/ maps to the -packages that read it, and a vendored crate (vendor/, a [patch] of a crates.io dependency) to the -packages that use it. Anything that can change every package, or that no package owns, selects -the full workspace. Only documentation and packaging files are ignored. +packages that read it, a vendored crate (vendor/, a [patch] of a crates.io dependency) to the +packages that use it, and a root file or folder a crate embeds to that crate. Anything that can +change every package, or that no package owns, selects the full workspace. Documentation, +packaging, the other workflows and the GitHub bots' scripts are ignored: when only those change, +nothing is selected and full is false, which CI reads as "no code changed" (on pull requests and +on pushes to main alike). """ import argparse import json import subprocess import sys -# Changes here can affect every package. -FULL_PREFIXES = (".cargo/", ".config/", ".github/", "scripts/") -FULL_FILES = ("Cargo.toml", "Cargo.lock") -# Changes here affect no code: documentation and packaging. -IGNORED_PREFIXES = ("docs/", "packaging/") +# Changes here can affect every package. The CI workflow itself is one; other workflows are not. +FULL_PREFIXES = (".cargo/", ".config/", "scripts/") +FULL_FILES = ("Cargo.toml", "Cargo.lock", ".github/workflows/ci.yml") +# Changes here affect no code: documentation, packaging, the other workflows and GitHub settings, +# and the GitHub bots' scripts (tested in the repository checks). Root *.md files too, unless +# embedded (below). +IGNORED_PREFIXES = ("docs/", "packaging/", ".github/", "scripts/claim/", "scripts/triage/") +IGNORED_FILES = ("vendor/README.md",) # Root files that a crate embeds (include_str!), so they are not ignorable documentation. -EMBEDDED = {"ATTRIBUTION.md": "solidcraft-ui-egui"} -# Asset folders that only some packages read: the UI embeds the catalogs, xtask checks them. -EMBEDDED_PREFIXES = {"assets/locales/": ("solidcraft-ui-egui", "xtask")} +EMBEDDED = { + "ATTRIBUTION.md": "solidcraft-ui-egui", + "LICENSE-MIT": "solidcraft-engine", + "LICENSE-APACHE": "solidcraft-engine", + "NOTICE": "solidcraft-engine", + "THIRD-PARTY-LICENSES.txt": "solidcraft-engine", +} +# Folders that only some packages read: the UI embeds the catalogs and xtask checks them; the MCP +# server's tests read the agent skills. +EMBEDDED_PREFIXES = {"assets/locales/": ("solidcraft-ui-egui", "xtask"), "skills/": ("solidcraft-mcp",)} # Test data and the package directories whose tests (or xtask) read it. Any other folder under # tests/ selects the full workspace. TEST_DATA = { @@ -38,7 +51,7 @@ TEST_DATA = { "tests/hole_matrix/": ("crates/engine/",), "tests/projection/": ("crates/engine/",), "tests/taper/": ("crates/engine/",), - "tests/oracle/": ("apps/solidcraft-cli/", "crates/kernel/", "xtask/"), + "tests/oracle/": ("apps/solidcraft-cli/", "xtask/"), "tests/action_matrix/": ("apps/solidcraft-cli/", "crates/engine/", "xtask/"), } # Vendored crates (vendor//, a [patch] of a crates.io dependency) and the dependency names @@ -102,12 +115,14 @@ def select(meta, paths): hit = set() for path in paths: - if path in FULL_FILES or path.startswith(FULL_PREFIXES) or path.startswith("rust-toolchain"): + if path in FULL_FILES or path.startswith("rust-toolchain"): + return True, [] + if path in IGNORED_FILES or path.startswith(IGNORED_PREFIXES): + continue + if path.startswith(FULL_PREFIXES): return True, [] owners = [d for d in by_dir if path.startswith(d)] if path.startswith("vendor/"): - if path == "vendor/README.md": - continue users = users_of(path.split("/")[1]) if not users: return True, [] @@ -124,7 +139,7 @@ def select(meta, paths): hit.add(EMBEDDED[path]) elif any(path.startswith(p) for p in EMBEDDED_PREFIXES): hit.update(next(v for p, v in EMBEDDED_PREFIXES.items() if path.startswith(p))) - elif path.startswith(IGNORED_PREFIXES) or ("/" not in path and path.endswith(".md")): + elif "/" not in path and path.endswith(".md"): continue else: return True, [] # not owned by any package diff --git a/scripts/ci/test_affected.py b/scripts/ci/test_affected.py index 6afb880665..34f6ded3ef 100644 --- a/scripts/ci/test_affected.py +++ b/scripts/ci/test_affected.py @@ -52,6 +52,12 @@ class Synthetic(unittest.TestCase): def test_docs_only_selects_nothing(self): self.assertEqual(affected.select(SYNTH, ["docs/book/a.md", "README.md", "packaging/x.yml"]), (False, [])) + def test_no_code_paths_select_nothing(self): + # What CI reads as "no code changed", on pull requests and pushes to main alike. + for p in [".github/workflows/docker.yml", ".github/ISSUE_TEMPLATE/bug.yml", "scripts/claim/claim.js", + "scripts/triage/triage.test.js", "vendor/README.md", "SECURITY.md"]: + self.assertEqual(affected.select(SYNTH, [p]), (False, []), p) + class RealWorkspace(unittest.TestCase): @classmethod @@ -78,11 +84,21 @@ class RealWorkspace(unittest.TestCase): self.assertNotIn("solidcraft-kernel", pk) self.assertNotIn("solidcraft-render", pk) - def test_oracle_cases_select_the_cli_and_xtask(self): + def test_oracle_cases_select_the_cli_and_xtask_not_the_kernel(self): full, pk = affected.select(self.meta, ["tests/oracle/box/recipe.json"]) self.assertFalse(full) self.assertIn("solidcraft-cli", pk) self.assertIn("xtask", pk) + self.assertNotIn("solidcraft-kernel", pk) + + def test_embedded_root_files_select_their_crate(self): + for path, pkg in [("ATTRIBUTION.md", "solidcraft-ui-egui"), ("LICENSE-MIT", "solidcraft-engine"), + ("NOTICE", "solidcraft-engine"), ("THIRD-PARTY-LICENSES.txt", "solidcraft-engine"), + ("skills/solidcraft-modeling/SKILL.md", "solidcraft-mcp")]: + full, pk = affected.select(self.meta, [path]) + self.assertFalse(full, path) + self.assertIn(pkg, pk, path) + self.assertNotIn("solidcraft-kernel", pk, path) def test_unmapped_test_data_selects_full(self): self.assertEqual(affected.select(self.meta, ["tests/tools/x.json"]), (True, []))