chore: restore nightly protobuf compatibility check by whowes · Pull Request #13884 · googleapis/google-cloud-java

whowes · 2026-07-23T23:31:16Z

WIP

gemini-code-assist

Code Review

This pull request refactors the nightly downstream Protobuf compatibility scripts to test local monorepo submodules directly instead of cloning them from GitHub, removing the special-casing for the google-cloud-java repository. Feedback on these changes recommends using standard CLI commands to retrieve tool versions rather than manually parsing versions.txt, redirecting error messages to standard error (>&2), and appending || exit 1 to pushd commands to ensure the scripts terminate safely if directory changes fail.

gemini-code-assist · 2026-07-23T23:33:02Z

    # The grep query tries to match `{artifact_id}:{released_version}:{current_version}`.
    # The artifact_id must be exact otherwise multiple entries may match
-    version=$(cat "versions.txt" | grep -E "^${artifact_id}:.*:.*$" | cut -d ':' -f3)
+    version=$(cat "${monorepoRoot}/versions.txt" | grep -E "^${artifact_id}:.*:.*$" | cut -d ':' -f3)


Prefer using standard CLI commands to retrieve tool versions rather than manually parsing configuration files, unless performance or latency issues are actively observed.

Suggested change

version=$(cat "${monorepoRoot}/versions.txt" | grep -E "^${artifact_id}:.*:.*$" | cut -d ':' -f3)

version=$(librarian config get version "${artifact_id}")

References

Prefer using standard CLI commands (such as librarian config get version) to retrieve tool versions rather than manually parsing configuration files, unless performance or latency issues are actively observed.

gemini-code-assist · 2026-07-23T23:33:02Z

-    mvn clean install -B -V -ntp -T 1C -DskipTests -Dclirr.skip -Denforcer.skip -Dmaven.javadoc.skip
+  repo_dir="${monorepoRoot}/${repo}"
+  if [ ! -d "${repo_dir}" ]; then
+    echo "Directory ${repo_dir} does not exist. Skipping or failed."


Error messages should be redirected to standard error (>&2) instead of standard output.

Suggested change

echo "Directory ${repo_dir} does not exist. Skipping or failed."

echo "Directory ${repo_dir} does not exist. Skipping or failed." >&2

gemini-code-assist · 2026-07-23T23:33:02Z

+    exit 1
  fi

+  pushd "${repo_dir}"


It is safer to append || exit 1 to pushd to ensure the script terminates if the directory change fails, preventing subsequent commands (like mvn clean install) from running in the wrong directory.

Suggested change

pushd "${repo_dir}"

pushd "${repo_dir}" || exit 1

gemini-code-assist · 2026-07-23T23:33:02Z

-  pushd "$repo"
+  repo_dir="${monorepoRoot}/${repo}"
+  if [ ! -d "${repo_dir}" ]; then
+    echo "Directory ${repo_dir} does not exist. Skipping or failed."


Error messages should be redirected to standard error (>&2) instead of standard output.

Suggested change

echo "Directory ${repo_dir} does not exist. Skipping or failed."

echo "Directory ${repo_dir} does not exist. Skipping or failed." >&2

gemini-code-assist · 2026-07-23T23:33:02Z

+    exit 1
+  fi
+
+  pushd "${repo_dir}"


It is safer to append || exit 1 to pushd to ensure the script terminates if the directory change fails, preventing subsequent commands from running in the wrong directory.

Suggested change

pushd "${repo_dir}"

pushd "${repo_dir}" || exit 1

sonarqubecloud · 2026-07-25T02:04:56Z

Quality Gate passed for 'gapic-generator-java-root'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

sonarqubecloud · 2026-07-25T02:06:28Z

Quality Gate passed for 'gapic-generator-java-root'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

whowes · 2026-07-26T05:42:26Z

/gemini review

gemini-code-assist

Code Review

This pull request updates the Kokoro nightly downstream compatibility scripts to test local monorepo submodules instead of cloning external repositories. It also adds KMS dependencies to the installation list, excludes StorageITRunnerTest from storage tests, and configures test options for Java 8 compatibility. Feedback on the changes suggests improving Bash script efficiency and robustness, specifically by using native parameter expansion instead of cut, avoiding useless use of cat with grep, and using Bash arrays instead of unquoted string variables to prevent word splitting issues.

gemini-code-assist · 2026-07-26T05:44:00Z

