-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Update minimum gradle version and remove assumption around target compatibility #18595
Conversation
@rjernst can you have a look please? This is working in my branch but I may have missed something :) |
options.forkOptions.memoryMaximumSize = "1g" | ||
if (targetCompatibility == '1.8' || targetCompatibility == '1.9') { | ||
options.fork = true | ||
options.forkOptions.executable = new File(project.javaHome, 'bin/javac') |
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.
I assume we will be using java 8 to compile java 7? In that case, i don't see why we need to conditionalize any of the fork options. It seems like it should only be the compact profile arg that is conditionalized below?
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.
you are right, I pushed a new commit to fix that
e008dfd
to
7979cb6
Compare
@@ -372,18 +372,20 @@ class BuildPlugin implements Plugin<Project> { | |||
options.fork = true | |||
options.forkOptions.executable = new File(project.javaHome, 'bin/javac') | |||
options.forkOptions.memoryMaximumSize = "1g" | |||
if (targetCompatibility == '1.8' || targetCompatibility == '1.9') { |
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.
We actually set targetCompatibility to a value from gradle's JavaVersion enum. So this comparison can be something like:
if (targetCompatibility >= JavaVersion.VERSION_1_8) {
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.
perfect, I was not sure about that, thanks for bringing that up
LGTM, I left one more minor request. |
f07b98a
to
28663b3
Compare
It came out with improvements around idea integration and language levels. This will make it possible to have the upcoming java client as a new project compiled against java 7 and have idea working on the right language level.
This change makes it possible to compile a separate project with e.g. targetCompatibility 1.7. Adds specific options (compact profile) only when compiling for >= 1.8.
28663b3
to
0e563b3
Compare
Up until now we assumed that we always compile with java 8 or java 9 and never with targetCompatibility lower than 1.8. This will be necessary with the upcoming java http client as it will be 1.7 compatible. We should then apply specific options only when using 1.8 or 1.9.
Also by upgrading the minimum required gradle version to 2.13 we better support subprojects that need to compile against different target versions as there were previously issues with the intellij integration around that.