Repository navigation
ci: skip build steps for docs-only pull requests - #74
Conversation
![biobo-bot[bot]](https://avatars.githubusercontent.com/in/5358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nice change. Keeping the required jobs alive and turning off only their steps is the right way around the PR #45 problem, since a skipped matrix job would otherwise report under its unexpanded name and never satisfy branch protection. The classifier also fails safe in the right places: unknown paths count as code, non-pull_request events run everything, and a failed git diff falls back to running everything.
I checked:
- All six workflows gate every step of the required jobs (
build-and-test, the full matrix,coverage,Ubuntu / Clang / Debug, the compile-time bench build,clang-format), and only the non-required ones (clang-tidy,fuzz, the benchmark comparison) are skipped whole. Report build timeusesneeds.changes.outputs.code == 'true' && always(), which keeps thealways()semantics, so it's fine.fetch-depth: 2withHEAD^1on the defaultpull_requestmerge checkout gives exactly the PR's files.- The
perfset coversinclude/,bench/,cmake/, the bench scripts, the top-levelCMakeLists.txtandci-pr.ymlitself, which matches what the benchmark job builds.
There's one small hole, noted inline: rename detection can hide a moved code file. It's a one-flag fix. Approving, since it's an edge case, but please pick it up before merging.
Non-blocking nit: the changes job is copied into six workflows. If it ever grows, a reusable workflow or a composite action would keep them from drifting apart. That's fine to leave as is for now.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Benchmark comparison (informational)Base Noise estimate: the 20 control rows below do not run Patternia code, so they should not change between base and PR. As a group they moved by a median of +0.0% (run order and machine drift); that offset is divided out of the Patternia deltas (Adjusted). The controls then scatter by a median of 0.2%. Adjusted differences under 5.0% are treated as noise. Patternia: 8 benchmarks, 0 slower, 0 faster, 8 within noise.
Control rows (not Patternia code)
Raw results and the comparison chart are attached to this workflow run as an artifact. Shared runners are noisy, so use this as a trend, not a measurement. |
Problem - Every pull request ran the whole matrix, clang-tidy, fuzzing and a 15 minute benchmark comparison, even when it only touched docs, agent instructions or packaging metadata. - The workflows cannot use paths-ignore: branch protection requires their checks, and a skipped workflow leaves them unreported. Implementation - Add scripts/ci_changes.py, which classifies the files a pull request changes and reports code and perf outputs. Anything it cannot classify counts as code, and non-pull_request events always run. - Add a changes job to ci, ci-pr, ci-full, clang-format, clang-tidy and fuzz. - Required jobs keep running and skip their steps, so the check names stay unchanged, including the expanded matrix names. clang-tidy, fuzz and the benchmark comparison are skipped as whole jobs, and the benchmark runs only when a file that can move its numbers changed. Tests - Classified sample path sets, and a simulated pull_request merge checkout, with the script. - Workflow YAML parses. The skipping itself cannot run here: this pull request changes workflows, so it runs every job.
34b9c2e to
6761e52
Compare
There was a problem hiding this comment.
Re-reviewed 6761e52 against the previously reviewed 34b9c2e (same base, amended commit). The only change is in scripts/ci_changes.py: changed_files() now runs git diff --no-renames --name-only HEAD^1 HEAD, which addresses the earlier rename comment. I checked that a moved header (include/a.hpp and docs/a.hpp) classifies as code=true, and a docs-only set still gives code=false. No other files changed. LGTM.
Summary
Docs-only pull requests (for example #66, #68, #70) ran the full compiler matrix, clang-tidy, fuzzing and a roughly 15 minute benchmark comparison. This adds change detection so those pull requests skip the expensive work, while the required check names keep being reported.
Changes
scripts/ci_changes.py: classifies the files a pull request changes.code=falseonly when every file is documentation, agent instructions (ai/,.claude/) or packaging metadata (packaging/);perf=truewhen a file that can move benchmark numbers changed. Unknown files count as code; pushes, schedules and manual runs always run everything.ci.yml,ci-pr.yml,ci-full.yml,clang-format-check.yml: achangesjob, and every step of the required jobs getsif: needs.changes.outputs.code == 'true'. The jobs still run (a few seconds each), so check names, including the expanded matrix names such asgcc-13 C++23, are unchanged. Branch protection does not need to change.clang-tidy.yml,fuzz.yml: not required, so the whole job is skipped for docs-only changes.ci-pr.ymlbenchmark comparison: runs only whenperfis true, so test-only and CI-only pull requests skip it too.scripts/README.md: documents the script and the rules.Testing
code=false; any test, header or workflow file:code=true; headers:perf=true), and gave the right answer on a simulatedpull_requestmerge checkout, where the base moving does not leak into the file list.