Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix verifyJavaVersion in a few places #32183

Merged
merged 3 commits into from
Aug 14, 2024
Merged

Fix verifyJavaVersion in a few places #32183

merged 3 commits into from
Aug 14, 2024

Conversation

Abacn
Copy link
Contributor

@Abacn Abacn commented Aug 14, 2024

Two issues

  1. It is found that after Setup Java 21 container #28918, the java version verification test only checks that the source code was compile with Java8, but not test code compiled/running on certain Java version

  2. We had overly strict sdks:java:docker:javaX:validateJavaHome . If system java version is Java11, xlangPythonUsingJava will add a java 11 docker task, which currently requires -Pjava11Home, which is not necessary.

Please add a meaningful description for your change here


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

@github-actions github-actions bot added the java label Aug 14, 2024
@Abacn
Copy link
Contributor Author

Abacn commented Aug 14, 2024

Run SQL_Java8 PreCommit

https://github.com/apache/beam/actions/runs/10393016359

now runs

verifyTestCodeIsCompiledWithJava8
PASSED	0.051s			
verifyCodeIsCompiledWithJava8
PASSED	0.006s			
verifyRunningJVMVersionIs8
PASSED	0.000s			

@Abacn
Copy link
Contributor Author

Abacn commented Aug 14, 2024

Run SQL_Java17 PreCommit

https://github.com/apache/beam/actions/runs/10393018367

now runs

:sdks:java:testing:test-utils:verifyJavaVersion17
PASSED	1.698s	1.556s	1.698s	
org.apache.beam.sdk.testutils.jvmverification.JvmVerification
PASSED	0.129s	0.089s	0.129s	
verifyCodeIsCompiledWithJava8
PASSED	0.036s			
verifyRunningJVMVersionIs17
PASSED	0.002s			
verifyTestCodeIsCompiledWithJava17
PASSED	0.002s			

@@ -40,15 +40,15 @@ dependencies {
provided library.java.jmh_core

testImplementation library.java.junit
testRuntimeOnly project(path: ":runners:direct-java", configuration: "shadowTest")
testRuntimeOnly project(path: ":runners:direct-java", configuration: "shadow")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shadowTest currently does not work on Java21 tests because shadow plugin does not support Java21:

org.gradle.api.GradleException: Could not add file '/Users/yathu/beam/sdks/java/core/build/classes/java/test/DefaultPackageTest$MyFn.class' to ZIP '/Users/yathu/beam/sdks/java/core/build/libs/beam-sdks-java-core-2.59.0-SNAPSHOT-tests.jar'.
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
Caused by: java.lang.IllegalArgumentException: Unsupported class file major version 65
        at shadow.org.objectweb.asm.ClassReader.<init>(ClassReader.java:199)
        at shadow.org.objectweb.asm.ClassReader.<init>(ClassReader.java:180)
        at shadow.org.objectweb.asm.ClassReader.<init>(ClassReader.java:166)
        at shadow.org.objectweb.asm.ClassReader.<init>(ClassReader.java:287)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
        at org.codehaus.groovy.reflection.CachedConstructor.invoke(CachedConstructor.java:72)

here "shadow" configuration is sufficient (like other tests runs pipeline on direct runner)

@Abacn Abacn marked this pull request as ready for review August 14, 2024 19:31
@Abacn
Copy link
Contributor Author

Abacn commented Aug 14, 2024

PreCommit Java Example Java21 now verifies java version:

https://github.com/apache/beam/actions/runs/10393658121?pr=32183

:sdks:java:testing:test-utils:verifyJavaVersion21
PASSED	2.146s	1.953s	2.146s	
org.apache.beam.sdk.testutils.jvmverification.JvmVerification
PASSED	0.179s	0.121s	0.179s	
verifyCodeIsCompiledWithJava8
PASSED	0.045s			
verifyRunningJVMVersionIs21
PASSED	0.011s			
verifyTestCodeIsCompiledWithJava21
PASSED	0.002s			

Copy link
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

@Abacn
Copy link
Contributor Author

Abacn commented Aug 14, 2024

assign set of reviewers

@Abacn
Copy link
Contributor Author

Abacn commented Aug 14, 2024

R: @tvalentyn @damccorm

Copy link
Contributor

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @m-trieu for label java.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

Copy link
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

['8', '11', '17', '21'].each {
tasks.create(name: "verifyJavaVersion${it}", type: Test) {
['8', '11', '17', '21'].each { String ver ->
tasks.create(name: "verifyJavaVersion${ver}", type: Test) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Groovy has a many implicit and errorprone syntax and this is one of it. Foreach closure has an implicit variable that has default name it. However, the tasks.create closure override the same name, causing "${it}" resolved to "Task " and as a result, only verifyCodeIsCompiledWithJava8 was run

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof. good catch.

@Abacn
Copy link
Contributor Author

Abacn commented Aug 14, 2024

The latest commit should fix https://github.com/apache/beam/actions/workflows/beam_PreCommit_Xlang_Generated_Transforms.yml currently failing on master

@Abacn
Copy link
Contributor Author

Abacn commented Aug 14, 2024

Run Xlang_Generated_Transforms PreCommit

update:

https://github.com/apache/beam/actions/runs/10395162471/job/28786672477 pased

@Abacn Abacn changed the title Fix verifyJavaVersion wrong filter Fix verifyJavaVersion in a few places Aug 14, 2024
@Abacn Abacn merged commit 1eb416e into apache:master Aug 14, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants