From 723158618ee25a1bc4aad481d5e7f694f8133639 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Dinis=20Ferreira?= Date: Sat, 30 May 2026 22:58:31 +0200 Subject: [PATCH 1/6] ci: fast static analysis with early-fail lint + inline SARIF Replace the pmd/checkstyle jobs with a parallel shape that gives early, inline feedback and stops re-running analysis inside the build: - lint: compile + pmd:pmd + checkstyle:checkstyle (SARIF) + pmd:cpd-check, -T 2C, --fail-never; gates by counting the merged SARIF (+ cpd.xml grep). Fails in ~3-5 min on its own check, independent of the build. - spotbugs: compile + spotbugs:spotbugs (SARIF), -Xmx4g, own parallel lane (the slow analysis). - maven-verify: build + tests only; the redundant checkstyle/pmd/spotbugs goals are dropped (now owned by lint/spotbugs). - line-endings: unchanged. - both new lanes restore the master-produced Linux-maven-publish-* cache, restore-only, mirroring snapshot.yml's path spec and key recipe exactly (the path spec is hashed into the cache version, so the mirroring must be literal). All three emit SARIF 2.1.0, merged per tool and uploaded to Code Scanning (security-events: write) for inline annotations on the PR diff + Security tab. No custom Python annotator. Count-gate rather than the *:check goals: the check goals @Execute-fork a second analysis and cannot emit SARIF, and without the full compile classpath they false-positive on type-resolving rules. Each report goal runs once (full-reactor compile -> correct + SARIF) and the gate counts the result. Rationale + tables in docs/ci-static-analysis-design.md; measurement protocol in docs/ci-measurement-protocol.md. CPD gating is wired but inert until #1339 lowers the token threshold. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/verify.yml | 204 ++++++++++++++++++++++++++++-- docs/ci-measurement-protocol.md | 65 ++++++++++ docs/ci-static-analysis-design.md | 108 ++++++++++++++++ 3 files changed, 368 insertions(+), 9 deletions(-) create mode 100644 docs/ci-measurement-protocol.md create mode 100644 docs/ci-static-analysis-design.md diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index fbece6cf24..d792377a5b 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -1,8 +1,30 @@ name: verify on: pull_request: + +# Code Scanning needs write access to upload SARIF results for inline annotations. +permissions: + contents: read + security-events: write + jobs: - pmd: + # Fast, early-fail lint lane: PMD + Checkstyle (+ CPD). Turns red in a few + # minutes on any violation, independent of the long build below, so a stray + # PMD/Checkstyle issue is reported immediately rather than after `verify`. + # + # `compile` is in the same invocation as the analysis goals: PMD's + # type-resolving rules (e.g. InvalidLogMessageFormat on the SLF4J + # trailing-Throwable idiom) need Tycho's aux-classpath, which a fresh `mvn` + # does not inherit from a prior step's target/classes. + # + # `--fail-never` lets every module produce its report (no Maven cascade-skip), + # so the uploaded SARIF — and therefore the inline annotations — are complete. + # The trade-off: --fail-never suppresses even compile and target-resolution + # failures (mvn exits 0 on a broken build), so the gate pairs the jq count + # with a presence check — zero valid analyzer inputs fails the lane rather + # than reading as a clean pass. Compilation itself is independently gated by + # maven-verify. + lint: runs-on: ubuntu-24.04 steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 @@ -12,10 +34,111 @@ jobs: java-version: '21' - name: Set up Workspace Environment Variable run: echo "WORKSPACE=${{ github.workspace }}" >> $GITHUB_ENV - - name: PMD Check - run: mvn pmd:pmd pmd:cpd pmd:check pmd:cpd-check -f ./ddk-parent/pom.xml --batch-mode --fail-at-end - checkstyle: + - name: Restore Maven dependency cache + # Restore-only, mirroring snapshot.yml's producer cache exactly (path and + # key are hashed into the cache version — see the maven-verify step). + uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: ~/.m2/repository + key: ${{ runner.os }}-maven-publish-${{ hashFiles('**/pom.xml', '**/*.target') }} + restore-keys: ${{ runner.os }}-maven-publish- + + - name: PMD + Checkstyle reports (SARIF) + # PMD: SarifRenderer FQCN — emits pmd.sarif.json AND keeps pmd.xml. + # Checkstyle: output.format=sarif — SARIF content in checkstyle-result.xml. + # CPD is excluded here: the global -Dformat flag uses PMD's Renderer + # hierarchy and would ClassCastException CPD's CPDReportRenderer. + run: | + mvn -T 2C -f ./ddk-parent/pom.xml --batch-mode --fail-never \ + compile \ + pmd:pmd checkstyle:checkstyle \ + -Dformat=net.sourceforge.pmd.renderers.SarifRenderer \ + -Dcheckstyle.output.format=sarif + + - name: CPD report (separate invocation — no SARIF support) + # CPD has no SARIF renderer; emits cpd.xml only. Run standalone so the + # PMD -Dformat flag isn't in scope. + # NOTE: project CPD token threshold is currently very high (issue #1339), + # which effectively disables detection; re-tune once #1339 lands. + run: | + mvn -T 2C -f ./ddk-parent/pom.xml --batch-mode --fail-never \ + compile \ + pmd:cpd-check + + - name: Merge per-module SARIFs (PMD + Checkstyle) + if: always() + # Code Scanning accepts one run per category per upload; each analyzer + # writes one SARIF per module, so concatenate each analyzer's results + # into a single run under .sarif-merged/. + run: | + mkdir -p .sarif-merged + # Merge per-module SARIFs into one run. Filter to JSON-parseable files: + # the ddk-parent aggregator writes a plain-XML checkstyle-result.xml that + # would break jq, and modules with no findings may emit non-SARIF stubs. + merge() { # $1 = find-glob, $2 = output + local f valid=() + while IFS= read -r f; do + jq -e . "$f" >/dev/null 2>&1 && valid+=("$f") + done < <(find . -path "$1") + if [ ${#valid[@]} -gt 0 ]; then + # del(.ruleIndex): each result's ruleIndex points into its OWN run's + # rules array, but the merge keeps only the first run's tool — Code + # Scanning must resolve rules by ruleId string instead. + jq -s '{ + "$schema": .[0]."$schema", version: .[0].version, + runs: [{ tool: .[0].runs[0].tool, + results: [.[].runs[].results[]? | del(.ruleIndex)], + invocations: [.[].runs[].invocations[]?] }] + }' "${valid[@]}" > "$2" + fi + } + merge '*/target/pmd.sarif.json' .sarif-merged/pmd.sarif + merge '*/target/checkstyle-result.xml' .sarif-merged/checkstyle.sarif + + - name: Gate on PMD / CPD / Checkstyle violations + # merge() only writes its output when it found at least one valid input, + # so a missing merged file means that analyzer silently died (e.g. a + # plugin bump broke a renderer flag) — never a clean pass. + run: | + set -eu + for f in .sarif-merged/pmd.sarif .sarif-merged/checkstyle.sarif; do + if [ ! -s "$f" ]; then + echo "::error::No valid SARIF input produced for ${f##*/} — the analysis silently failed." + exit 1 + fi + done + if [ "$(find . -name 'cpd.xml' -path '*/target/*' | wc -l)" -eq 0 ]; then + echo "::error::No cpd.xml produced — CPD silently failed." + exit 1 + fi + sarif_total=$(jq '[.runs[].results[]?] | length' \ + .sarif-merged/pmd.sarif .sarif-merged/checkstyle.sarif 2>/dev/null \ + | awk '{s+=$1} END {print s+0}') + cpd_total=$(find . -name 'cpd.xml' -path '*/target/*' -exec grep -c '/dev/null \ + | awk -F: '{s+=$2} END {print s+0}') + echo "PMD/Checkstyle SARIF violations: $sarif_total" + echo "CPD duplications: $cpd_total" + if [ "$sarif_total" != "0" ] || [ "$cpd_total" != "0" ]; then + echo "::error::Static analysis found violations (PMD/CPD/Checkstyle)." + exit 1 + fi + + - name: Upload PMD/Checkstyle SARIF to Code Scanning + if: always() + # Annotation-only, never the gate: a fork PR gets a read-only token and + # upload-sarif 403s, which must not red an otherwise-clean lane. + continue-on-error: true + uses: github/codeql-action/upload-sarif@7fd177fa680c9881b53cdab4d346d32574c9f7f4 # v3.35.4 + with: + sarif_file: .sarif-merged + category: lint + + # SpotBugs is the slow critical-path analysis (the experiments' durable + # finding), so it runs in its own parallel lane and never delays `lint`. + spotbugs: runs-on: ubuntu-24.04 + env: + MAVEN_OPTS: -Xmx4g steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - uses: actions/setup-java@0f481fcb613427c0f801b606911222b5b6f3083a # v5 @@ -24,14 +147,79 @@ jobs: java-version: '21' - name: Set up Workspace Environment Variable run: echo "WORKSPACE=${{ github.workspace }}" >> $GITHUB_ENV - - name: Checkstyle Check - run: mvn checkstyle:checkstyle checkstyle:check -f ./ddk-parent/pom.xml --batch-mode --fail-at-end + - name: Restore Maven dependency cache + # Restore-only, mirroring snapshot.yml's producer cache exactly (path and + # key are hashed into the cache version — see the maven-verify step). + uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: ~/.m2/repository + key: ${{ runner.os }}-maven-publish-${{ hashFiles('**/pom.xml', '**/*.target') }} + restore-keys: ${{ runner.os }}-maven-publish- + + - name: SpotBugs report (SARIF) + # sarifOutput=true emits spotbugsSarif.json (also writes spotbugsXml.xml). + run: | + mvn -T 2C -f ./ddk-parent/pom.xml --batch-mode --fail-never \ + compile \ + spotbugs:spotbugs \ + -Dspotbugs.sarifOutput=true + + - name: Merge per-module SpotBugs SARIFs + if: always() + run: | + mkdir -p .sarif-merged + valid=() + while IFS= read -r f; do + jq -e . "$f" >/dev/null 2>&1 && valid+=("$f") + done < <(find . -path '*/target/spotbugsSarif.json') + if [ ${#valid[@]} -gt 0 ]; then + # del(.ruleIndex): see the lint merge — indexes are per-run, the + # merged tool keeps only the first run's rules. + jq -s '{ + "$schema": .[0]."$schema", version: .[0].version, + runs: [{ tool: .[0].runs[0].tool, + results: [.[].runs[].results[]? | del(.ruleIndex)], + invocations: [.[].runs[].invocations[]?] }] + }' "${valid[@]}" > .sarif-merged/spotbugs.sarif + fi + + - name: Gate on SpotBugs violations + # A missing merged SARIF means the analysis silently died (--fail-never + # suppresses even compile/resolution failures) — never a clean pass. + run: | + set -eu + if [ ! -s .sarif-merged/spotbugs.sarif ]; then + echo "::error::No SpotBugs SARIF produced — the analysis silently failed." + exit 1 + fi + sb_total=$(jq '[.runs[].results[]?] | length' .sarif-merged/spotbugs.sarif 2>/dev/null || echo 0) + echo "SpotBugs SARIF violations: $sb_total" + if [ "$sb_total" != "0" ]; then + echo "::error::SpotBugs found violations." + exit 1 + fi + + - name: Upload SpotBugs SARIF to Code Scanning + if: always() + # Annotation-only, never the gate: a fork PR gets a read-only token and + # upload-sarif 403s, which must not red an otherwise-clean lane. + continue-on-error: true + uses: github/codeql-action/upload-sarif@7fd177fa680c9881b53cdab4d346d32574c9f7f4 # v3.35.4 + with: + sarif_file: .sarif-merged + category: spotbugs + line-endings: runs-on: ubuntu-24.04 steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - name: Check LF line endings run: bash .github/scripts/check-line-endings.sh + + # Build + tests only. Static analysis now lives in the `lint` and `spotbugs` + # jobs, so the redundant checkstyle/pmd/spotbugs goals are dropped from here — + # this is the wall-clock long pole and no longer re-runs analysis. + # No `-T 2C`: tests are not known to pass reliably under reactor parallelism. maven-verify: runs-on: ubuntu-24.04 steps: @@ -58,9 +246,7 @@ jobs: key: ${{ runner.os }}-maven-publish-${{ hashFiles('**/pom.xml', '**/*.target') }} restore-keys: ${{ runner.os }}-maven-publish- - name: Build with Maven within a virtual X Server Environment - # Run pmd:pmd and pmd:cpd first to generate reports for all modules, then run pmd:check and pmd:cpd-check - # This ensures all violations are collected and reported before the build fails - run: xvfb-run mvn clean verify checkstyle:check pmd:pmd pmd:cpd pmd:check pmd:cpd-check spotbugs:check -f ./ddk-parent/pom.xml --batch-mode --fail-at-end + run: xvfb-run mvn clean verify -f ./ddk-parent/pom.xml --batch-mode --fail-at-end - name: Fail on missing surefire reports if: always() run: bash .github/scripts/check-surefire-reports.sh diff --git a/docs/ci-measurement-protocol.md b/docs/ci-measurement-protocol.md new file mode 100644 index 0000000000..470392f488 --- /dev/null +++ b/docs/ci-measurement-protocol.md @@ -0,0 +1,65 @@ +# CI timing-measurement protocol + +How to get **trustworthy** numbers for a CI-shape change. Written because the +earlier experiment program (2026-05) produced numbers that can't be trusted: +single samples, taken during an Eclipse p2 mirror-flaky window, against a CI +shape that has since changed. Don't repeat that. + +## What we're measuring + +- **Wall-clock** per run = the slowest job (what a developer waits for). +- **Per-job duration** = the analysis bottleneck (settles e.g. spotbugs-vs-maven-verify). +- **Failure latency** = time-to-red on a *planted* lint violation — the metric the + early-fail goal actually cares about. Measure it separately. + +Wall-clock is the headline, but per-job + failure-latency are what tell you *why*. + +## Noise floor (observed on this repo's `verify` runs) + +Per-job spread across recent runs — any change must beat this to be real signal: + +| Job | median | spread (±) | +|---|---|---| +| line-endings | ~5s | ±3s | +| checkstyle (old shape) | ~115s | ±~104s (≈90%!) | +| pmd (old shape) | ~191s | ±63s | +| maven-verify | ~869s (~14.5m) | ±~93s (≈11%) | + +**Decision rule:** accept candidate B as faster than A only if +`median(B) < median(A) − 2 × IQR(A)`. For maven-verify a real win must exceed ~100s; +for checkstyle, ~200s. Anything smaller is noise. + +## Protocol + +1. **Pre-flight — confirm mirrors are healthy.** Run one throwaway build; if + target-platform resolution stalls or errors, **stop** — the window is bad + (this is what voided the 2026-05-09 numbers). Measure only on a clean window. +2. **Warm the caches.** Run 3–5 discard builds first so `~/.m2` + `.cache/tycho` + are populated; cold-cache runs have a different (network-bound) profile. +3. **Sample.** N = 15–20 `workflow_dispatch` runs per candidate, back-to-back in a + ≤30-minute window. Run A and B **interleaved within ≤10 minutes** of each other + so they see the same mirror weather and runner-pool load. +4. **Report medians + IQR**, per job *and* total wall-clock. Never a single sample, + never the mean. +5. **Pin `ubuntu-24.04`** (not a matrix) for consistent runner hardware. +6. **Record per-sample metadata:** cache hit/miss, run start time, headSha. +7. **Failure latency:** plant a synthetic PMD/Checkstyle violation, measure how long + until the `lint` check goes red (independent of the build job). + +## Why not just trust the old experiment numbers + +- Single sample each (no repetition). +- 2026-05-09 mirror-flaky window → resolution time is contaminated and varies per job + even within one dispatch. +- Numbers disagree across rounds (sequential measured 21m one round, 33m another). +- `#1369` reshaped master's CI after the experiments ran, so their baseline is stale. + +## What is *not* a timing lever (validated) + +- **Local p2 mirror**: with a warm `~/.m2`/`.cache/tycho`, offline resolution is + ~equal to online (measured 5s vs 6s) — a mirror's steady-state speedup is ~0. + Its only value is cold-cache + flaky-mirror insurance; deferred per the rare-flake + rule. +- **`-Dtycho.mode=maven` on a gate pass**: marginal on a warm cache (resolution is + already seconds); and it *breaks* PMD type-resolving rules (strips the classpath → + false positives). Not used. diff --git a/docs/ci-static-analysis-design.md b/docs/ci-static-analysis-design.md new file mode 100644 index 0000000000..6cef71781d --- /dev/null +++ b/docs/ci-static-analysis-design.md @@ -0,0 +1,108 @@ +# Static-analysis CI design: SARIF inline display + count-gate + +This documents *why* the static-analysis CI is shaped the way it is, so the +mechanics (verified against the plugin source) don't have to be re-discovered. + +## Goal + +Fast, complete, **early** PMD + Checkstyle feedback — a violation should fail CI +in minutes, on its own check, not after the ~15-minute build. SpotBugs (the slow +analysis) runs in its own parallel lane so it never delays the fast checks. All +of PMD, Checkstyle, and SpotBugs surface **inline** on the PR via SARIF + GitHub +Code Scanning (no custom annotator). + +## Job shape (`verify.yml`) + +Four independent parallel jobs (no `needs:`); wall-clock = the slowest job. + +| Job | Runs | Threads | Gate | +|---|---|---|---| +| `lint` | `compile` + `pmd:pmd` + `checkstyle:checkstyle` (SARIF) + `pmd:cpd-check` | `-T 2C` | jq count of SARIF results (+ CPD `` grep) | +| `spotbugs` | `compile` + `spotbugs:spotbugs` (SARIF), `-Xmx4g` | `-T 2C` | jq count of SARIF results | +| `maven-verify` | `clean verify` (build + tests only) | none | Tycho-surefire | +| `line-endings` | `git ls-files` check | n/a | shell | + +`maven-verify` **no longer re-runs** the analysis goals (now owned by `lint` / +`spotbugs`). Analysis jobs use `-T 2C`; the test job does not (tests are not known +reliable under reactor parallelism). + +## Report goals vs. check goals (code-verified at each plugin version tag) + +| Goal | Kind | `@Execute` fork | Runs analysis? | Writes | Fails build? | SARIF-capable? | +|---|---|---|---|---|---|---| +| `pmd:pmd` | report | — | yes | `pmd.xml` (+ `pmd.sarif.json` w/ `-Dformat=…SarifRenderer`) | no | **yes** | +| `pmd:check` | check | `@Execute(goal=pmd)` | yes (forked) | `pmd.xml` | yes (`> maxAllowedViolations`, dflt 0; `failurePriority` dflt 5 = all) | no (wants `pmd.xml`) | +| `pmd:cpd` | report | — | yes | `cpd.xml` | no | no (no CPD SARIF renderer) | +| `pmd:cpd-check` | check | `@Execute(goal=cpd)` | yes (forked) | `cpd.xml` | yes | no | +| `checkstyle:checkstyle` | report | — | yes | `checkstyle-result.xml` (XML, or SARIF w/ `-Dcheckstyle.output.format=sarif`) | no | **yes** | +| `checkstyle:check` | check | **none** (self-contained) | yes (internal, source-based) | `checkstyle-result.xml` (XML) | yes (severity ≥ `violationSeverity`) | no | +| `spotbugs:spotbugs` | report | — | yes (bytecode) | `spotbugsXml.xml` (+ `spotbugsSarif.json` w/ `-Dspotbugs.sarifOutput=true`) | no | **yes** | +| `spotbugs:check` | check | `@Execute(goal=spotbugs)` | yes (forked) | `spotbugsXml.xml` | yes (`bugCount > 0`) | no | + +The split is **observe vs. enforce**: report goals produce output (for the Maven +site / dashboards / SARIF consumers) and never fail the build; check goals bind to +`verify` and exist to fail the build. The `@Execute(goal=…)` on the PMD/CPD/SpotBugs +checks forks the report goal first (so `mvn pmd:check` is self-contained) — which +means they **re-run the analysis every time**. `checkstyle:check` is the lone +exception: no `@Execute`, runs Checkstyle internally in one pass. + +## Maven reactor failure flags + +| Flag | Reactor behavior | Multi-module report completeness | Suppresses violation failure? | +|---|---|---|---| +| `--fail-fast` (default) | halt at first failing module | downstream skipped → reports missing | no | +| `--fail-at-end` | build independent modules, fail at end | modules downstream of a failure still cascade-skipped → incomplete | no | +| `--fail-never` | never fail the build — verified empirically: even compile and target-resolution failures leave `mvn` exit 0 ("Build failures were ignored") | **all** modules run → **complete** reports/SARIF | **yes** — swallowed entirely; the gate's presence checks compensate, and `maven-verify` independently gates compilation | + +## Why `report goal + --fail-never + jq count` (the count-gate) + +We want **both** SARIF (for inline display) **and** a build gate, from **one** +analysis pass. The plugin design forces a choice: + +| Combination | Result | +|---|---| +| check goal + `--fail-fast` | gates, but first violation hides later modules' findings | +| check goal + `--fail-at-end` | gates, but cascade-skip drops downstream findings | +| check goal + `--fail-never` | doesn't gate (failure swallowed) — pointless | +| **report goal + `--fail-never` + jq count** | **all** modules analyzed → complete SARIF (full inline display); the jq count does the gating | + +So each tool runs its report goal **once** (`compile` + report, full classpath → +correct + SARIF), and a jq count over the merged SARIF fails the job. + +### Why not the `:check` goals +- They `@Execute`-fork a **second** analysis on top of the `pmd:pmd` we already run + for SARIF — pure waste. +- The forked analysis is only correct with the full `compile` + classpath; run cheaply + (`-Dtycho.mode=maven` / no compile) it loses the classpath and **false-positives** + (e.g. PMD `InvalidLogMessageFormat` flags the SLF4J trailing-`Throwable` idiom because + it can't resolve the last arg as a `Throwable`). +- `pmd:check` is **incompatible with `-Dformat=sarif`**: its fork writes `pmd.sarif.json`, + but the check looks for `pmd.xml` → "unable to find report." + +### count-gate == `:check` fidelity (for this project's config) +Counting **all** SARIF results equals what `:check` would fail on, because: PMD has no +`failurePriority` override (default 5 = all priorities), Checkstyle is globally +`severity=warning` with no info-level results, and SpotBugs uses `threshold=Low` with no +`failThreshold`/`maxAllowedViolations` override. Three config-drift caveats would make +the count-gate *stricter* than `:check` (over-fail, never under-fail), each guard-worthy: +1. PMD `pmd.failurePriority` set below 5. +2. A Checkstyle rule emitting `info`/`note` severity. +3. SpotBugs `failThreshold` or non-zero `maxAllowedViolations`. + +## Two operational rules + +- **`compile` must be full-reactor** (`-f ddk-parent/pom.xml`, no `-pl`). PMD's + type-resolving rules need the complete aux-classpath; a `-pl` subset produces + false positives (the trailing-`Throwable` case). +- **Merge SARIFs from SARIF files only.** Code Scanning accepts one run per category, + so per-module SARIFs are merged (jq) before upload. The `ddk-parent` aggregator emits + plain-XML `checkstyle-result.xml`; the merge must filter to JSON-parseable files. + +## SARIF mechanics + +- PMD: `-Dformat=net.sourceforge.pmd.renderers.SarifRenderer` → `pmd.sarif.json`. +- Checkstyle: `-Dcheckstyle.output.format=sarif` → SARIF content in `checkstyle-result.xml`. +- SpotBugs: `-Dspotbugs.sarifOutput=true` → `spotbugsSarif.json`. +- All emit SARIF 2.1.0. Merge per tool → `github/codeql-action/upload-sarif` + (`permissions: security-events: write`). CPD has no SARIF renderer → XML + grep gate + (no inline display for CPD; acceptable, duplications are rare). From c501946bcb7eb80427f8f8a4ae55932f7dea585b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Dinis=20Ferreira?= Date: Sun, 31 May 2026 12:02:13 +0200 Subject: [PATCH 2/6] ci: scope SpotBugs to the PR's changed modules (per-module skip) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SpotBugs' per-module analysis is the spotbugs job's long pole. A PR only needs its changed modules scanned, so a pre-step injects true> into every unchanged reactor module's pom — the plugin then skips the goal, and the per-module JVM fork, for them. The full-reactor compile is kept (a changed module keeps its complete aux-classpath); a build/config change falls back to a full scan. pull_request only — master/snapshot run a full scan. -Dspotbugs.onlyAnalyze was the cleaner-looking alternative but screens too late (after the per-module fork), ~17% vs ~88% measured; the script header documents the migration if an upstream SpotBugs early-exit ever lands. - .github/scripts/compute-spotbugs-skip.sh: diff -> changed modules -> inject skip into the unchanged ones (idempotent; build/config change -> full scan). - verify.yml spotbugs job: fetch-depth 0 + a scope step before compile; -Djgit.dirtyWorkingTree=ignore because the scope step dirties poms on purpose and this job releases nothing (releases/maven-verify keep =error); SARIF upload guarded so an empty scan set (no module scanned) doesn't fail the upload. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/scripts/compute-spotbugs-skip.sh | 89 ++++++++++++++++++++++++ .github/workflows/verify.yml | 26 ++++++- 2 files changed, 113 insertions(+), 2 deletions(-) create mode 100755 .github/scripts/compute-spotbugs-skip.sh diff --git a/.github/scripts/compute-spotbugs-skip.sh b/.github/scripts/compute-spotbugs-skip.sh new file mode 100755 index 0000000000..6be5adaa1a --- /dev/null +++ b/.github/scripts/compute-spotbugs-skip.sh @@ -0,0 +1,89 @@ +#!/usr/bin/env bash +# +# Scope SpotBugs to a pull request's changed modules. +# +# Default is RUN (analyze). On a PR this injects true +# into every UNCHANGED reactor module's pom, so spotbugs-maven-plugin skips the goal — +# and therefore the per-module JVM fork (SpotBugsMojo gates on `skip` before forking) — +# for those modules. The full-reactor compile is left intact (a changed module is still +# analysed with its complete aux-classpath). Master/snapshot builds run a full scan; +# this script is invoked on pull_request only. +# +# Why this and not -Dspotbugs.onlyAnalyze: onlyAnalyze is one clean flag, but SpotBugs +# applies its class screener too late (after the per-module fork + class scan), so it +# only trimmed ~17% of the goal vs ~88% for this per-module skip (measured on this +# reactor). A small upstream SpotBugs early-exit (skip the run when no application class +# matches the screener) would make onlyAnalyze competitive; if that ever lands, switch +# to onlyAnalyze and delete this script. +# +# Run from the repository root. Usage: compute-spotbugs-skip.sh +set -euo pipefail +base="${1:?base sha required}" + +changed=$(git diff --name-only --diff-filter=ACMR "${base}...HEAD") + +# 1) A change to shared build/config can affect any module -> full scan (skip nothing). +# ddk-configuration holds the analyzers' rulesets and filters (e.g. the SpotBugs +# exclusion-filter), so a change there must re-scan everything, not skip silently. +# ddk-target defines the target platform every module resolves against. +# Fail safe: the worst case here is "analyse everything", never "analyse nothing". +while IFS= read -r f; do + [ -n "$f" ] || continue + case "$f" in + pom.xml | ddk-parent/* | .mvn/* | *.target | ddk-target/* | .github/* | ddk-configuration/* | *[Ss]pot[Bb]ugs*[Ee]xclude*) + echo "Build/config change ($f) -> full SpotBugs scan (no skips)." + exit 0 + ;; + esac +done < (strip the leading ../). +# ddk-parent is NOT in its own , so it can never be skip-injected — which +# is what prevents an accidental inherited (global) skip. +module_dirs=$(grep -oE '\.\./[^<]+' ddk-parent/pom.xml \ + | sed -E 's#.*\.\./([^<]+)#\1#') + +# 4) Idempotently inject the skip property; handle poms with and without . +# sed -i.bak + rm is portable across GNU (CI) and BSD (local) sed. +inject_skip() { + local pom="$1/pom.xml" + [ -f "$pom" ] || return 0 + if grep -q '' "$pom"; then return 0; fi + if grep -q '' "$pom"; then + sed -i.bak 's##\n true#' "$pom" + else + sed -i.bak 's## \n true\n \n#' "$pom" + fi + rm -f "$pom.bak" +} + +# 5) Skip every reactor module that was not touched by this PR. +kept=0 +skipped=0 +while IFS= read -r mod; do + [ -n "$mod" ] || continue + if printf '%s\n' "${changed_mods}" | grep -qx "$mod"; then + kept=$((kept + 1)) + else + inject_skip "$mod" + skipped=$((skipped + 1)) + fi +done <> "$GITHUB_ENV" +fi + +echo "SpotBugs scope: scanning ${kept} changed module(s), skipping ${skipped} unchanged." +echo "Changed modules: ${changed_mods:-}" diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index d792377a5b..a0eb8de695 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -141,6 +141,8 @@ jobs: MAVEN_OPTS: -Xmx4g steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + fetch-depth: 0 # need the PR base commit to diff the changed modules - uses: actions/setup-java@0f481fcb613427c0f801b606911222b5b6f3083a # v5 with: distribution: 'temurin' @@ -156,13 +158,25 @@ jobs: key: ${{ runner.os }}-maven-publish-${{ hashFiles('**/pom.xml', '**/*.target') }} restore-keys: ${{ runner.os }}-maven-publish- + - name: Scope SpotBugs to the PR's changed modules + # Injects true> into unchanged module poms so the per-module + # SpotBugs fork is skipped for them (the lever that actually scopes the cost). + # Full compile is preserved (correct aux-classpath); a build/config change -> + # full scan. pull_request only; master/snapshot run a full scan. + run: bash .github/scripts/compute-spotbugs-skip.sh "${{ github.event.pull_request.base.sha }}" + - name: SpotBugs report (SARIF) # sarifOutput=true emits spotbugsSarif.json (also writes spotbugsXml.xml). + # jgit.dirtyWorkingTree=ignore: the scope step intentionally edits poms, so the + # working tree is dirty here; this job releases nothing, so we tell Tycho's jgit + # build-qualifier to use the last commit's timestamp instead of failing (the + # repo keeps jgit.dirtyWorkingTree=error for maven-verify / releases). run: | mvn -T 2C -f ./ddk-parent/pom.xml --batch-mode --fail-never \ compile \ spotbugs:spotbugs \ - -Dspotbugs.sarifOutput=true + -Dspotbugs.sarifOutput=true \ + -Djgit.dirtyWorkingTree=ignore - name: Merge per-module SpotBugs SARIFs if: always() @@ -186,8 +200,14 @@ jobs: - name: Gate on SpotBugs violations # A missing merged SARIF means the analysis silently died (--fail-never # suppresses even compile/resolution failures) — never a clean pass. + # Exception: the scope step skip-injected every module (no reactor module + # changed, e.g. a docs-only PR), where zero reports is the expected state. run: | set -eu + if [ "${SPOTBUGS_KEPT:-}" = "0" ]; then + echo "All modules skip-injected (no reactor module changed) — nothing to scan." + exit 0 + fi if [ ! -s .sarif-merged/spotbugs.sarif ]; then echo "::error::No SpotBugs SARIF produced — the analysis silently failed." exit 1 @@ -200,7 +220,9 @@ jobs: fi - name: Upload SpotBugs SARIF to Code Scanning - if: always() + # Skip when nothing was scanned (e.g. a docs-only PR -> all modules skipped): + # an empty .sarif-merged would otherwise fail upload-sarif ("No SARIF files"). + if: ${{ always() && hashFiles('.sarif-merged/spotbugs.sarif') != '' }} # Annotation-only, never the gate: a fork PR gets a read-only token and # upload-sarif 403s, which must not red an otherwise-clean lane. continue-on-error: true From 813bb2bab38a849e9b3312f9533696097e2369aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Dinis=20Ferreira?= Date: Fri, 10 Jul 2026 16:18:31 +0200 Subject: [PATCH 3/6] ci: run SpotBugs in-process in the verify spotbugs lane spotbugs-maven-plugin forks a fresh JVM per module by default (maxHeap 2048), so a full scan pays 59 JVM startups. -Dspotbugs.fork=false runs the analysis inside the Maven JVM; the lane's MAVEN_OPTS -Xmx4g governs the shared heap (the plugin's maxHeap setting applies to forks only). Measured locally (full scan, clean tree, JDK 21): 75s vs 95s wall at -T 3C, and stable at CI thread width (-T 8, 4 GB heap) with identical findings (59 module SARIFs, same result count). Co-Authored-By: Claude Fable 5 --- .github/workflows/verify.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index a0eb8de695..e62bcd96a4 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -171,11 +171,15 @@ jobs: # working tree is dirty here; this job releases nothing, so we tell Tycho's jgit # build-qualifier to use the last commit's timestamp instead of failing (the # repo keeps jgit.dirtyWorkingTree=error for maven-verify / releases). + # spotbugs.fork=false analyses in the Maven JVM instead of forking a fresh 2 GB + # JVM per module (59 forks); the shared heap is governed by MAVEN_OPTS above + # (the plugin's maxHeap applies to forks only). run: | mvn -T 2C -f ./ddk-parent/pom.xml --batch-mode --fail-never \ compile \ spotbugs:spotbugs \ -Dspotbugs.sarifOutput=true \ + -Dspotbugs.fork=false \ -Djgit.dirtyWorkingTree=ignore - name: Merge per-module SpotBugs SARIFs From 802f66914e0373bd3ec41942d631c5ca74e381e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Dinis=20Ferreira?= Date: Sat, 11 Jul 2026 16:23:32 +0200 Subject: [PATCH 4/6] ci: build only changed modules + upstream deps in the spotbugs lane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit compute-spotbugs-skip.sh now also exports SPOTBUGS_SCOPE_ARGS ("-pl ../ddk-target, -am") and the spotbugs lane passes it to mvn, so a scoped run builds only the PR's changed modules plus their upstream dependencies instead of the full 64-module reactor. The -am-pulled unchanged dependencies keep the injected spotbugs.skip: they compile (complete aux-classpath) but are not analysed. ddk-target is pinned into every scoped reactor: the target-definition artifact is referenced by target-platform-configuration, not by any bundle MANIFEST, so -am alone never pulls it — Tycho then falls back to a local-repository copy of the .target, which fails on a cold cache and can silently resolve a stale target definition on a warm one (verified: a June install of the same sequenceNumber still pointed at the 2026-03 release train while the tree's points at 2026-06). The gate verifies that every scanned source-bearing module produced its SARIF (and that a full scan produced any at all): --fail-never swallows even target-resolution failures, so without a presence check a dead build uploads nothing, counts zero violations, and passes vacuously. Fail-safes are unchanged: a build/config change means a full reactor and full scan, and a PR touching no reactor module builds the full reactor with every analysis skipped. Measured locally (single-module change, clean tree, JDK 21): 34s wall vs 75s for the full-reactor equivalent, resolving the current target platform (jdt.core 3.46.0) with findings identical to the full scan. Verified against a repository with no installed ddk-target: without the pin the run reproduces the swallowed resolution failure with zero SARIFs; with it the scoped run succeeds. Co-Authored-By: Claude Fable 5 --- .github/scripts/compute-spotbugs-skip.sh | 87 +++++++++++++++++++++--- .github/workflows/verify.yml | 23 +++++-- docs/ci-static-analysis-design.md | 11 ++- 3 files changed, 102 insertions(+), 19 deletions(-) diff --git a/.github/scripts/compute-spotbugs-skip.sh b/.github/scripts/compute-spotbugs-skip.sh index 6be5adaa1a..96f7f7906c 100755 --- a/.github/scripts/compute-spotbugs-skip.sh +++ b/.github/scripts/compute-spotbugs-skip.sh @@ -14,7 +14,12 @@ # only trimmed ~17% of the goal vs ~88% for this per-module skip (measured on this # reactor). A small upstream SpotBugs early-exit (skip the run when no application class # matches the screener) would make onlyAnalyze competitive; if that ever lands, switch -# to onlyAnalyze and delete this script. +# to onlyAnalyze and delete this script (tracked in #1455 / spotbugs/spotbugs#3796). +# +# On top of the skips, the changed reactor modules are exported as SPOTBUGS_SCOPE_ARGS +# ("-pl -am") so the lane builds only those modules plus their upstream +# dependencies instead of the full reactor. The -am-pulled unchanged dependencies still +# carry the injected skip: they compile (complete aux-classpath) but are not analysed. # # Run from the repository root. Usage: compute-spotbugs-skip.sh set -euo pipefail @@ -22,6 +27,23 @@ base="${1:?base sha required}" changed=$(git diff --name-only --diff-filter=ACMR "${base}...HEAD") +# Reactor module dirs from ddk-parent's (strip the leading ../), and the +# subset that bears sources. ddk-parent is NOT in its own , so it can never be +# skip-injected — which prevents an accidental inherited (global) skip. Both lists are +# computed up front because the full-scan early-exit below needs the source-bearing set +# for its report-presence gate. +module_dirs=$(grep -oE '\.\./[^<]+' ddk-parent/pom.xml \ + | sed -E 's#.*\.\./([^<]+)#\1#') +all_source_modules="" +while IFS= read -r mod; do + [ -n "$mod" ] || continue + if [ -f "${mod}/META-INF/MANIFEST.MF" ] && [ -d "${mod}/src" ]; then + all_source_modules="${all_source_modules:+${all_source_modules} }${mod}" + fi +done < full scan (skip nothing). # ddk-configuration holds the analyzers' rulesets and filters (e.g. the SpotBugs # exclusion-filter), so a change there must re-scan everything, not skip silently. @@ -31,6 +53,16 @@ while IFS= read -r f; do [ -n "$f" ] || continue case "$f" in pom.xml | ddk-parent/* | .mvn/* | *.target | ddk-target/* | .github/* | ddk-configuration/* | *[Ss]pot[Bb]ugs*[Ee]xclude*) + # KEPT must be set on every path: in a workflow `if:` an unset env var is + # null, which coerces to 0 and compares EQUAL to '0' — wrongly skipping + # the build. "all" marks the full-scan case (only "0" relaxes the gate). + # EXPECT_REPORTS lists every source-bearing module so the gate verifies a + # full scan analysed all of them (not just >=1) — a mojo death swallowed by + # --fail-never can't pass as long as one sibling reported. + if [ -n "${GITHUB_ENV:-}" ]; then + echo "SPOTBUGS_KEPT=all" >> "$GITHUB_ENV" + echo "SPOTBUGS_EXPECT_REPORTS=${all_source_modules}" >> "$GITHUB_ENV" + fi echo "Build/config change ($f) -> full SpotBugs scan (no skips)." exit 0 ;; @@ -44,13 +76,7 @@ EOF # grep's no-match exit would otherwise kill the script under pipefail. changed_mods=$(printf '%s\n' "${changed}" | { grep '/' || true; } | cut -d/ -f1 | sort -u) -# 3) Reactor module dirs from ddk-parent's (strip the leading ../). -# ddk-parent is NOT in its own , so it can never be skip-injected — which -# is what prevents an accidental inherited (global) skip. -module_dirs=$(grep -oE '\.\./[^<]+' ddk-parent/pom.xml \ - | sed -E 's#.*\.\./([^<]+)#\1#') - -# 4) Idempotently inject the skip property; handle poms with and without . +# 3) Idempotently inject the skip property; handle poms with and without . # sed -i.bak + rm is portable across GNU (CI) and BSD (local) sed. inject_skip() { local pom="$1/pom.xml" @@ -64,13 +90,23 @@ inject_skip() { rm -f "$pom.bak" } -# 5) Skip every reactor module that was not touched by this PR. +# 4) Skip every reactor module that was not touched by this PR. Kept modules with a +# bundle MANIFEST are expected to produce an analysis report — the gate checks this +# so a swallowed resolution/compile failure can never pass as "nothing to scan". kept=0 skipped=0 +kept_pl="" +expect_reports="" while IFS= read -r mod; do [ -n "$mod" ] || continue if printf '%s\n' "${changed_mods}" | grep -qx "$mod"; then kept=$((kept + 1)) + kept_pl="${kept_pl:+${kept_pl},}../${mod}" + # Only bundles with sources reliably emit a report (a source-less bundle, + # e.g. pure branding, has nothing for the analyzer to write a SARIF about). + if [ -f "${mod}/META-INF/MANIFEST.MF" ] && [ -d "${mod}/src" ]; then + expect_reports="${expect_reports:+${expect_reports} }${mod}" + fi else inject_skip "$mod" skipped=$((skipped + 1)) @@ -81,9 +117,38 @@ EOF # The gate's presence check needs to distinguish "all modules skip-injected" # (zero reports is the expected state) from "the analysis silently died". +# If the only changed modules are source-less (feature / target / repository — nothing +# any analyzer can report), fold into the docs-only no-op: export KEPT=0 so the lane +# skips the Maven step, gate, and upload instead of failing the merged-report presence +# check on output that could never exist. +if [ "$kept" -eq 0 ] || [ -z "$expect_reports" ]; then + effective_kept=0 +else + effective_kept=$kept +fi if [ -n "${GITHUB_ENV:-}" ]; then - echo "SPOTBUGS_KEPT=${kept}" >> "$GITHUB_ENV" + echo "SPOTBUGS_KEPT=${effective_kept}" >> "$GITHUB_ENV" fi -echo "SpotBugs scope: scanning ${kept} changed module(s), skipping ${skipped} unchanged." +# 5) Scope the reactor to the changed modules + their upstream dependencies. ddk-target +# is always kept in the -pl list: the target-definition artifact is referenced by +# target-platform-configuration, not by any MANIFEST, so -am never pulls it — without +# it in the reactor Tycho falls back to a local-repository copy, which fails on a +# cold cache and can silently resolve a stale target definition on a warm one. +# With no analysable changed module (docs-only, or source-less-only) no scope args +# are exported; the workflow skips the Maven step entirely on SPOTBUGS_KEPT=0. +if [ "$effective_kept" -gt 0 ] && [ -n "${GITHUB_ENV:-}" ]; then + echo "SPOTBUGS_SCOPE_ARGS=-pl ../ddk-target,${kept_pl} -am" >> "$GITHUB_ENV" + echo "SPOTBUGS_EXPECT_REPORTS=${expect_reports}" >> "$GITHUB_ENV" +fi + +echo "SpotBugs scope: scanning ${effective_kept} changed module(s), skipping ${skipped} unchanged." echo "Changed modules: ${changed_mods:-}" +if [ "$kept" -gt 0 ] && [ "$effective_kept" -eq 0 ]; then + echo "Only source-less modules changed (no analysable sources) -> no-op (KEPT=0)." +fi +if [ "$effective_kept" -gt 0 ]; then + echo "Reactor scope args: -pl ../ddk-target,${kept_pl} -am" +else + echo "Reactor scope args: " +fi diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index e62bcd96a4..eebf7eab2c 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -159,10 +159,11 @@ jobs: restore-keys: ${{ runner.os }}-maven-publish- - name: Scope SpotBugs to the PR's changed modules - # Injects true> into unchanged module poms so the per-module - # SpotBugs fork is skipped for them (the lever that actually scopes the cost). - # Full compile is preserved (correct aux-classpath); a build/config change -> - # full scan. pull_request only; master/snapshot run a full scan. + # Injects true> into unchanged module poms so their analysis is + # skipped, and exports SPOTBUGS_SCOPE_ARGS (-pl -am) so only the + # changed modules and their upstream deps build at all (skip-injected deps + # compile for the aux-classpath but are not analysed). A build/config change -> + # full scan, full reactor. pull_request only; master/snapshot run a full scan. run: bash .github/scripts/compute-spotbugs-skip.sh "${{ github.event.pull_request.base.sha }}" - name: SpotBugs report (SARIF) @@ -174,8 +175,12 @@ jobs: # spotbugs.fork=false analyses in the Maven JVM instead of forking a fresh 2 GB # JVM per module (59 forks); the shared heap is governed by MAVEN_OPTS above # (the plugin's maxHeap applies to forks only). + # Skipped entirely when the scope step kept no modules (e.g. a docs-only + # PR): every module would carry spotbugs.skip, so the compile output + # would be unused. The gate below relaxes on the same condition. + if: env.SPOTBUGS_KEPT != '0' run: | - mvn -T 2C -f ./ddk-parent/pom.xml --batch-mode --fail-never \ + mvn -T 2C -f ./ddk-parent/pom.xml ${SPOTBUGS_SCOPE_ARGS:-} --batch-mode --fail-never \ compile \ spotbugs:spotbugs \ -Dspotbugs.sarifOutput=true \ @@ -206,12 +211,20 @@ jobs: # suppresses even compile/resolution failures) — never a clean pass. # Exception: the scope step skip-injected every module (no reactor module # changed, e.g. a docs-only PR), where zero reports is the expected state. + # Each scanned source-bearing module must additionally have produced its + # own SARIF, so a partially-dead scoped build cannot hide either. run: | set -eu if [ "${SPOTBUGS_KEPT:-}" = "0" ]; then echo "All modules skip-injected (no reactor module changed) — nothing to scan." exit 0 fi + for mod in ${SPOTBUGS_EXPECT_REPORTS:-}; do + if [ ! -s "${mod}/target/spotbugsSarif.json" ]; then + echo "::error::${mod} was scanned but produced no SpotBugs SARIF — a build failure was swallowed by --fail-never." + exit 1 + fi + done if [ ! -s .sarif-merged/spotbugs.sarif ]; then echo "::error::No SpotBugs SARIF produced — the analysis silently failed." exit 1 diff --git a/docs/ci-static-analysis-design.md b/docs/ci-static-analysis-design.md index 6cef71781d..dc5f970ebc 100644 --- a/docs/ci-static-analysis-design.md +++ b/docs/ci-static-analysis-design.md @@ -91,9 +91,14 @@ the count-gate *stricter* than `:check` (over-fail, never under-fail), each guar ## Two operational rules -- **`compile` must be full-reactor** (`-f ddk-parent/pom.xml`, no `-pl`). PMD's - type-resolving rules need the complete aux-classpath; a `-pl` subset produces - false positives (the trailing-`Throwable` case). +- **A changed module must compile against its complete aux-classpath**, or PMD's + type-resolving rules false-positive (the trailing-`Throwable` case). A bare `-pl ` + subset breaks this, but `-pl -am` does not: `--also-make` restores the module's + full **Maven** dependency closure, which compiles for the classpath even though those deps + carry the analysis skip. Caveat: OSGi `Require-Bundle` siblings are *not* Maven dependencies, + so `-am` never pulls them — they resolve from the restored `~/.m2` p2 cache; `ddk-target` is + the one edge with no MANIFEST reference at all, so it is pinned explicitly into every scoped + reactor (a cold cache without it fails loudly rather than resolving a stale target). - **Merge SARIFs from SARIF files only.** Code Scanning accepts one run per category, so per-module SARIFs are merged (jq) before upload. The `ddk-parent` aggregator emits plain-XML `checkstyle-result.xml`; the merge must filter to JSON-parseable files. From 6f8b3e6d09253261bfea5af96e5cbe278b805a5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Dinis=20Ferreira?= Date: Fri, 10 Jul 2026 23:42:55 +0200 Subject: [PATCH 5/6] ci: run CPD without a compile pass in the lint lane CPD tokenizes sources under src/ and needs neither bytecode nor a resolved target platform, so the second lint invocation drops its compile goals. cpd.xml outputs are identical with and without the compile pass, verified at the current token threshold and at the PMD default of 100 (timestamp attributes aside). Measured locally (warm tree, JDK 21): 6.8s vs 44.3s at the current threshold; 4.5s vs 28.5s at threshold 100. In CI the invocation was 53s, ~40s of it redundant recompilation and JVM startup. Co-Authored-By: Claude Fable 5 --- .github/workflows/verify.yml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index eebf7eab2c..727f044b86 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -58,11 +58,13 @@ jobs: - name: CPD report (separate invocation — no SARIF support) # CPD has no SARIF renderer; emits cpd.xml only. Run standalone so the # PMD -Dformat flag isn't in scope. - # NOTE: project CPD token threshold is currently very high (issue #1339), - # which effectively disables detection; re-tune once #1339 lands. + # No `compile`: CPD is token-based over src/ and needs neither bytecode + # nor the target platform — outputs are identical with and without a + # compile pass (verified at the current token threshold and at 100). + # NOTE: the CPD token threshold is governed by pmd.cpd.min in + # ddk-parent/pom.xml (#1339; tuned to 100 by #1397). run: | mvn -T 2C -f ./ddk-parent/pom.xml --batch-mode --fail-never \ - compile \ pmd:cpd-check - name: Merge per-module SARIFs (PMD + Checkstyle) From f08c43c2e155301ed4a309ddd35ef855cc6c9826 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Dinis=20Ferreira?= Date: Fri, 10 Jul 2026 23:48:30 +0200 Subject: [PATCH 6/6] ci: scope PMD/CPD/Checkstyle to changed modules in the lint lane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit compute-spotbugs-skip.sh becomes compute-analysis-skip.sh with a mode argument: `spotbugs` injects spotbugs.skip as before, `lint` injects pmd.skip, cpd.skip and checkstyle.skip, and each mode exports its -pl/-am reactor scope args. The lint lane gains the scope step and passes LINT_SCOPE_ARGS to both invocations; `compile` stays in the PMD/Checkstyle invocation because PMD's type-resolving rules need Tycho's aux-classpath (skip-injected -am dependencies compile but are not analysed). Changes under ddk-configuration (rulesets, filters) now also trigger the full-scan fail-safe in both lanes. Code Scanning note: repo-wide alert state reflects the default branch, which receives no lint/spotbugs analyses (verify runs on pull_request only), so a scoped upload that omits unchanged modules can only affect PR-context annotations — the same property the spotbugs category has had since the per-module skip landed. Co-Authored-By: Claude Fable 5 --- ...tbugs-skip.sh => compute-analysis-skip.sh} | 92 +++++++++++-------- .github/workflows/verify.yml | 56 +++++++++-- 2 files changed, 102 insertions(+), 46 deletions(-) rename .github/scripts/{compute-spotbugs-skip.sh => compute-analysis-skip.sh} (67%) diff --git a/.github/scripts/compute-spotbugs-skip.sh b/.github/scripts/compute-analysis-skip.sh similarity index 67% rename from .github/scripts/compute-spotbugs-skip.sh rename to .github/scripts/compute-analysis-skip.sh index 96f7f7906c..d645ee9a83 100755 --- a/.github/scripts/compute-spotbugs-skip.sh +++ b/.github/scripts/compute-analysis-skip.sh @@ -1,29 +1,40 @@ #!/usr/bin/env bash # -# Scope SpotBugs to a pull request's changed modules. +# Scope static analysis (SpotBugs, or PMD/CPD/Checkstyle) to a pull request's +# changed modules. # -# Default is RUN (analyze). On a PR this injects true -# into every UNCHANGED reactor module's pom, so spotbugs-maven-plugin skips the goal — -# and therefore the per-module JVM fork (SpotBugsMojo gates on `skip` before forking) — -# for those modules. The full-reactor compile is left intact (a changed module is still -# analysed with its complete aux-classpath). Master/snapshot builds run a full scan; -# this script is invoked on pull_request only. +# Default is RUN (analyze). On a PR this injects true +# properties into every UNCHANGED reactor module's pom, so the analysis mojos +# skip those modules — for SpotBugs that also skips the per-module JVM fork +# (SpotBugsMojo gates on `skip` before forking). A changed module is still +# analysed with its complete aux-classpath: the -am-pulled unchanged +# dependencies compile but are not analysed. Master/snapshot builds run a full +# scan; this script is invoked on pull_request only. # # Why this and not -Dspotbugs.onlyAnalyze: onlyAnalyze is one clean flag, but SpotBugs # applies its class screener too late (after the per-module fork + class scan), so it # only trimmed ~17% of the goal vs ~88% for this per-module skip (measured on this # reactor). A small upstream SpotBugs early-exit (skip the run when no application class # matches the screener) would make onlyAnalyze competitive; if that ever lands, switch -# to onlyAnalyze and delete this script (tracked in #1455 / spotbugs/spotbugs#3796). +# to onlyAnalyze and delete the spotbugs mode here (tracked in #1455 / +# spotbugs/spotbugs#3796). # -# On top of the skips, the changed reactor modules are exported as SPOTBUGS_SCOPE_ARGS -# ("-pl -am") so the lane builds only those modules plus their upstream -# dependencies instead of the full reactor. The -am-pulled unchanged dependencies still -# carry the injected skip: they compile (complete aux-classpath) but are not analysed. +# On top of the skips, the changed reactor modules are exported as +# SPOTBUGS_SCOPE_ARGS / LINT_SCOPE_ARGS ("-pl -am") so the lane builds +# only those modules plus their upstream dependencies instead of the full reactor. +# The lane's gate cross-checks _KEPT / _EXPECT_REPORTS so a build +# failure swallowed by --fail-never can never pass as "nothing to scan". # -# Run from the repository root. Usage: compute-spotbugs-skip.sh +# Run from the repository root. Usage: compute-analysis-skip.sh set -euo pipefail base="${1:?base sha required}" +mode="${2:?mode required: spotbugs|lint}" + +case "$mode" in + spotbugs) props="spotbugs.skip"; prefix="SPOTBUGS" ;; + lint) props="pmd.skip cpd.skip checkstyle.skip"; prefix="LINT" ;; + *) echo "unknown mode: $mode" >&2; exit 2 ;; +esac changed=$(git diff --name-only --diff-filter=ACMR "${base}...HEAD") @@ -45,8 +56,7 @@ ${module_dirs} EOF # 1) A change to shared build/config can affect any module -> full scan (skip nothing). -# ddk-configuration holds the analyzers' rulesets and filters (e.g. the SpotBugs -# exclusion-filter), so a change there must re-scan everything, not skip silently. +# ddk-configuration holds the analyzers' rulesets and filters, so it counts too. # ddk-target defines the target platform every module resolves against. # Fail safe: the worst case here is "analyse everything", never "analyse nothing". while IFS= read -r f; do @@ -60,10 +70,10 @@ while IFS= read -r f; do # full scan analysed all of them (not just >=1) — a mojo death swallowed by # --fail-never can't pass as long as one sibling reported. if [ -n "${GITHUB_ENV:-}" ]; then - echo "SPOTBUGS_KEPT=all" >> "$GITHUB_ENV" - echo "SPOTBUGS_EXPECT_REPORTS=${all_source_modules}" >> "$GITHUB_ENV" + echo "${prefix}_KEPT=all" >> "$GITHUB_ENV" + echo "${prefix}_EXPECT_REPORTS=${all_source_modules}" >> "$GITHUB_ENV" fi - echo "Build/config change ($f) -> full SpotBugs scan (no skips)." + echo "Build/config change ($f) -> full ${mode} scan (no skips)." exit 0 ;; esac @@ -76,18 +86,20 @@ EOF # grep's no-match exit would otherwise kill the script under pipefail. changed_mods=$(printf '%s\n' "${changed}" | { grep '/' || true; } | cut -d/ -f1 | sort -u) -# 3) Idempotently inject the skip property; handle poms with and without . +# 3) Idempotently inject the skip properties; handle poms with and without . # sed -i.bak + rm is portable across GNU (CI) and BSD (local) sed. inject_skip() { - local pom="$1/pom.xml" + local pom="$1/pom.xml" prop [ -f "$pom" ] || return 0 - if grep -q '' "$pom"; then return 0; fi - if grep -q '' "$pom"; then - sed -i.bak 's##\n true#' "$pom" - else - sed -i.bak 's## \n true\n \n#' "$pom" - fi - rm -f "$pom.bak" + for prop in $props; do + if grep -q "<${prop//./\\.}>" "$pom"; then continue; fi + if grep -q '' "$pom"; then + sed -i.bak "s##\n <${prop}>true#" "$pom" + else + sed -i.bak "s## \n <${prop}>true\n \n#" "$pom" + fi + rm -f "$pom.bak" + done } # 4) Skip every reactor module that was not touched by this PR. Kept modules with a @@ -103,7 +115,7 @@ while IFS= read -r mod; do kept=$((kept + 1)) kept_pl="${kept_pl:+${kept_pl},}../${mod}" # Only bundles with sources reliably emit a report (a source-less bundle, - # e.g. pure branding, has nothing for the analyzer to write a SARIF about). + # e.g. pure branding, has nothing for PMD to write a SARIF about). if [ -f "${mod}/META-INF/MANIFEST.MF" ] && [ -d "${mod}/src" ]; then expect_reports="${expect_reports:+${expect_reports} }${mod}" fi @@ -115,6 +127,13 @@ done <_KEPT=0. # The gate's presence check needs to distinguish "all modules skip-injected" # (zero reports is the expected state) from "the analysis silently died". # If the only changed modules are source-less (feature / target / repository — nothing @@ -127,25 +146,18 @@ else effective_kept=$kept fi if [ -n "${GITHUB_ENV:-}" ]; then - echo "SPOTBUGS_KEPT=${effective_kept}" >> "$GITHUB_ENV" + echo "${prefix}_KEPT=${effective_kept}" >> "$GITHUB_ENV" fi -# 5) Scope the reactor to the changed modules + their upstream dependencies. ddk-target -# is always kept in the -pl list: the target-definition artifact is referenced by -# target-platform-configuration, not by any MANIFEST, so -am never pulls it — without -# it in the reactor Tycho falls back to a local-repository copy, which fails on a -# cold cache and can silently resolve a stale target definition on a warm one. -# With no analysable changed module (docs-only, or source-less-only) no scope args -# are exported; the workflow skips the Maven step entirely on SPOTBUGS_KEPT=0. if [ "$effective_kept" -gt 0 ] && [ -n "${GITHUB_ENV:-}" ]; then - echo "SPOTBUGS_SCOPE_ARGS=-pl ../ddk-target,${kept_pl} -am" >> "$GITHUB_ENV" - echo "SPOTBUGS_EXPECT_REPORTS=${expect_reports}" >> "$GITHUB_ENV" + echo "${prefix}_SCOPE_ARGS=-pl ../ddk-target,${kept_pl} -am" >> "$GITHUB_ENV" + echo "${prefix}_EXPECT_REPORTS=${expect_reports}" >> "$GITHUB_ENV" fi -echo "SpotBugs scope: scanning ${effective_kept} changed module(s), skipping ${skipped} unchanged." +echo "${mode} scope: scanning ${effective_kept} changed module(s), skipping ${skipped} unchanged." echo "Changed modules: ${changed_mods:-}" if [ "$kept" -gt 0 ] && [ "$effective_kept" -eq 0 ]; then - echo "Only source-less modules changed (no analysable sources) -> no-op (KEPT=0)." + echo "Only source-less modules changed (no analysable sources) -> no-op (${prefix}_KEPT=0)." fi if [ "$effective_kept" -gt 0 ]; then echo "Reactor scope args: -pl ../ddk-target,${kept_pl} -am" diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index 727f044b86..bdaf2ab585 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -28,6 +28,8 @@ jobs: runs-on: ubuntu-24.04 steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + fetch-depth: 0 # need the PR base commit to diff the changed modules - uses: actions/setup-java@0f481fcb613427c0f801b606911222b5b6f3083a # v5 with: distribution: 'temurin' @@ -43,17 +45,33 @@ jobs: key: ${{ runner.os }}-maven-publish-${{ hashFiles('**/pom.xml', '**/*.target') }} restore-keys: ${{ runner.os }}-maven-publish- + - name: Scope static analysis to the PR's changed modules + # Injects pmd/cpd/checkstyle skip properties into unchanged module poms and + # exports LINT_SCOPE_ARGS (-pl -am) so only the changed modules and + # their upstream deps build (skip-injected deps compile for PMD's type + # resolution but are not analysed). Build/config change -> full scan, full + # reactor. pull_request only; master/snapshot run a full scan. + run: bash .github/scripts/compute-analysis-skip.sh "${{ github.event.pull_request.base.sha }}" lint + - name: PMD + Checkstyle reports (SARIF) # PMD: SarifRenderer FQCN — emits pmd.sarif.json AND keeps pmd.xml. # Checkstyle: output.format=sarif — SARIF content in checkstyle-result.xml. # CPD is excluded here: the global -Dformat flag uses PMD's Renderer # hierarchy and would ClassCastException CPD's CPDReportRenderer. + # `compile` stays: PMD's type-resolving rules need Tycho's aux-classpath. + # jgit.dirtyWorkingTree=ignore: the scope step edits poms (see the spotbugs + # lane for the rationale; this job releases nothing). + # Skipped entirely when the scope step kept no modules (e.g. a docs-only + # PR): every module would carry the skip properties, so the compile + # output would be unused. The gate below relaxes on the same condition. + if: env.LINT_KEPT != '0' run: | - mvn -T 2C -f ./ddk-parent/pom.xml --batch-mode --fail-never \ + mvn -T 2C -f ./ddk-parent/pom.xml ${LINT_SCOPE_ARGS:-} --batch-mode --fail-never \ compile \ pmd:pmd checkstyle:checkstyle \ -Dformat=net.sourceforge.pmd.renderers.SarifRenderer \ - -Dcheckstyle.output.format=sarif + -Dcheckstyle.output.format=sarif \ + -Djgit.dirtyWorkingTree=ignore - name: CPD report (separate invocation — no SARIF support) # CPD has no SARIF renderer; emits cpd.xml only. Run standalone so the @@ -63,8 +81,11 @@ jobs: # compile pass (verified at the current token threshold and at 100). # NOTE: the CPD token threshold is governed by pmd.cpd.min in # ddk-parent/pom.xml (#1339; tuned to 100 by #1397). + # No jgit flag needed: a direct goal invocation runs no lifecycle, so the + # build-qualifier's dirty-tree check never executes here. + if: env.LINT_KEPT != '0' run: | - mvn -T 2C -f ./ddk-parent/pom.xml --batch-mode --fail-never \ + mvn -T 2C -f ./ddk-parent/pom.xml ${LINT_SCOPE_ARGS:-} --batch-mode --fail-never \ pmd:cpd-check - name: Merge per-module SARIFs (PMD + Checkstyle) @@ -101,8 +122,29 @@ jobs: # merge() only writes its output when it found at least one valid input, # so a missing merged file means that analyzer silently died (e.g. a # plugin bump broke a renderer flag) — never a clean pass. + # Exception: the scope step skip-injected every module (no reactor module + # changed), where zero reports is the expected state. Each scanned + # source-bearing module must additionally have produced its own PMD + # SARIF, cpd.xml, and Checkstyle SARIF, so a partially-dead scoped + # build cannot hide either. run: | set -eu + if [ "${LINT_KEPT:-}" = "0" ]; then + echo "All modules skip-injected (no reactor module changed) — nothing to lint." + exit 0 + fi + for mod in ${LINT_EXPECT_REPORTS:-}; do + for rep in pmd.sarif.json cpd.xml; do + if [ ! -s "${mod}/target/${rep}" ]; then + echo "::error::${mod} was scanned but produced no ${rep} — a build failure was swallowed by --fail-never." + exit 1 + fi + done + if ! jq -e . "${mod}/target/checkstyle-result.xml" >/dev/null 2>&1; then + echo "::error::${mod} was scanned but produced no valid Checkstyle SARIF — a build failure was swallowed by --fail-never." + exit 1 + fi + done for f in .sarif-merged/pmd.sarif .sarif-merged/checkstyle.sarif; do if [ ! -s "$f" ]; then echo "::error::No valid SARIF input produced for ${f##*/} — the analysis silently failed." @@ -116,7 +158,7 @@ jobs: sarif_total=$(jq '[.runs[].results[]?] | length' \ .sarif-merged/pmd.sarif .sarif-merged/checkstyle.sarif 2>/dev/null \ | awk '{s+=$1} END {print s+0}') - cpd_total=$(find . -name 'cpd.xml' -path '*/target/*' -exec grep -c '/dev/null \ + cpd_total=$(find . -name 'cpd.xml' -path '*/target/*' -exec grep -cH '/dev/null \ | awk -F: '{s+=$2} END {print s+0}') echo "PMD/Checkstyle SARIF violations: $sarif_total" echo "CPD duplications: $cpd_total" @@ -126,7 +168,9 @@ jobs: fi - name: Upload PMD/Checkstyle SARIF to Code Scanning - if: always() + # Skip when nothing was scanned (e.g. a docs-only PR -> all modules skipped): + # an empty .sarif-merged would otherwise fail upload-sarif ("No SARIF files"). + if: ${{ always() && hashFiles('.sarif-merged/pmd.sarif', '.sarif-merged/checkstyle.sarif') != '' }} # Annotation-only, never the gate: a fork PR gets a read-only token and # upload-sarif 403s, which must not red an otherwise-clean lane. continue-on-error: true @@ -166,7 +210,7 @@ jobs: # changed modules and their upstream deps build at all (skip-injected deps # compile for the aux-classpath but are not analysed). A build/config change -> # full scan, full reactor. pull_request only; master/snapshot run a full scan. - run: bash .github/scripts/compute-spotbugs-skip.sh "${{ github.event.pull_request.base.sha }}" + run: bash .github/scripts/compute-analysis-skip.sh "${{ github.event.pull_request.base.sha }}" spotbugs - name: SpotBugs report (SARIF) # sarifOutput=true emits spotbugsSarif.json (also writes spotbugsXml.xml).