Skip to content

SpotlessApply uses by default the Java formatter #1243

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

Merged
merged 15 commits into from
May 15, 2025
Merged
19 changes: 19 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,25 @@ shortcut.

![Install plugin from disk](./docs/images/install_plugin_from_disk.png)

## Java 21 Support

In [1211](https://github.com/palantir/palantir-java-format/pull/1211) we shipped Java 21 support. In order to use the
Java 21 formatting capabilities, ensure that either:

- the Gradle daemon and the Intellij Project SDK are set to Java 21
- or that the gradle property `palantir.native.formatter=true`. This will run the formatter as a native image,
- independent of the Gradle daemon/Intellij project JDK version.

### Native image formatter

[This comment](https://github.com/palantir/palantir-java-format/issues/952#issuecomment-2575750610) explains why we
switched to a native image for the formatter. The startup time for the native image esp. in Intellij is >10x faster than
spinning up a new JVM process that does the formatting.
However, the throughput of running the native image for a large set of files (eg. running `./gradlew spotlessApply`) is
considerably slower (eg. 30s using the Java implementation vs 1m20s using the native image implementation). Therefore,
when running the formatter from `spotlessApply` we will default to using the Java implementation (if the Java version >= 21).


## Future works

- [ ] preserve [NON-NLS markers][] - these are comments that are used when implementing NLS internationalisation, and need to stay on the same line with the strings they come after.
Expand Down
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-1243.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: fix
fix:
description: SpotlessApply uses Java 21 version
links:
- https://github.com/palantir/palantir-java-format/pull/1243
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public void apply(Project rootProject) {
}

private static Optional<Configuration> maybeGetNativeImplConfiguration(Project rootProject) {
return NativeImageFormatProviderPlugin.shouldUseNativeImage(rootProject)
return NativeImageFormatProviderPlugin.isNativeImageConfigured(rootProject)
? Optional.of(rootProject
.getConfigurations()
.getByName(NativeImageFormatProviderPlugin.NATIVE_CONFIGURATION_NAME))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public void apply(Project project) {
// TODO(dfox): in the future we may want to offer a simple 'format' task so people don't need to use
// spotless to try out our formatter
project.getTasks().register("formatDiff", FormatDiffTask.class, task -> {
if (NativeImageFormatProviderPlugin.shouldUseNativeImage(project)) {
if (NativeImageFormatProviderPlugin.isNativeImageConfigured(project)) {
task.getNativeImage().fileProvider(getNativeImplConfiguration(project));
}
});
Expand Down Expand Up @@ -76,13 +76,13 @@ public FormatDiffTask() {
@TaskAction
public final void formatDiff() throws IOException, InterruptedException {
if (getNativeImage().isPresent()) {
log.info("Using the native-image to format");
log.info("Using the native-image formatter");
FormatDiff.formatDiff(
getProject().getProjectDir().toPath(),
new NativeImageFormatterService(
getNativeImage().get().getAsFile().toPath()));
} else {
log.info("Using legacy java formatter");
log.info("Using the Java-based formatter");
JavaFormatExtension extension =
getProject().getRootProject().getExtensions().getByType(JavaFormatExtension.class);
FormatterService formatterService = extension.serviceLoad();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public void apply(Project rootProject) {
rootProject == rootProject.getRootProject(),
"May only apply com.palantir.java-format-provider to the root project");

if (!shouldUseNativeImage(rootProject)) {
if (!isNativeImageConfigured(rootProject)) {
log.info("Skipping native image configuration as it is not supported on this platform");
return;
}
Expand Down Expand Up @@ -68,7 +68,7 @@ public void apply(Project rootProject) {
});
}

public static boolean shouldUseNativeImage(Project project) {
public static boolean isNativeImageConfigured(Project project) {
return isNativeImageSupported() && isNativeFlagEnabled(project);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.diffplug.spotless.FormatterStep;
import com.palantir.javaformat.gradle.spotless.NativePalantirJavaFormatStep;
import com.palantir.javaformat.gradle.spotless.PalantirJavaFormatStep;
import org.gradle.api.JavaVersion;
import org.gradle.api.Project;
import org.gradle.api.logging.Logger;
import org.gradle.api.logging.Logging;
Expand All @@ -38,13 +39,19 @@ static void addSpotlessJavaStep(Project project) {
}

static FormatterStep addSpotlessJavaFormatStep(Project project) {
if (NativeImageFormatProviderPlugin.shouldUseNativeImage(project)) {
logger.info("Using the native-image palantir-java-formatter");

if (NativeImageFormatProviderPlugin.isNativeImageConfigured(project)
// Native images have lower throughput than Java implementations. This logic gets called by the
// Gradle spotlessApply step, which formats a full project.
// If we are already running on java 21, then we can run the spotlessApply logic using the Java
// formatter. Otherwise, we need to run the native-image.
&& JavaVersion.current().compareTo(JavaVersion.VERSION_21) < 0) {
logger.info("Using the native-image formatter");
return NativePalantirJavaFormatStep.create(project.getRootProject()
.getConfigurations()
.getByName(NativeImageFormatProviderPlugin.NATIVE_CONFIGURATION_NAME));
}
logger.info("Using the legacy palantir-java-formatter");
logger.info("Using the Java-based formatter {}", JavaVersion.current());
return PalantirJavaFormatStep.create(
project.getRootProject()
.getConfigurations()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,8 @@ class PalantirJavaFormatPluginTest extends IntegrationTestKitSpec {
'''.stripIndent()

where:
extraGradleProperties | expectedOutput
"" | "Using legacy java formatter"
"palantir.native.formatter=true" | "Using the native-image to format"
extraGradleProperties | expectedOutput
"" | "Using the Java-based formatter"
"palantir.native.formatter=true" | "Using the native-image formatter"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ package com.palantir.javaformat.gradle
import nebula.test.IntegrationTestKitSpec
import spock.lang.Unroll

import java.nio.charset.StandardCharsets
import java.nio.file.Path
import java.util.stream.Collectors
import java.util.stream.Stream

class PalantirJavaFormatSpotlessPluginTest extends IntegrationTestKitSpec {
/** ./gradlew writeImplClasspath generates this file. */
private static final CLASSPATH_FILE = new File("build/impl.classpath").absolutePath
Expand All @@ -26,20 +31,61 @@ class PalantirJavaFormatSpotlessPluginTest extends IntegrationTestKitSpec {


@Unroll
def "formats with spotless when spotless is applied"(String extraGradleProperties, String expectedOutput) {
def "formats with spotless when spotless is applied"(String extraGradleProperties, String javaVersion, String expectedOutput) {
def extraDependencies = extraGradleProperties.isEmpty() ? "" : NATIVE_CONFIG
settingsFile << """
buildscript {
repositories {
mavenCentral() { metadataSources { mavenPom(); ignoreGradleMetadataRedirection() } }
gradlePluginPortal() { metadataSources { mavenPom(); ignoreGradleMetadataRedirection() } }
}
dependencies {
classpath 'com.palantir.gradle.jdks:gradle-jdks-settings:0.62.0'
}
}
apply plugin: 'com.palantir.jdks.settings'
""".stripIndent(true)

buildFile << """
buildscript {
repositories {
mavenCentral() { metadataSources { mavenPom(); ignoreGradleMetadataRedirection() } }
gradlePluginPortal() { metadataSources { mavenPom(); ignoreGradleMetadataRedirection() } }
}
dependencies {
classpath 'com.palantir.baseline:gradle-baseline-java:6.21.0'
classpath 'com.palantir.gradle.jdks:gradle-jdks:0.62.0'
classpath 'com.palantir.gradle.jdkslatest:gradle-jdks-latest:0.17.0'
classpath files(FILES)
}
}

// The 'com.diffplug.spotless:spotless-plugin-gradle' dependency is already added by palantir-java-format
plugins {
id 'java'
id 'com.palantir.java-format'
}

apply plugin: 'com.palantir.java-format'
apply plugin: 'com.palantir.baseline-java-versions'
apply plugin: 'com.palantir.jdks'
apply plugin: 'com.palantir.jdks.latest'

dependencies {
palantirJavaFormat files(file("${CLASSPATH_FILE}").text.split(':'))
EXTRA_CONFIGURATION
}
""".replace("EXTRA_CONFIGURATION", extraDependencies).stripIndent()

javaVersions {
libraryTarget = JAVA_VERSION

Choose a reason for hiding this comment

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

Can we just string interpolate here?

}

jdks {
daemonTarget = JAVA_VERSION
}

""".replace("FILES", getBuildPluginClasspathInjector().join(","))
.replace("JAVA_VERSION", javaVersion)
.replace("EXTRA_CONFIGURATION", extraDependencies).stripIndent()

// Add jvm args to allow spotless and formatter gradle plugins to run with Java 16+
file('gradle.properties') << """
Expand All @@ -48,6 +94,7 @@ class PalantirJavaFormatSpotlessPluginTest extends IntegrationTestKitSpec {
--add-exports jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED \
--add-exports jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED \
--add-exports jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED
palantir.jdk.setup.enabled=true
""".stripIndent()
file('gradle.properties') << extraGradleProperties

Expand All @@ -56,19 +103,58 @@ class PalantirJavaFormatSpotlessPluginTest extends IntegrationTestKitSpec {
""".stripIndent()

file('src/main/java/Main.java').text = invalidJavaFile
runTasks('wrapper')

when:
def result = runTasks('spotlessApply', '--info')
def result = runGradlewTasks('spotlessApply', '--info')

then:
result.output.contains(expectedOutput)
result.contains(expectedOutput)
file('src/main/java/Main.java').text == validJavaFile

where:
extraGradleProperties | expectedOutput
"" | "Using the legacy palantir-java-formatter"
"palantir.native.formatter=true" | "Using the native-image"
extraGradleProperties | javaVersion | expectedOutput
"" | 21 | "Using the Java-based formatter"
"palantir.native.formatter=true" | 21 | "Using the Java-based formatter"
"palantir.native.formatter=true" | 17 | "Using the native-image formatter"

}

private static Iterable<File> getBuildPluginClasspathInjector() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I move this to https://github.com/palantir/gradle-plugin-testing ? gradle-jdks needs to do the same thing in order to run on multiple Java versions.

Choose a reason for hiding this comment

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

I don't understand what it's supposed to do?

Choose a reason for hiding this comment

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

I suppose what I don't understand is why you need this at all? I thought nebula does this for you?

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 need to test how the task works against Java 17 and Java 21. So, I am using the gradle-jdks automanagement workflow to set-up the JDKs but then I also need to run ./gradlew instead of the classic nebula runTask . When using nebula runTask the plugin is injected automatically, but now I need to set it on my own. This is what getBuildPluginClasspathInjector does - it adds the files of the plugin to the test classpath.

I have the same setup in gradle-jdks for tests - so I was wondering if it makes sense to move this logic in the testing plugin.

Choose a reason for hiding this comment

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

oh, I thought the tooling api invoked gradlew, interesting!

you could move it to gradle-plugin-testing, it's a bit niche, but I don't see why not.

Copy link

@felixdesouza felixdesouza Apr 9, 2025

Choose a reason for hiding this comment

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

I think a nicer way to do this, is probably to reuse some of the stuff in Nebula, it makes your life a lot easier I think:

  • Use ClasspathAddingInitScriptBuilder to write the classpath in an init script form
  • include the init script when running gradlew. i.e. ./gradlew <task> -i init-script

I think the current approach - I also mentioned something similar in the testing plugin PRs - detracts from the test by adding a bunch of cruft that's not actually important to know for the test, but is unfortunately required for the test to pass.

Choose a reason for hiding this comment

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

It also means, that you can hide all of this behind a runGradlew method and not have to pollute the test with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 9cd9bc2

however, I still need to keep this method, if I use the ClassLoader to add all the files loaded by default - the test will fail here

Choose a reason for hiding this comment

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

An aside: I think you could probably ship this method + runTasksViaGradlew into gradle-testing-plugin or a new thing. It'd just look very similar to IntegrationSpec or something.

return getPluginClasspathInjector(Path.of("../gradle-palantir-java-format/build/pluginUnderTestMetadata/plugin-under-test-metadata.properties"))
}

private static Iterable<File> getPluginClasspathInjector(Path path) {
File propertiesFile = path.toFile()
Properties properties = new Properties()
propertiesFile.withInputStream { inputStream ->
properties.load(inputStream)
}
String classpath = properties.getProperty('implementation-classpath')
return classpath.split(File.pathSeparator).collect { "'" + it + "'" }
}

private String runGradlewTasks(String... tasks) {
ProcessBuilder processBuilder = getProcessBuilder(tasks)
Process process = processBuilder.start()
String output = readAllInput(process.getInputStream())
return output
}

public static String readAllInput(InputStream inputStream) {
try (Stream<String> lines =
new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8)).lines()) {
return lines.collect(Collectors.joining("\n"));
}
}

private ProcessBuilder getProcessBuilder(String... tasks) {
List<String> arguments = ["./gradlew"]
Arrays.asList(tasks).forEach(arguments::add)
ProcessBuilder processBuilder = new ProcessBuilder()
.command(arguments)
.directory(projectDir).redirectErrorStream(true)
return processBuilder
}

def validJavaFile = '''\
Expand Down