+  artifact_list="${module_linkage_checker_arguments[$1]}"

  for artifact in ${artifact_list//,/ }; do # Split on comma
    artifact_id=$(echo "${artifact}" | cut -d ':' -f2)


Using a subshell with echo and cut to extract the artifact ID is inefficient. We can use native Bash parameter expansion ${artifact#*:} instead, which is faster and cleaner.

Suggested change

artifact_id=$(echo "${artifact}" | cut -d ':' -f2)

artifact_id="${artifact#*:}"

gemini-code-assist · 2026-07-26T05:44:01Z

    # The artifact_id must be exact otherwise multiple entries may match
-    version=$(cat "versions.txt" | grep -E "^${artifact_id}:.*:.*$" | cut -d ':' -f3)
-    repo_gav_coordinate="${artifact}:${version}"
+    version=$(cat "${monorepoRoot}/versions.txt" | grep -E "^${artifact_id}:.*:.*$" | cut -d ':' -f3 || true)


Avoid useless use of cat (UUOC) by passing the file directly to grep.

Suggested change

version=$(cat "${monorepoRoot}/versions.txt" | grep -E "^${artifact_id}:.*:.*$" | cut -d ':' -f3 || true)

version=$(grep -E "^${artifact_id}:.*:.*$" "${monorepoRoot}/versions.txt" | cut -d ':' -f3 || true)

gemini-code-assist · 2026-07-26T05:44:01Z

  if [ -n "${JAVA8_HOME}" ]; then
-    surefire_opt="-Djvm=${JAVA8_HOME}/bin/java"
+    jvm_opt="-Djvm=${JAVA8_HOME}/bin/java"
+    # Required for JDK 8 to bypass Java 11 LogbackServiceProvider and exclude JDK 11 Jqwik property tests
+    test_opts="-Dslf4j.provider=org.slf4j.helpers.NOP_FallbackServiceProvider -P!jqwik-tests -Pexclude-jqwik-on-java8"
  else
    # Provide a default value for local executions that don't configure JAVA8_HOME
-    surefire_opt="-Djvm=${JAVA_HOME}/bin/java"
+    jvm_opt="-Djvm=${JAVA_HOME}/bin/java"
+    test_opts=""
  fi

-  # Compile the Handwritten Library with the Protobuf-Java version to test source compatibility
+  # Compile the library with the Protobuf-Java version to test source compatibility
  # Run unit tests to help check for any behavior differences (dependant on coverage)
-  if [ "${repo}" == "google-cloud-java" ]; then
-    # The `-am` command also builds anything these libraries depend on (i.e. proto-* and grpc-* sub modules)
-    mvn test -B -V -ntp \
-      -Dclirr.skip \
-      -Denforcer.skip \
-      -Dmaven.javadoc.skip \
-      -Denforcer.skip \
-      -Dprotobuf.version=${PROTOBUF_RUNTIME_VERSION} \
-      -pl "${google_cloud_java_handwritten_maven_args}" -am \
-      "${surefire_opt}" \
-      -T 1C
-  else
-    mvn test -B -V -ntp \
-      -Dclirr.skip \
-      -Denforcer.skip \
-      -Dmaven.javadoc.skip \
-      -Denforcer.skip \
-      -Dprotobuf.version=${PROTOBUF_RUNTIME_VERSION} \
-      "${surefire_opt}" \
-      -T 1C
-  fi
+  mvn test -B -V -ntp \
+    -Dclirr.skip \
+    -Denforcer.skip \
+    -Dmaven.javadoc.skip \
+    -Dprotobuf.version=${PROTOBUF_RUNTIME_VERSION} \
+    "${jvm_opt}" \
+    ${test_opts} \
+    -T 1C


Using unquoted variables like ${test_opts} can lead to unexpected word splitting or globbing issues if they contain special characters. A more robust and idiomatic way to handle multiple command-line arguments in Bash is using arrays.

declare -a test_opts=() if [ -n "${JAVA8_HOME}" ]; then jvm_opt="-Djvm=${JAVA8_HOME}/bin/java" # Required for JDK 8 to bypass Java 11 LogbackServiceProvider and exclude JDK 11 Jqwik property tests test_opts=( "-Dslf4j.provider=org.slf4j.helpers.NOP_FallbackServiceProvider" "-P!jqwik-tests" "-Pexclude-jqwik-on-java8" ) else # Provide a default value for local executions that don't configure JAVA8_HOME jvm_opt="-Djvm=${JAVA_HOME}/bin/java" fi # Compile the library with the Protobuf-Java version to test source compatibility # Run unit tests to help check for any behavior differences (dependant on coverage) mvn test -B -V -ntp \ -Dclirr.skip \ -Denforcer.skip \ -Dmaven.javadoc.skip \ -Dprotobuf.version="${PROTOBUF_RUNTIME_VERSION}" \ "${jvm_opt}" \ "${test_opts[@]}" \ -T 1C

gemini-code-assist Bot reviewed Jul 23, 2026

View reviewed changes

whowes force-pushed the whowes/restore-proto-compat-check branch 21 times, most recently from cd8ef4f to d8b7a8e Compare July 25, 2026 01:44

chore: restore nightly protobuf compatibility check

24ef975

whowes force-pushed the whowes/restore-proto-compat-check branch from d8b7a8e to 24ef975 Compare July 25, 2026 01:54

gemini-code-assist Bot reviewed Jul 26, 2026

View reviewed changes

	version=$(cat "${monorepoRoot}/versions.txt" \| grep -E "^${artifact_id}:.:.$" \| cut -d ':' -f3)
	version=$(librarian config get version "${artifact_id}")

	echo "Directory ${repo_dir} does not exist. Skipping or failed."
	echo "Directory ${repo_dir} does not exist. Skipping or failed." >&2

	artifact_id=$(echo "${artifact}" \| cut -d ':' -f2)
	artifact_id="${artifact#*:}"

	version=$(cat "${monorepoRoot}/versions.txt" \| grep -E "^${artifact_id}:.:.$" \| cut -d ':' -f3 \|\| true)
	version=$(grep -E "^${artifact_id}:.:.$" "${monorepoRoot}/versions.txt" \| cut -d ':' -f3 \|\| true)

Uh oh!

Conversation

whowes commented Jul 23, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Jul 25, 2026

Quality Gate passed for 'gapic-generator-java-root'

Uh oh!

sonarqubecloud Bot commented Jul 25, 2026

Quality Gate passed for 'gapic-generator-java-root'

Uh oh!

whowes commented Jul 26, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant