-
Notifications
You must be signed in to change notification settings - Fork 424
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
Conversation
this is supposed to fail until TEZ-4387 is not present on master |
This comment was marked as outdated.
This comment was marked as outdated.
c4e531c
to
dba321d
Compare
@ayushtkn: could you take a look? this activates Java17 compilation in precommit (only compliation, no tests yes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@abstractdog This is Runtime compatibility right? not Compile time, for compile time, we should change this to 17: Line 44 in c7a3791
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? |
🎊 +1 overall
This message was automatically generated. |
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 |
As of now the byte code would be for JDK 8 only to have the byte code for JDK-17 we need to change that to 17 this is the offcial doc: 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 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? do you agree with the above points? what title do you recommend? |
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
wdyt? |
Sounds cool |
No description provided.