Skip to content
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

Merged
merged 17 commits into from
Jul 16, 2021
Merged

Add Gradle 7 support #1824

merged 17 commits into from
Jul 16, 2021

Conversation

fawind
Copy link
Contributor

@fawind fawind commented Jul 12, 2021

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==

@changelog-app
Copy link

changelog-app bot commented Jul 12, 2021

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Add Gradle 7 support. Increase minimum required Gradle version to 6.7.

Check the box to generate changelog(s)

  • Generate changelog entry

@fawind fawind requested a review from CRogers July 12, 2021 16:54
@@ -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());
Copy link
Contributor Author

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).

Copy link
Contributor

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?

Copy link
Contributor Author

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());
Copy link
Contributor Author

@fawind fawind Jul 12, 2021

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

@fawind fawind Jul 15, 2021

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 =
Copy link
Contributor Author

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?

Copy link
Contributor

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?

Copy link
Contributor Author

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");
Copy link
Contributor

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?

Copy link
Contributor Author

@fawind fawind Jul 15, 2021

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

lol

@@ -0,0 +1,5 @@
type: improvement
improvement:
description: Add Gradle 7 support
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@CRogers
Copy link
Contributor

CRogers commented Jul 16, 2021

👍

@fawind fawind marked this pull request as ready for review July 16, 2021 14:40
bulldozer-bot bot pushed a commit to palantir/witchcraft-api that referenced this pull request Jul 16, 2021
###### _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> {
Copy link
Contributor

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.

gradle-baseline/README.md

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).

Copy link
Contributor Author

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants