From f64e1b2b6b2bf9722c8943aa1fe5dc99937297b3 Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Tue, 1 Sep 2026 14:43:24 +0000 Subject: [PATCH] fix(ci): fix array iteration, variable leaks, and spurious full-reactor lint runs - In .kokoro/common.sh (update_all_poms_dependency): fix array iteration where `for pom in $POMS; do` only processed the first POM element. Update to `for pom in "${POMS[@]}"`, and use pure-Bash `${pom%/*}` instead of `$(dirname "$pom")`. - In .kokoro/common.sh (find_all_poms_with_versioned_dependency): reset POMS=(), use local arrays, and pre-filter candidates with grep -rl before executing xmllint. - In .kokoro/common.sh (generate_graalvm_modules_list): fix variable typo where modules_assigned_list appended leaked ${module} instead of loop variable ${maven_module}, and parse MAVEN_MODULES using pure-Bash `IFS=, read -ra`. - In .kokoro/build.sh: streamline changed Java module resolution into a single while loop with guard clauses, case pattern matching for exclusions, and cleanly skip linter checks when changed_modules is empty after exclusions (preventing spurious full-reactor lint runs without -pl). --- .kokoro/build.sh | 87 +++++++++++++++++++---------------------------- .kokoro/common.sh | 45 +++++++++++++++++------- 2 files changed, 68 insertions(+), 64 deletions(-) diff --git a/.kokoro/build.sh b/.kokoro/build.sh index e8a31ffc502d..46df57bed971 100755 --- a/.kokoro/build.sh +++ b/.kokoro/build.sh @@ -241,67 +241,50 @@ case ${JOB_TYPE} in changed_file_list=$(git diff --name-only "${BASE_SHA}" "${HEAD_SHA}" --relative) echo "${changed_file_list}" - has_code_change="false" - + changed_modules=() while IFS= read -r changed_file; do - # Checks if the line is not empty AND if it matches a .java file - if [ -n "${changed_file}" ] && [[ "${changed_file}" == *.java ]]; then - echo "Matched: ${changed_file}" - has_code_change="true" - break - fi - done <<< "${changed_file_list}" + [[ -z "${changed_file}" || "${changed_file}" != *.java ]] && continue - if [ "${has_code_change}" == "false" ]; then - echo "No java modules affected. Skipping linter check." - exit 0 - fi + # Skip samples and test utilities that have standalone builds: + case "${changed_file}" in + *samples* | *java-showcase* | *test-proxy*) + continue + ;; + esac - # Compute list of changed Maven modules from changed Java files. - # We walk each changed .java file up to its nearest pom.xml to find the correct module. - # e.g., if "java-asset/google-cloud-asset/src/main/java/Foo.java" is changed, - # it traverses upward until finding "java-asset/google-cloud-asset/pom.xml" and adds that module. - changed_modules=() - while IFS= read -r changed_file; do - if [ -n "${changed_file}" ] && [[ "${changed_file}" == *.java ]]; then - dir=$(dirname "${changed_file}") - while [ "${dir}" != "." ] && [ ! -f "${dir}/pom.xml" ]; do - dir=$(dirname "${dir}") - done - if [ -f "${dir}/pom.xml" ] && [ "${dir}" != "." ]; then - # Filter out directories not participating in the default formatting reactor: - # - samples are handwritten by developers - # - benchmarks are handwritten by developers - # - proto-*/grpc-* are generated code and should use the compiler format - # - *-bom/parents are POM-only and contain no Java source - if [[ "${dir}" != *"samples"* ]] && \ - [[ "${dir}" != *"java-showcase"* ]] && \ - [[ "$(basename "${dir}")" != *"benchmark"* ]] && \ - [[ "$(basename "${dir}")" != "proto-google-"* ]] && \ - [[ "$(basename "${dir}")" != "grpc-google-"* ]] && \ - [[ "$(basename "${dir}")" != *"-bom" ]] && \ - [[ "$(basename "${dir}")" != "google-cloud-pom-parent" ]] && \ - [[ "$(basename "${dir}")" != "dependency-analyzer" ]] && \ - [[ "$(basename "${dir}")" != "dependency-convergence-check" ]] && \ - [[ "$(basename "${dir}")" != "unmanaged-dependency-check" ]] && \ - [[ "$(basename "${dir}")" != *"test-proxy"* ]] && \ - [[ "$(basename "${dir}")" != "google-cloud-jar-parent" ]]; then + # Traverse upward to the nearest enclosing pom.xml: + dir=$(dirname "${changed_file}") + while [ "${dir}" != "." ] && [ ! -f "${dir}/pom.xml" ]; do + dir=$(dirname "${dir}") + done + + [ -f "${dir}/pom.xml" ] && [ "${dir}" != "." ] || continue - changed_modules+=("${dir}") - fi - fi - fi + # Skip modules not participating in the formatting reactor: + case "${dir##*/}" in + *benchmark* | proto-google-* | grpc-google-* | *-bom | \ + google-cloud-pom-parent | google-cloud-jar-parent | \ + dependency-analyzer | dependency-convergence-check | unmanaged-dependency-check) + continue + ;; + *) + changed_modules+=("${dir}") + ;; + esac done <<< "${changed_file_list}" echo "Changed Modules: ${changed_modules[*]}" - # Deduplicate the modules using sort -u to pass a concise list of unique modules - # via the Maven `-pl` argument. - if [ ${#changed_modules[@]} -gt 0 ]; then - unique_modules=$(printf '%s\n' "${changed_modules[@]}" | sort -u | paste -sd ',' -) - MODULE_FILTER="-pl ${unique_modules}" - echo "Formatting only changed modules: ${unique_modules}" + # If only non-reactor files or no Java files changed, skip cleanly without running full reactor: + if [ ${#changed_modules[@]} -eq 0 ]; then + echo "No relevant java modules affected after exclusions. Skipping linter check." + exit 0 fi + + # Pass a concise comma-separated list of unique modules via the Maven `-pl` argument. + unique_modules=$(printf '%s\n' "${changed_modules[@]}" | sort -u | paste -sd ',' -) + MODULE_FILTER="-pl ${unique_modules}" + echo "Formatting only changed modules: ${unique_modules}" else echo "BASE_SHA or HEAD_SHA is empty. Cannot continue linting." exit 1 diff --git a/.kokoro/common.sh b/.kokoro/common.sh index 5a967df4e3fa..b9ca0b3e94b4 100644 --- a/.kokoro/common.sh +++ b/.kokoro/common.sh @@ -498,15 +498,14 @@ function generate_graalvm_modules_list() { num=$((num + 1)) done elif [[ ${#modified_module_list[@]} -gt 0 ]]; then - # MAVEN_MODULES ENV_VAR is expecting comma delimited string (similar to mvn -pl) - # This will get all the modules and put all the elements into an array - maven_modules_list=($(echo "${MAVEN_MODULES}" | tr ',' ' ')) + # Parse comma-delimited MAVEN_MODULES into array using pure Bash: + IFS=',' read -ra maven_modules_list <<< "${MAVEN_MODULES}" for maven_module in "${maven_modules_list[@]}"; do # Check that the modified_module_list contains a module from MAVEN_MODULES # Spaces are intentionally added -- Query is regex and array elements are space separated # It tries to match the *exact* `maven_module` text if [[ " ${modified_module_list[*]} " =~ " ${maven_module} " ]]; then - modules_assigned_list+=("${module}") + modules_assigned_list+=("${maven_module}") fi done fi @@ -624,16 +623,36 @@ EOF popd || exit 1 } -# Find all pom.xml files that declare a specific version for the given artifact ($1) +# Find all pom.xml files that declare a specific version for the given artifact ($1). +# Pre-filters candidate files with grep -rl to avoid executing xmllint across hundreds +# of unrelated POM files in the repository. function find_all_poms_with_versioned_dependency { - poms=($(find . -name pom.xml)) - for pom in "${poms[@]}"; do + POMS=() + local found=() + local pom + + # Stream matching pom.xml paths line-by-line via process substitution '< <(...)', + # which executes the while loop in the current shell process so 'found' array mutations persist: + while IFS= read -r pom; do + [[ -z "${pom}" ]] && continue + # Verify the POM declares an explicit tag following the target : if xmllint --xpath "//*[local-name()='artifactId' and text()='$1']/following-sibling::*[local-name()='version']" "$pom" &>/dev/null; then found+=("$pom") fi - done - POMS=(${found[@]}) - unset found + done < <( + # Fast pre-filter using 'grep' to avoid parsing hundreds of unrelated POMs: + # - '-r': recursively scans directories. + # - '-l': prints each matching file path once (stops scanning a file on first match, + # preventing duplicate paths in the stream and saving I/O on large POMs). + # - '--include="pom.xml"': scopes search exclusively to POMs, ignoring non-POM files. + # - '...${1}...': matches tags around $1 with optional whitespace. + # - '2>/dev/null || true': silences errors and prevents 'set -e' failure when 0 files match. + grep -rlE "[[:space:]]*${1}[[:space:]]*" \ + --include="pom.xml" \ + . 2>/dev/null || true + ) + + POMS=("${found[@]}") export POMS } @@ -643,8 +662,10 @@ function find_all_poms_with_versioned_dependency { function update_all_poms_dependency { pushd "$1" || exit 1 find_all_poms_with_versioned_dependency "$2" - for pom in $POMS; do - update_pom_dependency "$(dirname "$pom")" "$2" "$3" + # Quote "${POMS[@]}" so the loop iterates over each array element safely: + for pom in "${POMS[@]}"; do + # Use '${pom%/*}' to extract the parent directory in pure Bash without a 'dirname' subshell: + update_pom_dependency "${pom%/*}" "$2" "$3" done git diff popd || exit 1