search

googleapis / java-shared-config

Shared Maven build configuration for Google Cloud Java client libraries.
Apache License 2.0
19 stars 15 forks source link

test: add all handwritten repos to downstream check #795

Open alicejli opened 5 months ago

alicejli commented 5 months ago

Fixes #790 ☕️

This adds all handwritten library repos to the downstream checks for clirr, lint, and test.

This removes the javadoc check in favor of just checking the javadoc generation with the doclet tool as that is the main documentation generation we care about.

generated-files-bot[bot] commented 5 months ago

Warning: This pull request is touching the following templated files:

alicejli commented 5 months ago

Failing java-spanner test, lint, and clirr checks in Java 8:

Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.13.0:compile (default-compile) on project google-cloud-spanner: Compilation failure
Error:  /home/runner/work/java-shared-config/java-shared-config/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/nativeimage/SpannerFeature.java:[20,38] cannot access org.graalvm.nativeimage.hosted.Feature
Error:    bad class file: /home/runner/.m2/repository/org/graalvm/sdk/graal-sdk/22.3.5/graal-sdk-22.3.5.jar(org/graalvm/nativeimage/hosted/Feature.class)
Error:      class file has wrong version 55.0, should be 52.0
Error:      Please remove or make sure it appears in the correct subdirectory of the classpath.
burkedavison commented 5 months ago

GraalVM 22.3.5 supports JDK 11 and 17, so the SDK has been compiled with JDK11 as its minimum.

Thought: Can we disable compiling .../nativeimage/... unless using JDK11+, or unless the native profile is enabled? Would this break anything about the native build for downstream dependencies of Spanner?

cc @mpeddada1

alicejli commented 5 months ago

ling .../nativeimage/... unless using JDK11+, or unless the native profile is enabled? Would this break anything about the native bu

Should I just remove Java 8 from the matrix of tests? I think ci.yaml contains the relevant Java8 tests. And should I hold off on adding Java21 from these tests then?

burkedavison commented 5 months ago

Ah - in ci.yaml, we test Java 8 by compiling with JDK 17, and setting the SUREFIRE_JVM_OPT var to JDK 8.

If we can compile with JDK17 in Spanner then test with JDK8, it should resolve the original issue. Is that possible?

alicejli commented 5 months ago

Ah - in ci.yaml, we test Java 8 by compiling with JDK 17, and setting the SUREFIRE_JVM_OPT var to JDK 8.

If we can compile with JDK17 in Spanner then test with JDK8, it should resolve the original issue. Is that possible?

Just to double-check; this should be how we want to test for all the handwritten repos, not just Spanner right?

And to check my understanding of what we want to test: the test, lint, and clirr checks in downstream-maven-plugins.yaml should: 1) Compile the libraries in Java 11, test in Java 11 2) Compile the libraries in 17, test in Java 17 and test with Java 8 3) Compile with Java 21, test in Java 21

Is that accurate? Or do we only need to test the compilation of the libraries in 17 and test with Java 8?

burkedavison commented 5 months ago
alicejli commented 5 months ago

@suztomo Burke and I chatted about these tests and wanted to confirm that the clirr and lint checks only need to be checked on one version of Java (I picked Java 17) . Does this look correct to you? If so, then I can update the branch protection rules accordingly.

suztomo commented 5 months ago

Ensure Java 8 builds in the downstream repositories are checked. When a plugin stops working for Java 8, it should be caught in this repository, not after a release.

I picked Java 17

Ensure you use the same version of JDK that run the check in the downstream repository. E.g., https://github.com/googleapis/java-bigtable/blob/79988b2295b8a6093fa0cd272d058299b1ce3a03/.github/workflows/ci.yaml#L119

Here is the protection rule defintion. https://github.com/googleapis/java-shared-config/blob/34164052444ed9bd40a0789ed020b45cf4da17fb/.github/sync-repo-settings.yaml#L31 To make changes there, you may need to manually remove them in the Settings panel.

alicejli commented 5 months ago

Ensure you use the same version of JDK that run the check in the downstream repository. E.g.,

Yup that's the intention of https://github.com/googleapis/java-shared-config/pull/795/files#diff-796de8db06964f64e6d050f294aa49f53a031b33a40a0e3a4bdedbb03a453fb1R45

Ensure you use the same version of JDK that run the check in the downstream repository. E.g., https://github.com/googleapis/java-bigtable/blob/79988b2295b8a6093fa0cd272d058299b1ce3a03/.github/workflows/ci.yaml#L119

Good catch. @suztomo Do you know why we picked Java 11 for lint and Java 8 for clirr? If possible, it seems like if I update them in the synthtool template (https://github.com/googleapis/synthtool/blob/master/synthtool/gcp/templates/java_library/.github/workflows/ci.yaml#L111) to be consistent as Java17.