diff --git a/.github/scripts/compute-spotbugs-skip.sh b/.github/scripts/compute-spotbugs-skip.sh index 6be5adaa1..96f7f7906 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 ca05a56a1..3298fa093 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) @@ -171,11 +172,19 @@ 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). + # 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 \ + -Dspotbugs.fork=false \ -Djgit.dirtyWorkingTree=ignore - name: Merge per-module SpotBugs SARIFs @@ -202,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 6cef71781..dc5f970eb 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.