diff --git a/.github/scripts/compute-analysis-skip.sh b/.github/scripts/compute-analysis-skip.sh new file mode 100755 index 000000000..b30699b19 --- /dev/null +++ b/.github/scripts/compute-analysis-skip.sh @@ -0,0 +1,171 @@ +#!/usr/bin/env bash +# +# 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 +# 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. +# +# Accepted trade-off: only changed modules are analysed. A change whose effect shows +# up as a finding in an unchanged dependent module surfaces on the master full scan. +# +# 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 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 / 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-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 + +# --no-renames reports a move as delete + add, so both the old and the new module +# count as changed; D keeps deletion-only changes (e.g. a removed ruleset) visible. +changed=$(git diff --name-only --no-renames --diff-filter=ACMD "${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, 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 + [ -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 "${prefix}_KEPT=all" >> "$GITHUB_ENV" + echo "${prefix}_EXPECT_REPORTS=${all_source_modules}" >> "$GITHUB_ENV" + fi + echo "Build/config change ($f) -> full ${mode} scan (no skips)." + exit 0 + ;; + esac +done <. +# sed -i.bak + rm is portable across GNU (CI) and BSD (local) sed. +inject_skip() { + local pom="$1/pom.xml" prop + [ -f "$pom" ] || return 0 + 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 +# 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 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 + else + inject_skip "$mod" + skipped=$((skipped + 1)) + fi +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 +# 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 "${prefix}_KEPT=${effective_kept}" >> "$GITHUB_ENV" +fi + +if [ "$effective_kept" -gt 0 ] && [ -n "${GITHUB_ENV:-}" ]; then + echo "${prefix}_SCOPE_ARGS=-pl ../ddk-target,${kept_pl} -am" >> "$GITHUB_ENV" + echo "${prefix}_EXPECT_REPORTS=${expect_reports}" >> "$GITHUB_ENV" +fi + +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 (${prefix}_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 4e16cd0eb..daab13733 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -36,6 +36,8 @@ jobs: runs-on: ubuntu-24.04 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 # need the PR base commit to diff the changed modules - uses: actions/setup-java@de7274f081f381c8f8158605e0321c36c376e2e6 # v6.0.1 with: distribution: 'temurin' @@ -54,26 +56,47 @@ jobs: - name: Install XML report validator run: sudo apt-get update && sudo apt-get install --yes libxml2-utils + - 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-at-end \ + mvn -T 2C -f ./ddk-parent/pom.xml ${LINT_SCOPE_ARGS:-} --batch-mode --fail-at-end \ 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 # PMD -Dformat flag isn't in scope. + # No `compile`: CPD is token-based over src/ and needs neither bytecode + # nor the target platform — cpd.xml is identical with and without a + # compile pass. # NOTE: the CPD token threshold is governed by pmd.cpd.min in # ddk-parent/pom.xml. + # 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-at-end \ - compile \ + mvn -T 2C -f ./ddk-parent/pom.xml ${LINT_SCOPE_ARGS:-} --batch-mode --fail-at-end \ pmd:cpd-check - name: Merge per-module SARIFs (PMD + Checkstyle) @@ -81,6 +104,10 @@ jobs: # Merge only expected module reports into one run per analyzer. run: | set -euo pipefail + if [ "${LINT_KEPT:-}" = "0" ]; then + echo "Scope contains no analysable modules — nothing to lint." + exit 0 + fi source .github/scripts/sarif.sh source_modules=$(sarif_source_modules) merge_sarif pmd.sarif.json .sarif-merged/pmd.sarif "${LINT_EXPECT_REPORTS:-$source_modules}" PMD @@ -90,6 +117,10 @@ jobs: # Require successful reports from every expected module before counting findings. run: | set -euo pipefail + if [ "${LINT_KEPT:-}" = "0" ]; then + echo "Scope contains no analysable modules — nothing to lint." + exit 0 + fi source .github/scripts/sarif.sh source_modules=$(sarif_source_modules) cpd_reports=() @@ -119,7 +150,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 @@ -136,6 +169,8 @@ jobs: MAVEN_OPTS: -Xmx4g steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 # need the PR base commit to diff the changed modules - uses: actions/setup-java@de7274f081f381c8f8158605e0321c36c376e2e6 # v6.0.1 with: distribution: 'temurin' @@ -151,18 +186,39 @@ 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 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-analysis-skip.sh "${{ github.event.pull_request.base.sha }}" spotbugs + - 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). + # 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-at-end \ + mvn -T 2C -f ./ddk-parent/pom.xml ${SPOTBUGS_SCOPE_ARGS:-} --batch-mode --fail-at-end \ compile \ spotbugs:spotbugs \ - -Dspotbugs.sarifOutput=true + -Dspotbugs.sarifOutput=true \ + -Djgit.dirtyWorkingTree=ignore - name: Merge per-module SpotBugs SARIFs if: always() run: | set -euo pipefail + if [ "${SPOTBUGS_KEPT:-}" = "0" ]; then + echo "Scope contains no analysable modules — nothing to scan." + exit 0 + fi source .github/scripts/sarif.sh source_modules=$(sarif_source_modules) merge_sarif spotbugsSarif.json .sarif-merged/spotbugs.sarif "${SPOTBUGS_EXPECT_REPORTS:-$source_modules}" SpotBugs @@ -171,6 +227,10 @@ jobs: # Require successful reports from every expected module before counting findings. run: | set -euo pipefail + if [ "${SPOTBUGS_KEPT:-}" = "0" ]; then + echo "Scope contains no analysable modules — nothing to scan." + exit 0 + fi source .github/scripts/sarif.sh source_modules=$(sarif_source_modules) for mod in ${SPOTBUGS_EXPECT_REPORTS:-$source_modules}; do @@ -185,7 +245,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 diff --git a/docs/ci-static-analysis-design.md b/docs/ci-static-analysis-design.md index 8e7decc74..56ffba37b 100644 --- a/docs/ci-static-analysis-design.md +++ b/docs/ci-static-analysis-design.md @@ -94,9 +94,15 @@ 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 + dependency closure, which compiles for the classpath even though those deps carry the + analysis skip. Tycho adds each bundle's MANIFEST requirements (`Require-Bundle`, + `Import-Package`) to the Maven project model, so `-am` pulls in reactor siblings required + that way too. `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`; it is excluded by the expected source-module list.