Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

test: add all handwritten repos to downstream check#795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
alicejli wants to merge12 commits intomain
base:main
Choose a base branch
Loading
fromupdateDownstreamCheck

Conversation

@alicejli
Copy link
Contributor

@alicejlialicejli commentedMar 20, 2024
edited
Loading

Fixes#790 ☕️

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

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

@product-auto-labelproduct-auto-labelbot added the size: sPull request size is small. labelMar 20, 2024
@generated-files-bot
Copy link

generated-files-botbot commentedMar 20, 2024
edited
Loading

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

  • .github/workflows/ci.yaml
  • .kokoro/build.sh

@alicejlialicejli marked this pull request as ready for reviewMarch 20, 2024 19:09
@alicejlialicejli requested a review froma team as acode ownerMarch 20, 2024 19:09
@alicejli
Copy link
ContributorAuthor

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 failureError:  /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.FeatureError:    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.0Error:      Please remove or make sure it appears in the correct subdirectory of the classpath.

@burkedavison
Copy link
Member

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
Copy link
ContributorAuthor

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 thinkci.yaml contains the relevant Java8 tests. And should I hold off on adding Java21 from these tests then?

@burkedavison
Copy link
Member

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
Copy link
ContributorAuthor

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: thetest,lint, andclirr checks indownstream-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
Copy link
Member

  • All downstream Java 8 unit testing should occur by building with a recent JDK (11, 17, or 21) and invoking surefire with JDK8.
  • Lint checks should only need to be run in the latest JDK we support.
  • I believe clirr is also a JDK-independent check, but we should discuss with others to verify.

@product-auto-labelproduct-auto-labelbot added size: mPull request size is medium. and removed size: sPull request size is small. labelsMar 21, 2024
@alicejli
Copy link
ContributorAuthor

@suztomo Burke and I chatted about these tests and wanted to confirm that theclirr andlint 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
Copy link
Member

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.

requiredStatusCheckContexts:
To make changes there, you may need to manually remove them in the Settings panel.

@alicejli
Copy link
ContributorAuthor

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

Yup that's the intention ofhttps://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 forlint and Java 8 forclirr? 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.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

size: mPull request size is medium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement downstream check to catch failing dependency updates

3 participants

@alicejli@burkedavison@suztomo

[8]ページ先頭

©2009-2025 Movatter.jp