Skip to content

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

Merged
abstractdog merged 1 commit into
apache:masterfrom
abstractdog:TEZ-4374
Dec 31, 2024
Merged

TEZ-4374: Enable github action to compile with JDK 17#386
abstractdog merged 1 commit into
apache:masterfrom
abstractdog:TEZ-4374

Conversation

@abstractdog

Copy link
Copy Markdown
Contributor

No description provided.

@abstractdogabstractdog changed the title TEZ-4374: Check compile-time compatibility with JDK 17TEZ-4374: Check compile-time compatibility with JDK 17 in precommitDec 26, 2024
@abstractdog

Copy link
Copy Markdown
ContributorAuthor

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

@tez-yetus

This comment was marked as outdated.

@abstractdog

Copy link
Copy Markdown
ContributorAuthor

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

@ayushtknayushtkn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@ayushtkn

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

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec23m 52sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+1 💚@author0m 0sThe patch does not contain any @author tags.
_ master Compile Tests _
_ Patch Compile Tests _
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚yamllint0m 0sThere were no new yamllint issues.
_ Other Tests _
+1 💚asflicense1m 31sThe patch does not generate ASF License warnings.
26m 19s
SubsystemReport/Notes
DockerClientAPI=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 IssueTEZ-4374
Optional Testsdupname asflicense yamllint
unameLinux 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 toolmaven
Personalitypersonality/tez.sh
git revisionmaster / c7a3791
Max. process+thread count51 (vs. ulimit of 5500)
modulesC: . U: .
Console outputhttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-386/2/console
versionsgit=2.34.1 maven=3.6.3 yamllint=1.24.2
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@abstractdog

abstractdog commented Dec 31, 2024

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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

abstractdog commented Dec 31, 2024

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
Member

Sounds cool

@abstractdogabstractdog changed the title TEZ-4374: Check compile-time compatibility with JDK 17 in precommitTEZ-4374: Enable github action to compile with JDK 17Dec 31, 2024
@abstractdog
abstractdog merged commit 1e6c9e3 into apache:masterDec 31, 2024
Sign up for freeto 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

@abstractdog@tez-yetus@ayushtkn