From 840f9494edff75895b3e56a3ee458857d1b5989c Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 5 Oct 2026 20:31:10 +0200 Subject: [PATCH 1/4] ci: skip build and test jobs on docs-only pull requests A pull request that changes only files under docs/, or .md and .rst files, now skips the build, test, lint and sanitizer jobs in CI and Quality. scripts/ci_code_changed.sh decides from the merge commit diff. Renames count on both sides, so moving a source file under docs/ still runs everything. Pushes to main always run everything. ci-ok and quality-ok always run. They fail when a job they depend on failed or was cancelled, and on a code change also when one was skipped. They are the checks branch rules should require. A matrix job skipped by its `if` reports one check with the matrix expression unexpanded, so a required check named after a matrix job would never report. postgres and clang-tidy stay outside the gates, as they are not required today. Also pass the clang-tidy file list through an array and split the TSAN_OPTIONS exports from their assignment, which actionlint flagged. --- .github/workflows/ci.yml | 49 ++++++++++++++++++++++++++++ .github/workflows/quality.yml | 61 +++++++++++++++++++++++++++++++++-- CONTRIBUTING.md | 2 ++ QUALITY_DECLARATION.md | 12 ++++--- scripts/ci_code_changed.sh | 34 +++++++++++++++++++ 5 files changed, 150 insertions(+), 8 deletions(-) create mode 100755 scripts/ci_code_changed.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index df003f4cf..0760387d0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,7 +7,26 @@ on: branches: [main] jobs: + # code=false only on a pull request that changes documentation alone. The + # build and test jobs skip then, and ci-ok still reports. + changes: + runs-on: ubuntu-latest + timeout-minutes: 5 + outputs: + code: ${{ steps.classify.outputs.code }} + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + fetch-depth: 2 + + - name: Classify the change + id: classify + run: ./scripts/ci_code_changed.sh >> "$GITHUB_OUTPUT" + build-and-test: + needs: changes + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest strategy: fail-fast: false @@ -154,6 +173,8 @@ jobs: # so the changes most likely to break it are outside its own tree. graph-watchdog: name: graph_watchdog (${{ matrix.ros_distro }}) + needs: changes + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest strategy: fail-fast: false @@ -286,6 +307,8 @@ jobs: # builds with the option OFF, its default. 14 is the oldest supported server. postgres: name: postgres (${{ matrix.ros_distro }}) + needs: changes + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest strategy: fail-fast: false @@ -418,6 +441,8 @@ jobs: # # The job id stays jazzy-test because notify-demos gates on it by name. jazzy-test: + needs: changes + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest container: image: ros:jazzy-ros-base @@ -525,6 +550,30 @@ jobs: log/ build/*/test_results/ + # The one check branch rules require from this workflow. A matrix job skipped by + # its `if` reports under an unexpanded name, so the rules cannot name the jobs. + # always(), not !cancelled(): a skipped gate would count as passed. + ci-ok: + if: always() + needs: [changes, build-and-test, graph-watchdog, jazzy-test] + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Required jobs passed, or skipped on a docs-only change + env: + CODE: ${{ needs.changes.outputs.code }} + NEEDS: ${{ toJSON(needs) }} + run: | + jq -r 'to_entries[] | "\(.key): \(.value.result)"' <<<"$NEEDS" + allowed='["success"]' + if [[ "$CODE" == "false" ]]; then allowed='["success","skipped"]'; fi + bad=$(jq -r --argjson ok "$allowed" \ + 'to_entries[] | select(.value.result | IN($ok[]) | not) | .key' <<<"$NEEDS") + if [[ -n "$bad" ]]; then + echo "::error::required jobs did not pass: ${bad//$'\n'/ }" + exit 1 + fi + coverage: # Push to main only. This job owned CI's critical path in 79% of PR runs at # ~40 min, and what a PR loses by not running it is narrow: its Codecov diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index 73bb47c18..fb24e08f0 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -14,7 +14,26 @@ permissions: contents: read jobs: + # code=false only on a pull request that changes documentation alone. The + # lint and sanitizer jobs skip then, and quality-ok still reports. + changes: + runs-on: ubuntu-latest + timeout-minutes: 5 + outputs: + code: ${{ steps.classify.outputs.code }} + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + fetch-depth: 2 + + - name: Classify the change + id: classify + run: ./scripts/ci_code_changed.sh >> "$GITHUB_OUTPUT" + format-lint: + needs: changes + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest container: image: ros:jazzy-ros-base @@ -89,6 +108,8 @@ jobs: run: colcon test-result --verbose clang-tidy: + needs: changes + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest container: image: ros:jazzy-ros-base @@ -206,9 +227,10 @@ jobs: source install/setup.bash if [ -n "$CHANGED_FILES" ]; then # Incremental: specific files + read -r -a changed_files <<<"$CHANGED_FILES" run-clang-tidy -p build -clang-tidy-binary ctcache-wrapper \ -header-filter='.*ros2_medkit.*' \ - $CHANGED_FILES + "${changed_files[@]}" else # Full: analyze all source files under src/ros2_medkit_*/src/ # Intentionally excludes test/ directories (tested via sanitizer jobs) @@ -218,6 +240,8 @@ jobs: fi sanitizer-asan: + needs: changes + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest container: image: ros:jazzy-ros-base @@ -368,6 +392,8 @@ jobs: done sanitizer-tsan: + needs: changes + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest container: image: ros:jazzy-ros-base @@ -455,7 +481,8 @@ jobs: # sanitizer's own *SAN_OPTIONS, so this is belt and braces. MEDKIT_TEST_SANITIZED: 1 run: | - export TSAN_OPTIONS="halt_on_error=0:history_size=4:suppressions=$(pwd)/tsan_suppressions.txt" + TSAN_OPTIONS="halt_on_error=0:history_size=4:suppressions=$(pwd)/tsan_suppressions.txt" + export TSAN_OPTIONS source /opt/ros/jazzy/setup.bash source install/setup.bash failed=0 @@ -489,6 +516,8 @@ jobs: # discovery, so the changes most likely to break it are not in its own tree. sanitizer-graph-watchdog: name: Sanitizer ${{ matrix.name }} (graph_watchdog) + needs: changes + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest strategy: fail-fast: false @@ -597,7 +626,8 @@ jobs: MEDKIT_TEST_SANITIZED: 1 run: | if [ "${{ matrix.sanitizer }}" = "tsan" ]; then - export TSAN_OPTIONS="halt_on_error=0:history_size=4:suppressions=$(pwd)/tsan_suppressions.txt" + TSAN_OPTIONS="halt_on_error=0:history_size=4:suppressions=$(pwd)/tsan_suppressions.txt" + export TSAN_OPTIONS else # detect_leaks=0: FastDDS allocator leaks on shutdown (not our code) # new_delete_type_mismatch=0: ROS 2 DDS scalar/array new/delete mismatch @@ -614,3 +644,28 @@ jobs: run: | colcon test-result --test-result-base build/ros2_medkit_graph_watchdog \ --verbose 2>/dev/null || true + + # The one check branch rules require from this workflow. A matrix job skipped by + # its `if` reports under an unexpanded name, so the rules cannot name the jobs. + # always(), not !cancelled(): a skipped gate would count as passed. clang-tidy + # stays out: it is advisory. + quality-ok: + if: always() + needs: [changes, format-lint, sanitizer-asan, sanitizer-tsan, sanitizer-graph-watchdog] + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Required jobs passed, or skipped on a docs-only change + env: + CODE: ${{ needs.changes.outputs.code }} + NEEDS: ${{ toJSON(needs) }} + run: | + jq -r 'to_entries[] | "\(.key): \(.value.result)"' <<<"$NEEDS" + allowed='["success"]' + if [[ "$CODE" == "false" ]]; then allowed='["success","skipped"]'; fi + bad=$(jq -r --argjson ok "$allowed" \ + 'to_entries[] | select(.value.result | IN($ok[]) | not) | .key' <<<"$NEEDS") + if [[ -n "$bad" ]]; then + echo "::error::required jobs did not pass: ${bad//$'\n'/ }" + exit 1 + fi diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3c098fa78..c06c64305 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -222,6 +222,8 @@ build. All PRs are tested on Ubuntu 24.04 (Jazzy), where build and tests run in a single job, plus Humble and Lyrical. Linters and clang-tidy run in the Quality workflow. Coverage is uploaded to Codecov on push to main. ccache is configured in the CI and Quality workflows and in the OPC-UA workflow's colcon jobs. The Pixi workflow and the Docker image builds compile the workspace without it, so a slow build there is expected rather than a cache problem. +A pull request that changes only documentation - files under `docs/`, or `.md` and `.rst` files anywhere - skips the build, test, lint and sanitizer jobs. `scripts/ci_code_changed.sh` makes that call. Branch rules require two checks, `ci-ok` (CI workflow) and `quality-ok` (Quality workflow). Each passes when every job in its `needs` list passed, or was skipped on a docs-only change. A new job that must block a merge goes into one of those `needs` lists. + Every ccache-backed build step ends with `scripts/ccache_report.sh`, which writes the hit rate, cache size and cleanup count to the job summary. It raises a workflow warning in the two states that make a cache useless: cleanups above zero, meaning the cache evicted objects the same build was still producing and `CCACHE_MAXSIZE` is too small for it, and a hit rate under 50% with no cleanups, meaning the cache was never restored and the key or its `restore-keys` prefix is wrong. It never fails the step - a cold cache is a legitimate state. ### Pull Request Checklist diff --git a/QUALITY_DECLARATION.md b/QUALITY_DECLARATION.md index 913f2c6ea..665c5924d 100644 --- a/QUALITY_DECLARATION.md +++ b/QUALITY_DECLARATION.md @@ -84,8 +84,10 @@ GitHub Copilot code review is used in addition to human review. ### Continuous Integration [2.iv] -Every pull request runs the following, and a failure in any of them blocks -review: +Every pull request that changes more than documentation runs the following, and +a failure in any of them blocks review. A pull request that changes only files +under `docs/`, or `.md` and `.rst` files, skips Build & Test, Quality, clang-tidy +and Sanitizers: - **Build & Test:** build + unit/integration tests on Ubuntu Noble / ROS 2 Jazzy, Ubuntu Jammy / ROS 2 Humble, and Ubuntu Resolute / ROS 2 Lyrical. `ros2_medkit_opcua` is excluded here and covered by its own workflow instead - **Quality:** `clang-format` and the other ament linters, the static coverage-scope check, which fails if a package compiles production C++ without opting into coverage instrumentation, and two source gates (`check_no_naked_subscriptions.sh`, `check_handlers_typed_query.sh`). Jazzy only @@ -99,9 +101,9 @@ Reported on pushes to `main`, not run on pull requests: - **Coverage:** Debug build with coverage instrumentation. Reports are archived as artifacts and uploaded to [Codecov](https://codecov.io/gh/selfpatch/ros2_medkit) Merging additionally requires one approving review. Branch rules enforce the -review requirement and a linear history; the checks above are not configured as -required status checks, so passing them is a review expectation rather than a -mechanical block. +review requirement, a linear history, and two required status checks: `ci-ok` +and `quality-ok`. They fail when a Build & Test, Quality or Sanitizers job +failed or was cancelled. clang-tidy, Documentation and OPC-UA are not required. CI configuration: [`.github/workflows/ci.yml`](.github/workflows/ci.yml) and [`.github/workflows/quality.yml`](.github/workflows/quality.yml) diff --git a/scripts/ci_code_changed.sh b/scripts/ci_code_changed.sh new file mode 100755 index 000000000..88da5882a --- /dev/null +++ b/scripts/ci_code_changed.sh @@ -0,0 +1,34 @@ +#!/usr/bin/env bash +# Copyright 2026 bburda +# +# Prints `code=true` or `code=false` for $GITHUB_OUTPUT. False only on a pull +# request whose every changed file is documentation: under docs/, or a .md or +# .rst file. No build or test reads those. +# +# Runs in a checkout of the pull request merge commit, fetched with depth 2. Its +# first parent is the base branch tip, so the diff is what the merge adds. +set -euo pipefail + +if [[ "${GITHUB_EVENT_NAME:-}" != "pull_request" ]]; then + echo "code=true" + exit 0 +fi + +# On a single-parent commit HEAD^1 diffs only the last commit, not the pull request. +if ! git rev-parse --verify --quiet 'HEAD^2' >/dev/null; then + echo "::error::HEAD is not a merge commit - check out the pull request merge ref with fetch-depth 2" >&2 + exit 1 +fi + +# --no-renames lists both sides of a rename, so moving a source file under docs/ +# still counts as a code change. +changed=$(git diff --no-renames --name-only 'HEAD^1' HEAD) +code_files=$(grep -vE '^docs/|\.(md|rst)$' <<<"${changed}" || true) + +if [[ -z "${changed}" || -n "${code_files}" ]]; then + echo "code=true" + printf 'Files outside the docs-only set:\n%s\n' "${code_files:-}" >&2 +else + echo "code=false" + printf 'Docs-only change:\n%s\n' "${changed}" >&2 +fi From 031c0857493cd194f314cc81ca9c2eed3338c6a9 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 5 Oct 2026 20:34:52 +0200 Subject: [PATCH 2/4] probe: treat workflow and script changes as docs-only --- scripts/ci_code_changed.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/ci_code_changed.sh b/scripts/ci_code_changed.sh index 88da5882a..f9b4438ea 100755 --- a/scripts/ci_code_changed.sh +++ b/scripts/ci_code_changed.sh @@ -23,7 +23,7 @@ fi # --no-renames lists both sides of a rename, so moving a source file under docs/ # still counts as a code change. changed=$(git diff --no-renames --name-only 'HEAD^1' HEAD) -code_files=$(grep -vE '^docs/|\.(md|rst)$' <<<"${changed}" || true) +code_files=$(grep -vE '^(docs|\.github|scripts)/|\.(md|rst)$' <<<"${changed}" || true) if [[ -z "${changed}" || -n "${code_files}" ]]; then echo "code=true" From ac47912cc3f9ae846c3f82392b9841738102b94c Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 5 Oct 2026 20:36:29 +0200 Subject: [PATCH 3/4] probe: classifier fails --- scripts/ci_code_changed.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/ci_code_changed.sh b/scripts/ci_code_changed.sh index f9b4438ea..f143d6dd4 100755 --- a/scripts/ci_code_changed.sh +++ b/scripts/ci_code_changed.sh @@ -8,6 +8,8 @@ # Runs in a checkout of the pull request merge commit, fetched with depth 2. Its # first parent is the base branch tip, so the diff is what the merge adds. set -euo pipefail +echo "::error::probe: classifier fails on purpose" >&2 +exit 1 if [[ "${GITHUB_EVENT_NAME:-}" != "pull_request" ]]; then echo "code=true" From e85d542ffd38fe3e4b72a68dfffaf32a1e4bf697 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 5 Oct 2026 20:37:06 +0200 Subject: [PATCH 4/4] probe: real classifier, runs to be cancelled --- scripts/ci_code_changed.sh | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/scripts/ci_code_changed.sh b/scripts/ci_code_changed.sh index f143d6dd4..88da5882a 100755 --- a/scripts/ci_code_changed.sh +++ b/scripts/ci_code_changed.sh @@ -8,8 +8,6 @@ # Runs in a checkout of the pull request merge commit, fetched with depth 2. Its # first parent is the base branch tip, so the diff is what the merge adds. set -euo pipefail -echo "::error::probe: classifier fails on purpose" >&2 -exit 1 if [[ "${GITHUB_EVENT_NAME:-}" != "pull_request" ]]; then echo "code=true" @@ -25,7 +23,7 @@ fi # --no-renames lists both sides of a rename, so moving a source file under docs/ # still counts as a code change. changed=$(git diff --no-renames --name-only 'HEAD^1' HEAD) -code_files=$(grep -vE '^(docs|\.github|scripts)/|\.(md|rst)$' <<<"${changed}" || true) +code_files=$(grep -vE '^docs/|\.(md|rst)$' <<<"${changed}" || true) if [[ -z "${changed}" || -n "${code_files}" ]]; then echo "code=true"