-
Notifications
You must be signed in to change notification settings - Fork 135
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
Add Gradle 7 support #1824
Add Gradle 7 support #1824
Conversation
Generate changelog in
|
@@ -89,15 +89,13 @@ private static void configureSourceSet( | |||
TaskProvider<CheckImplicitDependenciesParentTask> checkImplicitDependencies) { | |||
Configuration implementation = | |||
project.getConfigurations().getByName(sourceSet.getImplementationConfigurationName()); | |||
Configuration compile = project.getConfigurations().getByName(sourceSet.getCompileConfigurationName()); |
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 break is hard to work around. Ideally we would still get the compile configuration as an optional if present. However with Gradle 7, the getCompileConfigurationName
method no longer exists and the method for deriving the configuration name based on the task is private (code ref).
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.
Can we just reimplement that logic while we support Gradle 6?
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.
Yep, its actually quite the simple method. Added it for now.
@@ -56,8 +56,7 @@ private ReleaseFlagProvider(JavaCompile javaCompile) { | |||
|
|||
@Override | |||
public Iterable<String> asArguments() { | |||
JavaVersion jdkVersion = | |||
JavaVersion.toVersion(javaCompile.getToolChain().getVersion()); |
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.
The JavaCompile#getToolChain
method got removed and I am not sure what a good replacement is.
I replaced it with targetCompat
for now but they are not equivalent. Maybe we can use JavaVersion#current
here?
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.
Does something like:
Optional<JavaVersion> taskTarget = Optional.ofNullable(javaCompile
.getProject()
.getExtensions()
.getByType(JavaPluginExtension.class)
.getToolchain()
.getLanguageVersion()
.getOrNull())
.map(JavaLanguageVersion::asInt)
.map(JavaVersion::toVersion);
replace it exactly? Not sure if it's an exact replacement, but tests seem to pass at least.
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 think JavaPluginExtension#getToolchain
is just always empty unless your explicitly configure a toolchain in your build.gradle
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 think what we can do is use getToolchain().getLanguageVersion().orElseGet(JavaVersion::current)
. In this case we would use the toolchain jdk version if specified and otherwise fall back to the global java version. Pushed up this version for now.
Only thing is that the new getToolchain
methods only exists since Gradle 6.7. We can either use Gradle 6.7 as the new minimum required Gradle version for baseline or do something like this again:
if (Gradle 6.7+) { get new toolchain through reflection }
else { get old toolchain through reflection }
@@ -92,12 +95,7 @@ public final void setShouldFix(boolean value) { | |||
|
|||
@TaskAction | |||
public final void taskAction() throws IOException { | |||
// We're doing this naughty casting because we need access to the `getRawSourceCompatibility` method. | |||
org.gradle.api.plugins.internal.DefaultJavaPluginConvention convention = |
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.
The internal API got moved to a different class. Naively, we could fix this with reflection based on the runtime Gradle version but that doesn't sound sustainable. Maybe this is a good time to lean into toolchains?
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.
doesn't java plugin extension exist since gradle 4.10? Maybe we can standardise on that?
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.
The extension exists since then but the internal method getRawSourceCompatibility
that we abuse here got moved from the convention to the extension with Gradle 7. So we can't use the extension for versions before that.
I'll do a bit more digging. Still hoping we find a non-internal way of figuring out if sourceCompat is explicitly set. Another alternative would be to actually look into projects gradle files and fail of sourceCompat is not set, similar to the baseline error-prone rules.
DefaultJavaPluginExtension extension = | ||
(DefaultJavaPluginExtension) getProject().getExtensions().getByType(JavaPluginExtension.class); | ||
try { | ||
Method rawSourceCompat = DefaultJavaPluginExtension.class.getMethod("getRawSourceCompatibility"); |
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.
Grim, but I can't immediately think of way to do this better (since the gradle jar we build against is 6.x, this API does not exist, right?). When this repo upgrades to 7, we'll probably need to switch this up and make the convention version use reflection, right?
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.
Yep, then we have to do reflection for the old method. But this should fail loudly once we upgrade to build with Gradle 7 as the JavaPluginConvention#getRawSourceCompatibility
can no longer be found.
|
||
// TODO(dsanduleac): enable this when people's idea{} blocks no longer reference things like | ||
// configurations.integrationTestCompile | ||
// proj.getPluginManager().apply(BaselineFixGradleJava.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.
lol
changelog/@unreleased/pr-1824.v2.yml
Outdated
@@ -0,0 +1,5 @@ | |||
type: improvement | |||
improvement: | |||
description: Add Gradle 7 support |
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.
Do we require a new minimum gradle version now? If so, should probably make it a break.
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.
If we decide with this one then we would require Gradle 6.7.
If we avoid this compile dependency e.g. through reflection, I don't think we introduce a new min version.
👍 |
###### _excavator_ is a bot for automating changes across repositories. Changes produced by the roomba/latest-baseline-oss check. # Release Notes ## 3.99.0-rc1 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Add Gradle 7 support | palantir/gradle-baseline#1824 | ## 4.0.0 | Type | Description | Link | | ---- | ----------- | ---- | | Break | Add Gradle 7 support. Increase minimum required Gradle version to 6.7. | palantir/gradle-baseline#1824 | To enable or disable this check, please contact the maintainers of Excavator.
* | ||
* <p>This will probably break some plugins in the wild, but we'd rather deal with that now than later. | ||
*/ | ||
public final class BaselineFixGradleJava implements Plugin<Project> { |
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 should remove this in the readme.md as it is still mentioned and removal of this plugin caused excavator failure palantir/tritium#1147
* Where:
Build file '/home/circleci/project/build.gradle' line: 36
* What went wrong:
A problem occurred evaluating root project 'tritium'.
> Plugin with id 'com.palantir.baseline-fix-gradle-java' not found.
Lines 414 to 426 in d2b734a
## com.palantir.baseline-fix-gradle-java (off by default) | |
Fixes up all Java [SourceSets](https://docs.gradle.org/current/userguide/building_java_projects.html#sec:java_source_sets) | |
by marking their deprecated [configurations](https://docs.gradle.org/current/userguide/java_plugin.html#tab:configurations) | |
- `compile` and `runtime` - as well as the `compileOnly` configuration as not resolvable | |
(can't call resolve on them) and not consumable (can't be depended on from other projects). | |
See [here](https://docs.gradle.org/current/userguide/declaring_dependencies.html#sec:resolvable-consumable-configs) | |
for a more in-depth discussion on what these terms mean. By configuring them thusly, we are saying that these configurations | |
now fulfil the "Bucket of dependencies" role described in that document, as they should. | |
This will become the default in Gradle 7 and leaving these as they currently are can cause both unnecessary confusion | |
(users looking in `compile` instead of `compileClasspath`) and [random crashes](https://github.com/gradle/gradle/issues/11844#issuecomment-585219427). |
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.
Good flag, will follow up with a PR!
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.
Removed in gradle-baseline 4.0.0 per palantir/gradle-baseline#1824
After this PR
Add support for Gradle 7. This raises the minimum required Gradle version to 6.7.
==COMMIT_MSG==
Add Gradle 7 support
==COMMIT_MSG==