Skip to content

Commit 95b922e

Browse files
joaodinissfclaude
andcommitted
ci: build only changed modules + upstream deps in the spotbugs lane
compute-spotbugs-skip.sh now also exports SPOTBUGS_SCOPE_ARGS ("-pl ../ddk-target,<changed modules> -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 <noreply@anthropic.com>
1 parent 3552f8d commit 95b922e

3 files changed

Lines changed: 102 additions & 19 deletions

File tree

.github/scripts/compute-spotbugs-skip.sh

Lines changed: 76 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,36 @@
1414
# only trimmed ~17% of the goal vs ~88% for this per-module skip (measured on this
1515
# reactor). A small upstream SpotBugs early-exit (skip the run when no application class
1616
# matches the screener) would make onlyAnalyze competitive; if that ever lands, switch
17-
# to onlyAnalyze and delete this script.
17+
# to onlyAnalyze and delete this script (tracked in #1455 / spotbugs/spotbugs#3796).
18+
#
19+
# On top of the skips, the changed reactor modules are exported as SPOTBUGS_SCOPE_ARGS
20+
# ("-pl <changed> -am") so the lane builds only those modules plus their upstream
21+
# dependencies instead of the full reactor. The -am-pulled unchanged dependencies still
22+
# carry the injected skip: they compile (complete aux-classpath) but are not analysed.
1823
#
1924
# Run from the repository root. Usage: compute-spotbugs-skip.sh <base-sha>
2025
set -euo pipefail
2126
base="${1:?base sha required}"
2227

2328
changed=$(git diff --name-only --diff-filter=ACMR "${base}...HEAD")
2429

30+
# Reactor module dirs from ddk-parent's <modules> (strip the leading ../), and the
31+
# subset that bears sources. ddk-parent is NOT in its own <modules>, so it can never be
32+
# skip-injected — which prevents an accidental inherited (global) skip. Both lists are
33+
# computed up front because the full-scan early-exit below needs the source-bearing set
34+
# for its report-presence gate.
35+
module_dirs=$(grep -oE '<module>\.\./[^<]+</module>' ddk-parent/pom.xml \
36+
| sed -E 's#.*\.\./([^<]+)</module>#\1#')
37+
all_source_modules=""
38+
while IFS= read -r mod; do
39+
[ -n "$mod" ] || continue
40+
if [ -f "${mod}/META-INF/MANIFEST.MF" ] && [ -d "${mod}/src" ]; then
41+
all_source_modules="${all_source_modules:+${all_source_modules} }${mod}"
42+
fi
43+
done <<EOF
44+
${module_dirs}
45+
EOF
46+
2547
# 1) A change to shared build/config can affect any module -> full scan (skip nothing).
2648
# ddk-configuration holds the analyzers' rulesets and filters (e.g. the SpotBugs
2749
# exclusion-filter), so a change there must re-scan everything, not skip silently.
@@ -30,6 +52,16 @@ while IFS= read -r f; do
3052
[ -n "$f" ] || continue
3153
case "$f" in
3254
pom.xml | ddk-parent/* | .mvn/* | *.target | .github/* | ddk-configuration/* | *[Ss]pot[Bb]ugs*[Ee]xclude*)
55+
# KEPT must be set on every path: in a workflow `if:` an unset env var is
56+
# null, which coerces to 0 and compares EQUAL to '0' — wrongly skipping
57+
# the build. "all" marks the full-scan case (only "0" relaxes the gate).
58+
# EXPECT_REPORTS lists every source-bearing module so the gate verifies a
59+
# full scan analysed all of them (not just >=1) — a mojo death swallowed by
60+
# --fail-never can't pass as long as one sibling reported.
61+
if [ -n "${GITHUB_ENV:-}" ]; then
62+
echo "SPOTBUGS_KEPT=all" >> "$GITHUB_ENV"
63+
echo "SPOTBUGS_EXPECT_REPORTS=${all_source_modules}" >> "$GITHUB_ENV"
64+
fi
3365
echo "Build/config change ($f) -> full SpotBugs scan (no skips)."
3466
exit 0
3567
;;
@@ -43,13 +75,7 @@ EOF
4375
# grep's no-match exit would otherwise kill the script under pipefail.
4476
changed_mods=$(printf '%s\n' "${changed}" | { grep '/' || true; } | cut -d/ -f1 | sort -u)
4577

46-
# 3) Reactor module dirs from ddk-parent's <modules> (strip the leading ../).
47-
# ddk-parent is NOT in its own <modules>, so it can never be skip-injected — which
48-
# is what prevents an accidental inherited (global) skip.
49-
module_dirs=$(grep -oE '<module>\.\./[^<]+</module>' ddk-parent/pom.xml \
50-
| sed -E 's#.*\.\./([^<]+)</module>#\1#')
51-
52-
# 4) Idempotently inject the skip property; handle poms with and without <properties>.
78+
# 3) Idempotently inject the skip property; handle poms with and without <properties>.
5379
# sed -i.bak + rm is portable across GNU (CI) and BSD (local) sed.
5480
inject_skip() {
5581
local pom="$1/pom.xml"
@@ -63,13 +89,23 @@ inject_skip() {
6389
rm -f "$pom.bak"
6490
}
6591

66-
# 5) Skip every reactor module that was not touched by this PR.
92+
# 4) Skip every reactor module that was not touched by this PR. Kept modules with a
93+
# bundle MANIFEST are expected to produce an analysis report — the gate checks this
94+
# so a swallowed resolution/compile failure can never pass as "nothing to scan".
6795
kept=0
6896
skipped=0
97+
kept_pl=""
98+
expect_reports=""
6999
while IFS= read -r mod; do
70100
[ -n "$mod" ] || continue
71101
if printf '%s\n' "${changed_mods}" | grep -qx "$mod"; then
72102
kept=$((kept + 1))
103+
kept_pl="${kept_pl:+${kept_pl},}../${mod}"
104+
# Only bundles with sources reliably emit a report (a source-less bundle,
105+
# e.g. pure branding, has nothing for the analyzer to write a SARIF about).
106+
if [ -f "${mod}/META-INF/MANIFEST.MF" ] && [ -d "${mod}/src" ]; then
107+
expect_reports="${expect_reports:+${expect_reports} }${mod}"
108+
fi
73109
else
74110
inject_skip "$mod"
75111
skipped=$((skipped + 1))
@@ -80,9 +116,38 @@ EOF
80116

81117
# The gate's presence check needs to distinguish "all modules skip-injected"
82118
# (zero reports is the expected state) from "the analysis silently died".
119+
# If the only changed modules are source-less (feature / target / repository — nothing
120+
# any analyzer can report), fold into the docs-only no-op: export KEPT=0 so the lane
121+
# skips the Maven step, gate, and upload instead of failing the merged-report presence
122+
# check on output that could never exist.
123+
if [ "$kept" -eq 0 ] || [ -z "$expect_reports" ]; then
124+
effective_kept=0
125+
else
126+
effective_kept=$kept
127+
fi
83128
if [ -n "${GITHUB_ENV:-}" ]; then
84-
echo "SPOTBUGS_KEPT=${kept}" >> "$GITHUB_ENV"
129+
echo "SPOTBUGS_KEPT=${effective_kept}" >> "$GITHUB_ENV"
85130
fi
86131

87-
echo "SpotBugs scope: scanning ${kept} changed module(s), skipping ${skipped} unchanged."
132+
# 5) Scope the reactor to the changed modules + their upstream dependencies. ddk-target
133+
# is always kept in the -pl list: the target-definition artifact is referenced by
134+
# target-platform-configuration, not by any MANIFEST, so -am never pulls it — without
135+
# it in the reactor Tycho falls back to a local-repository copy, which fails on a
136+
# cold cache and can silently resolve a stale target definition on a warm one.
137+
# With no analysable changed module (docs-only, or source-less-only) no scope args
138+
# are exported; the workflow skips the Maven step entirely on SPOTBUGS_KEPT=0.
139+
if [ "$effective_kept" -gt 0 ] && [ -n "${GITHUB_ENV:-}" ]; then
140+
echo "SPOTBUGS_SCOPE_ARGS=-pl ../ddk-target,${kept_pl} -am" >> "$GITHUB_ENV"
141+
echo "SPOTBUGS_EXPECT_REPORTS=${expect_reports}" >> "$GITHUB_ENV"
142+
fi
143+
144+
echo "SpotBugs scope: scanning ${effective_kept} changed module(s), skipping ${skipped} unchanged."
88145
echo "Changed modules: ${changed_mods:-<none>}"
146+
if [ "$kept" -gt 0 ] && [ "$effective_kept" -eq 0 ]; then
147+
echo "Only source-less modules changed (no analysable sources) -> no-op (KEPT=0)."
148+
fi
149+
if [ "$effective_kept" -gt 0 ]; then
150+
echo "Reactor scope args: -pl ../ddk-target,${kept_pl} -am"
151+
else
152+
echo "Reactor scope args: <full reactor>"
153+
fi

.github/workflows/verify.yml

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -159,10 +159,11 @@ jobs:
159159
restore-keys: ${{ runner.os }}-maven-publish-
160160

161161
- name: Scope SpotBugs to the PR's changed modules
162-
# Injects <spotbugs.skip>true> into unchanged module poms so the per-module
163-
# SpotBugs fork is skipped for them (the lever that actually scopes the cost).
164-
# Full compile is preserved (correct aux-classpath); a build/config change ->
165-
# full scan. pull_request only; master/snapshot run a full scan.
162+
# Injects <spotbugs.skip>true> into unchanged module poms so their analysis is
163+
# skipped, and exports SPOTBUGS_SCOPE_ARGS (-pl <changed> -am) so only the
164+
# changed modules and their upstream deps build at all (skip-injected deps
165+
# compile for the aux-classpath but are not analysed). A build/config change ->
166+
# full scan, full reactor. pull_request only; master/snapshot run a full scan.
166167
run: bash .github/scripts/compute-spotbugs-skip.sh "${{ github.event.pull_request.base.sha }}"
167168

168169
- name: SpotBugs report (SARIF)
@@ -174,8 +175,12 @@ jobs:
174175
# spotbugs.fork=false analyses in the Maven JVM instead of forking a fresh 2 GB
175176
# JVM per module (59 forks); the shared heap is governed by MAVEN_OPTS above
176177
# (the plugin's maxHeap applies to forks only).
178+
# Skipped entirely when the scope step kept no modules (e.g. a docs-only
179+
# PR): every module would carry spotbugs.skip, so the compile output
180+
# would be unused. The gate below relaxes on the same condition.
181+
if: env.SPOTBUGS_KEPT != '0'
177182
run: |
178-
mvn -T 2C -f ./ddk-parent/pom.xml --batch-mode --fail-never \
183+
mvn -T 2C -f ./ddk-parent/pom.xml ${SPOTBUGS_SCOPE_ARGS:-} --batch-mode --fail-never \
179184
compile \
180185
spotbugs:spotbugs \
181186
-Dspotbugs.sarifOutput=true \
@@ -206,12 +211,20 @@ jobs:
206211
# suppresses even compile/resolution failures) — never a clean pass.
207212
# Exception: the scope step skip-injected every module (no reactor module
208213
# changed, e.g. a docs-only PR), where zero reports is the expected state.
214+
# Each scanned source-bearing module must additionally have produced its
215+
# own SARIF, so a partially-dead scoped build cannot hide either.
209216
run: |
210217
set -eu
211218
if [ "${SPOTBUGS_KEPT:-}" = "0" ]; then
212219
echo "All modules skip-injected (no reactor module changed) — nothing to scan."
213220
exit 0
214221
fi
222+
for mod in ${SPOTBUGS_EXPECT_REPORTS:-}; do
223+
if [ ! -s "${mod}/target/spotbugsSarif.json" ]; then
224+
echo "::error::${mod} was scanned but produced no SpotBugs SARIF — a build failure was swallowed by --fail-never."
225+
exit 1
226+
fi
227+
done
215228
if [ ! -s .sarif-merged/spotbugs.sarif ]; then
216229
echo "::error::No SpotBugs SARIF produced — the analysis silently failed."
217230
exit 1

docs/ci-static-analysis-design.md

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,9 +91,14 @@ the count-gate *stricter* than `:check` (over-fail, never under-fail), each guar
9191

9292
## Two operational rules
9393

94-
- **`compile` must be full-reactor** (`-f ddk-parent/pom.xml`, no `-pl`). PMD's
95-
type-resolving rules need the complete aux-classpath; a `-pl` subset produces
96-
false positives (the trailing-`Throwable` case).
94+
- **A changed module must compile against its complete aux-classpath**, or PMD's
95+
type-resolving rules false-positive (the trailing-`Throwable` case). A bare `-pl <module>`
96+
subset breaks this, but `-pl <changed> -am` does not: `--also-make` restores the module's
97+
full **Maven** dependency closure, which compiles for the classpath even though those deps
98+
carry the analysis skip. Caveat: OSGi `Require-Bundle` siblings are *not* Maven dependencies,
99+
so `-am` never pulls them — they resolve from the restored `~/.m2` p2 cache; `ddk-target` is
100+
the one edge with no MANIFEST reference at all, so it is pinned explicitly into every scoped
101+
reactor (a cold cache without it fails loudly rather than resolving a stale target).
97102
- **Merge SARIFs from SARIF files only.** Code Scanning accepts one run per category,
98103
so per-module SARIFs are merged (jq) before upload. The `ddk-parent` aggregator emits
99104
plain-XML `checkstyle-result.xml`; the merge must filter to JSON-parseable files.

0 commit comments

Comments
 (0)