-
Notifications
You must be signed in to change notification settings - Fork 135
baseline-java-versions: javaCompiler property to compile Java code with a certain JDK version
#3342
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
base: develop
Are you sure you want to change the base?
Conversation
Generate changelog in
|
|
|
||
| @Override | ||
| public final void setLibraryTarget(int _value) { | ||
| throw throwCannotSetFromSubproject(); |
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.
Hilariously this is throwing the result of a method that always throws.
compiler property to compile code with a certain JDK versioncompiler property to compile Java code with a certain JDK version
README.md
Outdated
| The configurable fields of the `javaVersions` extension are: | ||
| * `libraryTarget`: (required) The Java version used for compilation of libraries that are published. | ||
| * `distributionTarget`: (optional) The Java version used for compilation of code used within distributions, but not published externally. Defaults to the `libraryTarget` version. | ||
| * `compiler`: (required) The version of the Java compiler used. |
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 wonder if this should be javaCompiler to make clear this version is not being used for groovy/scala compilation
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.
Done
README.md
Outdated
| The configurable fields of the `javaVersions` extension are: | ||
| * `libraryTarget`: (required) The Java version used for compilation of libraries that are published. | ||
| * `distributionTarget`: (optional) The Java version used for compilation of code used within distributions, but not published externally. Defaults to the `libraryTarget` version. | ||
| * `compiler`: (required) The version of the Java compiler used. |
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 have made this required because if it is not, everything remains unset and Gradle defaults to using the daemon JDK version, which seems not ideal. But it does mean it always needs to be set, even if your project only has groovy/scala and no java.
| } | ||
|
|
||
| // In Gradle <8, we need to set sourceCompatibility to opt out of '-release', allowing opens/exports to be used. | ||
| // https://github.com/gradle/gradle/issues/18824#issuecomment-1026909824 |
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.
Pretty sure this is irrelevant after #3324. But really need some tests (currently the new tests do not even test against Gradle 7). I'm not even sure groovy/scala have --add-exports at compilation time.
| groovyCompileTask | ||
| .getOptions() | ||
| .getCompilerArgumentProviders() | ||
| .add(new EnablePreviewArgumentProvider(target)); |
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.
Unsure if this actually does anything for Groovy compile. Same for Scala. Needs more testing. Might need it to be able to compile against preview APIs (maybe use Java preview source features when compiling with Java files with Groovy?)
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 guess not really relevant for this PR.
compiler property to compile Java code with a certain JDK versionjavaCompiler property to compile Java code with a certain JDK version
| apply plugin: 'com.palantir.baseline-java-versions' | ||
| javaVersions { | ||
| javaCompiler = 25 |
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'll need to excavate this (javaCompiler = 21) out.
| proj.getPluginManager().apply(BaselineReproducibility.class); | ||
| proj.getPluginManager().apply(BaselineClassUniquenessPlugin.class); | ||
| proj.getPluginManager().apply(BaselineExactDependencies.class); | ||
| proj.getPluginManager().apply(BaselineReleaseCompatibility.class); |
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.
This would set --release on Java <9, but we always set --release now so it's no longer needed.
Before this PR
Currently when compiling Java code, if we want to target Java major version XX bytecode in class files, we'll use an XX Java compiler. We do not use the
--releaseflag (which would allow us to target a lower source/bytecode/jdk api from a higher versioned Java compiler) because it's is verboten to use--releasewith--add-exports, which happens a lot around our codebases.Error Prone 2.43.0 has made JDK 21 the minimum required JDK that Error Prone is run in. The recommendation is to use
--releaseso a higher version JDK can safely output java classes at a lower language level. However, we can't do this due to the aforementioned issue with--add-exports. Similarly, we're stuck on Java 17 libraries for now, so can't just upgrade everything to Java 21. This makes us stuck, unable to upgrade Error Prone.Luckily, in a previous PR I have removed the
--release/--add-exportsrestriction (it also has more details about the errorprone issue). There is nothing stopping us using a higher compiler and targeting a lower version using--release.After this PR
==COMMIT_MSG==
baseline-java-versions: Introduce a required
javaCompilerproperty tojavaVersionswhich will determine which JDK is used to run the Java compiler.--releaseis now used to target lower Java language versions/class file bytecode than the compiler.==COMMIT_MSG==
I've expanded the tests quite a lot.
Possible downsides?
compilerproperty as part of baseline upgrades.