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 96f7f7906..d645ee9a8 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 3298fa093..2418ca384 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@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 # need the PR base commit to diff the changed modules - uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5 with: distribution: 'temurin' @@ -43,26 +45,47 @@ 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 # 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). + # 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 \ - compile \ + 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) @@ -99,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." @@ -114,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" @@ -124,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 @@ -164,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).