Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-1824.v2.yml
Original file line number Diff line number Diff line change
@@ -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.

links:
- https://github.com/palantir/gradle-baseline/pull/1824
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,6 @@ public void apply(Project project) {
proj.getPluginManager().apply(BaselineTestHeap.class);
proj.getPluginManager().apply(BaselineJavaParameters.class);
proj.getPluginManager().apply(BaselineImmutables.class);

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


});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Configuration compileClasspath =
project.getConfigurations().getByName(sourceSet.getCompileClasspathConfigurationName());

Configuration explicitCompile = project.getConfigurations()
.create("baseline-exact-dependencies-" + sourceSet.getName(), conf -> {
conf.setDescription(String.format(
"Tracks the explicit (not inherited) dependencies added to either %s or %s",
compile.toString(), implementation.toString()));
"Tracks the explicit (not inherited) dependencies added to either %s", implementation));
conf.setVisible(false);
conf.setCanBeConsumed(false);

Expand Down Expand Up @@ -137,18 +135,11 @@ private static void configureSourceSet(
// configurations (belonging to our source set)
project.afterEvaluate(p -> {
Configuration implCopy = implementation.copy();
Configuration compileCopy = compile.copy();
// Ensure it's not resolvable, otherwise plugins that resolve all configurations might have
// a bad time resolving this with GCV, if you have direct dependencies without corresponding entries in
// versions.props, but instead rely on getting a version for them from the lock file.
compileCopy.setCanBeResolved(false);
compileCopy.setCanBeConsumed(false);
// Without these, explicitCompile will successfully resolve 0 files and you'll waste 1 hour trying
// to figure out why.
project.getConfigurations().add(implCopy);
project.getConfigurations().add(compileCopy);

explicitCompile.extendsFrom(implCopy, compileCopy);
explicitCompile.extendsFrom(implCopy);
});

explicitCompile.withDependencies(deps -> {
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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 }

JavaVersion jdkVersion = JavaVersion.toVersion(javaCompile.getTargetCompatibility());
if (!supportsReleaseFlag(jdkVersion)) {
log.debug(
"BaselineReleaseCompatibility is a no-op for {} in {} as {} doesn't support --release",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.palantir.baseline.tasks;

import java.io.IOException;
import java.lang.reflect.Method;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.StandardOpenOption;
Expand All @@ -28,6 +29,8 @@
import org.gradle.api.Task;
import org.gradle.api.model.ObjectFactory;
import org.gradle.api.plugins.JavaPluginConvention;
import org.gradle.api.plugins.JavaPluginExtension;
import org.gradle.api.plugins.internal.DefaultJavaPluginExtension;
import org.gradle.api.provider.Property;
import org.gradle.api.publish.PublishingExtension;
import org.gradle.api.specs.Spec;
Expand Down Expand Up @@ -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.

(org.gradle.api.plugins.internal.DefaultJavaPluginConvention)
getProject().getConvention().getPlugin(JavaPluginConvention.class);

if (convention.getRawSourceCompatibility() != null) {
if (getRawSourceCompat() != null) {
// In theory, users could configure the fancy new 'java toolchain' as an alternative to explicit
// sourceCompatibility, but there's no method to access this yet (as of Gradle 6.8).
return;
Expand Down Expand Up @@ -127,4 +125,24 @@ public final void taskAction() throws IOException {
JavaVersion.current(),
getPath()));
}

private JavaVersion getRawSourceCompat() {
// TODO(fwindheuser): Remove internal api usage. Maybe through adopting toolchains?
// We're doing this naughty casting because we need access to the `getRawSourceCompatibility` method.
if (GradleVersion.current().compareTo(GradleVersion.version("7.0")) < 0) {
org.gradle.api.plugins.internal.DefaultJavaPluginConvention convention =
(org.gradle.api.plugins.internal.DefaultJavaPluginConvention)
getProject().getConvention().getPlugin(JavaPluginConvention.class);
return convention.getRawSourceCompatibility();
}

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.

return (JavaVersion) rawSourceCompat.invoke(extension);
} catch (Exception e) {
throw new RuntimeException("Error calling Gradle 7 getRawSourceCompatibility", e);
}
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class BaselineExactDependenciesTest extends AbstractPluginTest {
mavenCentral()
}
dependencies {
compile 'com.google.guava:guava:27.0.1-jre'
implementation 'com.google.guava:guava:27.0.1-jre'
}
"""
file('src/main/java/pkg/Foo.java') << minimalJavaFile
Expand Down Expand Up @@ -120,7 +120,7 @@ class BaselineExactDependenciesTest extends AbstractPluginTest {
buildFile << standardBuildFile
buildFile << """
dependencies {
compile project(':needs-building-first')
implementation project(':needs-building-first')
}
"""

Expand Down Expand Up @@ -156,7 +156,7 @@ class BaselineExactDependenciesTest extends AbstractPluginTest {
mavenCentral()
}
dependencies {
compile 'com.google.guava:guava:28.0-jre'
implementation 'com.google.guava:guava:28.0-jre'
}
"""
file('src/main/java/pkg/Foo.java') << '''
Expand All @@ -181,7 +181,7 @@ class BaselineExactDependenciesTest extends AbstractPluginTest {
mavenCentral()
}
dependencies {
compile 'com.fasterxml.jackson.datatype:jackson-datatype-guava:2.9.8' // pulls in guava transitively
implementation 'com.fasterxml.jackson.datatype:jackson-datatype-guava:2.9.8' // pulls in guava transitively
}
"""
file('src/main/java/pkg/Foo.java') << '''
Expand Down Expand Up @@ -248,7 +248,7 @@ class BaselineExactDependenciesTest extends AbstractPluginTest {

then:
BuildResult result = with(':checkUnusedDependencies', '--stacktrace').withDebug(true).buildAndFail()
result.output.contains "project(':sub-project-with-deps') (sub-project-with-deps.jar (project :sub-project-with-deps))"
result.output.contains "project(':sub-project-with-deps') (main (project :sub-project-with-deps))"
result.output.contains "project(':sub-project-no-deps')"
}

Expand All @@ -264,7 +264,7 @@ class BaselineExactDependenciesTest extends AbstractPluginTest {
}

/**
* Sets up a multi-module project with 2 sub projects. The root project has a transitive dependency on sub-project-no-deps
* Sets up a multi-module project with 2 sub projects. The root project has a transitive dependency on sub-project-no-deps
* and so checkImplicitDependencies should fail on it.
*/
private void setupMultiProject() {
Expand All @@ -275,16 +275,18 @@ class BaselineExactDependenciesTest extends AbstractPluginTest {
apply plugin: 'com.palantir.baseline-exact-dependencies'
}
dependencies {
compile project(':sub-project-with-deps')
implementation project(':sub-project-with-deps')
}
""".stripIndent()

def subProjects = multiProject.create(["sub-project-no-deps", "sub-project-with-deps"])

//properly declare dependency between two sub-projects
subProjects['sub-project-with-deps'].buildGradle << '''
apply plugin: 'java-library'

dependencies {
compile project(':sub-project-no-deps')
api project(':sub-project-no-deps')
}
'''.stripIndent()

Expand Down Expand Up @@ -323,6 +325,5 @@ class BaselineExactDependenciesTest extends AbstractPluginTest {
}
}
'''.stripIndent()

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.palantir.baseline

import spock.lang.Unroll

import java.nio.file.Files
import java.nio.file.Path
import org.apache.commons.io.FileUtils
Expand All @@ -27,6 +29,7 @@ import org.gradle.testkit.runner.TaskOutcome

import static org.assertj.core.api.Assertions.assertThat

@Unroll
class BaselineFormatIntegrationTest extends AbstractPluginTest {

def setup() {
Expand Down Expand Up @@ -89,7 +92,10 @@ class BaselineFormatIntegrationTest extends AbstractPluginTest {
buildFile << standardBuildFile

then:
with('format', '--stacktrace').build()
with('format', '--stacktrace').withGradleVersion(gradleVersion).build()

where:
gradleVersion << GradleTestVersions.VERSIONS
}

def 'eclipse formatter integration test'() {
Expand All @@ -108,12 +114,15 @@ class BaselineFormatIntegrationTest extends AbstractPluginTest {
file('gradle.properties') << "com.palantir.baseline-format.eclipse=true\n"

when:
BuildResult result = with(':format').build()
BuildResult result = with(':format').withGradleVersion(gradleVersion).build()
result.task(":format").outcome == TaskOutcome.SUCCESS
result.task(":spotlessApply").outcome == TaskOutcome.SUCCESS

then:
assertThatFilesAreTheSame(testedDir, expectedDir)

where:
gradleVersion << GradleTestVersions.VERSIONS
}

def 'palantir java format works'() {
Expand All @@ -138,12 +147,15 @@ class BaselineFormatIntegrationTest extends AbstractPluginTest {
file('gradle.properties') << "com.palantir.baseline-format.palantir-java-format=true\n"

when:
BuildResult result = with(':format').build()
BuildResult result = with(':format').withGradleVersion(gradleVersion).build()

then:
result.task(":format").outcome == TaskOutcome.SUCCESS
result.task(":spotlessApply").outcome == TaskOutcome.SUCCESS
assertThatFilesAreTheSame(testedDir, expectedDir)

where:
gradleVersion << GradleTestVersions.VERSIONS
}

private static void assertThatFilesAreTheSame(File outputDir, File expectedDir) throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,6 @@ class BaselineIntegrationTest extends AbstractPluginTest {
with().withArguments('-s').withGradleVersion(gradleVersion).build()

where:
gradleVersion << ['6.1', '6.2']
gradleVersion << GradleTestVersions.VERSIONS
}
}
Loading