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

TEZ-4374: Enable github action to compile with JDK 17 #386

Merged
merged 1 commit into from
Dec 31, 2024

Conversation

abstractdog
Copy link
Contributor

No description provided.

@abstractdog abstractdog requested a review from ayushtkn December 26, 2024 10:08
@abstractdog abstractdog changed the title TEZ-4374: Check compile-time compatibility with JDK 17 TEZ-4374: Check compile-time compatibility with JDK 17 in precommit Dec 26, 2024
@abstractdog
Copy link
Contributor Author

this is supposed to fail until TEZ-4387 is not present on master

@tez-yetus

This comment was marked as outdated.

@abstractdog
Copy link
Contributor Author

@ayushtkn: could you take a look? this activates Java17 compilation in precommit (only compliation, no tests yes)

Copy link
Member

@ayushtkn ayushtkn left a comment

Choose a reason for hiding this comment

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

LGTM

@ayushtkn
Copy link
Member

@abstractdog This is Runtime compatibility right? not Compile time, for compile time, we should change this to 17:

tez/pom.xml

Line 44 in c7a3791

<maven.compiler.source>1.8</maven.compiler.source>

As of now it is hardcoded 1.8 which is JDK-8 for 17 it should be changed to 17, or is there some place we do that which I missed?

@tez-yetus
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 23m 52s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
_ master Compile Tests _
_ Patch Compile Tests _
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 yamllint 0m 0s There were no new yamllint issues.
_ Other Tests _
+1 💚 asflicense 1m 31s The patch does not generate ASF License warnings.
26m 19s
Subsystem Report/Notes
Docker ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-386/2/artifact/out/Dockerfile
GITHUB PR #386
JIRA Issue TEZ-4374
Optional Tests dupname asflicense yamllint
uname Linux 8f9ccfed8c6a 5.15.0-124-generic #134-Ubuntu SMP Fri Sep 27 20:20:17 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/tez.sh
git revision master / c7a3791
Max. process+thread count 51 (vs. ulimit of 5500)
modules C: . U: .
Console output https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-386/2/console
versions git=2.34.1 maven=3.6.3 yamllint=1.24.2
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@abstractdog
Copy link
Contributor Author

abstractdog commented Dec 31, 2024

@abstractdog This is Runtime compatibility right? not Compile time, for compile time, we should change this to 17:

tez/pom.xml

Line 44 in c7a3791

<maven.compiler.source>1.8</maven.compiler.source>

As of now it is hardcoded 1.8 which is JDK-8 for 17 it should be changed to 17, or is there some place we do that which I missed?

hm, good question, I usually struggle with maven options like this honestly...what's driven by this patch is that the precommit will simply compile the source with different java versions, which is the same thing I achieve by pointing the java/javac etc. commands to a different JAVA_HOME (hence chaging java version) before compiling the code
whether maven should do any enforcement in this scenario is a different question

Screenshot 2024-12-31 at 10 34 06

@ayushtkn
Copy link
Member

As of now the byte code would be for JDK 8 only
https://stackoverflow.com/a/38883073

to have the byte code for JDK-17 we need to change that to 17

this is the offcial doc:
https://maven.apache.org/plugins/maven-compiler-plugin/examples/set-compiler-source-and-target.html

Maybe we can merge this with a changed title and explore the compile stuff in seperate ticket

@abstractdog
Copy link
Contributor Author

As of now the byte code would be for JDK 8 only https://stackoverflow.com/a/38883073

to have the byte code for JDK-17 we need to change that to 17

this is the offcial doc: https://maven.apache.org/plugins/maven-compiler-plugin/examples/set-compiler-source-and-target.html

Maybe we can merge this with a changed title and explore the compile stuff in seperate ticket

hm, okay, I just refreshed the source/target javac options a bit :D
what settings are recommended here?
in my understanding: the source version specifies the language features and syntax that the compiler should allow, while the target version specifies the bytecode compatibility for the Java runtime

according to official documentation (https://docs.oracle.com/javase/7/docs/technotes/tools/windows/javac.html#crosscomp-example) and other articles, -target defines the lowest JDK runtime version that we're about to make the code able to run against, I believe we haven't changed the project state at all from this point of view, right?
I mean, now with TEZ-4387 merged, we're able to compile against JDK17, but e.g. we haven't told that the minimum supported JDK version is anything greater than 8
hence the "Check compile-time compatibility with JDK 17 in precommit" makes sense

do you agree with the above points? what title do you recommend?

@ayushtkn
Copy link
Member

We can change the title to mention Runtime rather than compile time, same as in Hadoop it has runtime capabilities but doesn’t claim compile time bcoz the maven version is set as 1.8 changing doesn’t work either, that is something we also need to see what happens in case of Tez

and for 1.0 we can bump it to 17 and claim compile time compat.

anyway I am +1 here

@abstractdog
Copy link
Contributor Author

abstractdog commented Dec 31, 2024

We can change the title to mention Runtime rather than compile time, same as in Hadoop it has runtime capabilities but doesn’t claim compile time bcoz the maven version is set as 1.8 changing doesn’t work either, that is something we also need to see what happens in case of Tez

and for 1.0 we can bump it to 17 and claim compile time compat.

anyway I am +1 here

okay, I think I got your point, so it would be confusing to claim "compatibility" while keep using 1.8 -source and -target javac arguments
this PR does nothing more than enable a github action, that complies with the existing code with existing javac settings, so a title like this below is indeed more straightforward:

Enable github action to compile with JDK 17

wdyt?

@ayushtkn
Copy link
Member

Sounds cool

@abstractdog abstractdog changed the title TEZ-4374: Check compile-time compatibility with JDK 17 in precommit TEZ-4374: Enable github action to compile with JDK 17 Dec 31, 2024
@abstractdog abstractdog merged commit 1e6c9e3 into apache:master Dec 31, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants