From 6cb8bdb802c8a43c8e0ef81e8aff1077227175d1 Mon Sep 17 00:00:00 2001 From: Kelvin Ou Date: Thu, 12 Jun 2025 14:23:27 +0100 Subject: [PATCH] This should work but doesn't --- .../PalantirJavaFormatSpotlessPlugin.java | 10 ++ .../javaformat/gradle/SpotlessInterop.java | 18 ++-- .../NativePalantirJavaFormatStep.java | 27 +++--- .../spotless/PalantirJavaFormatStep.java | 27 ++++-- .../gradle/FormatterLazinessTest.groovy | 80 ++++++++++++++++ .../javaformat/gradle/GradlewExecutor.groovy | 96 +++++++++++++++++++ 6 files changed, 228 insertions(+), 30 deletions(-) create mode 100644 gradle-palantir-java-format/src/test/groovy/com/palantir/javaformat/gradle/FormatterLazinessTest.groovy create mode 100644 gradle-palantir-java-format/src/test/groovy/com/palantir/javaformat/gradle/GradlewExecutor.groovy diff --git a/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/PalantirJavaFormatSpotlessPlugin.java b/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/PalantirJavaFormatSpotlessPlugin.java index d091163af..78177a3c6 100644 --- a/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/PalantirJavaFormatSpotlessPlugin.java +++ b/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/PalantirJavaFormatSpotlessPlugin.java @@ -19,6 +19,10 @@ import org.gradle.api.Plugin; import org.gradle.api.Project; +/** + * Adds the spotless steps + * The formatters are lazily loaded + */ public class PalantirJavaFormatSpotlessPlugin implements Plugin { // The spotless gradle plugin got renamed to 'com.diffplug.spotless' at version 5.0.0 private static final ImmutableList SPOTLESS_PLUGINS = @@ -34,5 +38,11 @@ public void apply(Project project) { SpotlessInterop.addSpotlessJavaStep(project); })); }); + + // assertFormattersNotOnClasspath(); + } + + public void assertFormattersNotOnClasspath() { + SpotlessInterop.assertFormattersNotOnClasspath(); } } diff --git a/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/SpotlessInterop.java b/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/SpotlessInterop.java index 655ffc0fd..faa823339 100644 --- a/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/SpotlessInterop.java +++ b/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/SpotlessInterop.java @@ -51,12 +51,18 @@ static FormatterStep addSpotlessJavaFormatStep(Project project) { return NativePalantirJavaFormatStep.create(project.getRootProject() .getConfigurations() .getByName(NativeImageFormatProviderPlugin.NATIVE_CONFIGURATION_NAME)); + } else { + logger.info("Using the Java-based formatter {}", JavaVersion.current()); + return PalantirJavaFormatStep.create( + project.getRootProject() + .getConfigurations() + .getByName(PalantirJavaFormatProviderPlugin.CONFIGURATION_NAME), + project.getRootProject().getExtensions().getByType(JavaFormatExtension.class)); } - logger.info("Using the Java-based formatter {}", JavaVersion.current()); - return PalantirJavaFormatStep.create( - project.getRootProject() - .getConfigurations() - .getByName(PalantirJavaFormatProviderPlugin.CONFIGURATION_NAME), - project.getRootProject().getExtensions().getByType(JavaFormatExtension.class)); + } + + static void assertFormattersNotOnClasspath() { + // TODO(okelvin): is this needed for the native-image formatter as well? + PalantirJavaFormatStep.assertClassIsNotLoadable(); } } diff --git a/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/spotless/NativePalantirJavaFormatStep.java b/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/spotless/NativePalantirJavaFormatStep.java index 129c43939..c5bf85140 100644 --- a/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/spotless/NativePalantirJavaFormatStep.java +++ b/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/spotless/NativePalantirJavaFormatStep.java @@ -25,13 +25,13 @@ import java.io.Serializable; import java.nio.charset.StandardCharsets; import java.util.List; +import java.util.function.Supplier; import org.gradle.api.artifacts.Configuration; import org.gradle.api.logging.Logger; import org.gradle.api.logging.Logging; public final class NativePalantirJavaFormatStep { - @SuppressWarnings("for-rollout:NonFinalStaticField") - private static Logger logger = Logging.getLogger(NativePalantirJavaFormatStep.class); + private static final Logger logger = Logging.getLogger(NativePalantirJavaFormatStep.class); private NativePalantirJavaFormatStep() {} @@ -39,28 +39,23 @@ private NativePalantirJavaFormatStep() {} /** Creates a step which formats everything - code, import order, and unused imports. */ public static FormatterStep create(Configuration configuration) { - return FormatterStep.createLazy( - NAME, - () -> { - File execFile = configuration.getSingleFile(); - logger.info("Using native-image at {}", configuration.getSingleFile()); - return new State(FileSignature.signAsSet(execFile)); - }, - State::createFormat); + return FormatterStep.createLazy(NAME, () -> new State(configuration::getSingleFile), State::createFormat); } static class State implements Serializable { private static final long serialVersionUID = 1L; - final FileSignature pathToExe; + private final transient Supplier execSupplier; - State(FileSignature pathToExe) { - this.pathToExe = pathToExe; + State(Supplier supplier) { + this.execSupplier = supplier; } String format(ProcessRunner runner, String input) throws IOException, InterruptedException { - List argumentsWithPathToExe = - List.of(pathToExe.getOnlyFile().getAbsolutePath(), "--palantir", "-"); + File execFile = execSupplier.get(); + logger.info("Using native-image at {}", execFile); + File signedFile = FileSignature.signAsSet(execFile).getOnlyFile(); + List argumentsWithPathToExe = List.of(signedFile.getAbsolutePath(), "--palantir", "-"); return runner.exec(input.getBytes(StandardCharsets.UTF_8), argumentsWithPathToExe) .assertExitZero(StandardCharsets.UTF_8); } @@ -70,4 +65,4 @@ FormatterFunc.Closeable createFormat() { return FormatterFunc.Closeable.of(runner, this::format); } } -} +} \ No newline at end of file diff --git a/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/spotless/PalantirJavaFormatStep.java b/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/spotless/PalantirJavaFormatStep.java index 9d92fb783..da211efd9 100644 --- a/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/spotless/PalantirJavaFormatStep.java +++ b/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/spotless/PalantirJavaFormatStep.java @@ -36,10 +36,10 @@ private PalantirJavaFormatStep() {} /** Creates a step which formats everything - code, import order, and unused imports. */ public static FormatterStep create(Configuration palantirJavaFormat, JavaFormatExtension extension) { - ensureImplementationNotDirectlyLoadable(); + assertClassIsNotLoadable(); Supplier memoizedService = extension::serviceLoad; return FormatterStep.createLazy( - NAME, () -> new State(palantirJavaFormat.getFiles(), memoizedService), State::createFormat); + NAME, () -> new State(palantirJavaFormat::getFiles, memoizedService), State::createFormat); } static final class State implements Serializable { @@ -51,7 +51,9 @@ static final class State implements Serializable { // Kept for state serialization purposes. @SuppressWarnings({"unused", "FieldCanBeLocal"}) - private final FileSignature jarsSignature; + private FileSignature jarsSignature; + + private final transient Supplier> jarsSupplier; // Transient as this is not serializable. private final transient Supplier memoizedFormatter; @@ -59,22 +61,31 @@ static final class State implements Serializable { /** * Build a cacheable state for spotless from the given jars, that uses the given {@link FormatterService}. * - * @param jars The jars that contain the palantir-java-format implementation. This is only used for caching and + * @param jarsSupplier Supplies the jars that contain the palantir-java-format implementation. This is only used for caching and * up-to-dateness purposes. */ - State(Iterable jars, Supplier memoizedFormatter) throws IOException { - this.jarsSignature = FileSignature.signAsSet(jars); + State(Supplier> jarsSupplier, Supplier memoizedFormatter) { + this.jarsSupplier = jarsSupplier; this.memoizedFormatter = memoizedFormatter; } @SuppressWarnings("NullableProblems") FormatterFunc createFormat() { - return memoizedFormatter.get()::formatSourceReflowStringsAndFixImports; + return input -> { + try { + // Only resolve the jars and compute the signature at execution time! + Iterable jars = jarsSupplier.get(); + this.jarsSignature = FileSignature.signAsSet(jars); + return memoizedFormatter.get().formatSourceReflowStringsAndFixImports(input); + } catch (IOException e) { + throw new RuntimeException(e); + } + }; } } @SuppressWarnings("for-rollout:ThrowSpecificExceptions") - private static void ensureImplementationNotDirectlyLoadable() { + public static void assertClassIsNotLoadable() { try { PalantirJavaFormatStep.class.getClassLoader().loadClass(IMPL_CLASS); } catch (ClassNotFoundException e) { diff --git a/gradle-palantir-java-format/src/test/groovy/com/palantir/javaformat/gradle/FormatterLazinessTest.groovy b/gradle-palantir-java-format/src/test/groovy/com/palantir/javaformat/gradle/FormatterLazinessTest.groovy new file mode 100644 index 000000000..d83021052 --- /dev/null +++ b/gradle-palantir-java-format/src/test/groovy/com/palantir/javaformat/gradle/FormatterLazinessTest.groovy @@ -0,0 +1,80 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.palantir.javaformat.gradle + +import nebula.test.IntegrationTestKitSpec + + +class FormatterLazinessTest extends IntegrationTestKitSpec { + private static final CLASSPATH_FILE = new File("build/impl.classpath").absolutePath + + private GradlewExecutor executor + + def setup() { + definePluginOutsideOfPluginBlock = true + keepFiles = true + executor = new GradlewExecutor(projectDir) + } + + def "formatter is not loaded even if spotlessJava is realized lazily"() { + // language=Gradle + buildFile << """ + buildscript { + repositories { + maven { + url 'https://artifactory.palantir.build/artifactory/release-jar' + metadataSources { mavenPom(); ignoreGradleMetadataRedirection() } + } + } + + dependencies { + classpath 'com.diffplug.spotless:spotless-plugin-gradle:6.22.0' + } + } + + allprojects { + apply plugin: 'java' + apply plugin: 'com.diffplug.spotless' + apply plugin: 'com.palantir.java-format' + + repositories { + maven { url 'https://artifactory.palantir.build/artifactory/release-jar' } + } + } + + project.getTasks().getByName("spotlessJava") + """.stripIndent(true) + + file("versions.props") + file("versions.lock") + + runTasks('wrapper') + + buildFile << """ + dependencies { + palantirJavaFormat files(file("${CLASSPATH_FILE}").text.split(':')) + } + """.stripIndent() + + when: + def result = executor.runGradlewTasks('--info') + + then: + assert result.success + println(result.standardOutput) + result.standardOutput.contains('BUILD SUCCESSFUL') + } +} \ No newline at end of file diff --git a/gradle-palantir-java-format/src/test/groovy/com/palantir/javaformat/gradle/GradlewExecutor.groovy b/gradle-palantir-java-format/src/test/groovy/com/palantir/javaformat/gradle/GradlewExecutor.groovy new file mode 100644 index 000000000..bfd279456 --- /dev/null +++ b/gradle-palantir-java-format/src/test/groovy/com/palantir/javaformat/gradle/GradlewExecutor.groovy @@ -0,0 +1,96 @@ +/* + * (c) Copyright 2025 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.javaformat.gradle + +import nebula.test.functional.internal.classpath.ClasspathAddingInitScriptBuilder +import org.gradle.internal.impldep.org.eclipse.jgit.annotations.NonNull + +import java.nio.charset.StandardCharsets +import java.nio.file.Path +import java.util.concurrent.TimeUnit +import java.util.stream.Collectors +import java.util.stream.Stream + +/** + * IntegrationTestKitSpec currently [loads more than what it needs into the classpath](https://github.com/nebula-plugins/nebula-test/blob/c5d3af9004898276bde5c68da492c6b0b4c5facc/src/main/groovy/nebula/test/IntegrationTestKitBase.groovy#L136) + * This means the Formatter is loaded onto the classpath eagerly, [erroneously](https://github.com/palantir/palantir-java-format/blob/00b08d2f471d66382d6c4cd2d05f56b6bb546ad3/gradle-palantir-java-format/src/main/java/com/palantir/javaformat/gradle/spotless/PalantirJavaFormatStep.java#L83). + * As a workaround, let's use the classpath produced by Gradle Test Kit in plugin-under-test-metadata.properties + * This classpath only contains the dependencies required by the plugin, as well as the plugin itself. + * This means no more eager loading of Formatters onto the classpath, no more complaints from PalantirJavaFormatStep + */ +class GradlewExecutor { + private File projectDir + + GradlewExecutor(@NonNull File projectDir) { + this.projectDir = projectDir + } + + private static Iterable getBuildPluginClasspathInjector() { + return getPluginClasspathInjector(Path.of("../gradle-palantir-java-format/build/pluginUnderTestMetadata/plugin-under-test-metadata.properties")) + } + + private static Iterable 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 { new File(it) } + } + + GradlewExecutionResult runGradlewTasks(String... tasks) { + ProcessBuilder processBuilder = getProcessBuilder(tasks) + Process process = processBuilder.start() + String output = readAllInput(process.getInputStream()) + process.waitFor(1, TimeUnit.MINUTES) + GradlewExecutionResult result = new GradlewExecutionResult(process.exitValue(), output) + return result + } + + final class GradlewExecutionResult { + + final Boolean success + final String standardOutput + final Throwable failure + + GradlewExecutionResult(int exitValue, String output) { + this.success = exitValue == 0 + this.standardOutput = output + this.failure = exitValue != 0 ? new RuntimeException(String.format("Build failed with exitCode %s", exitValue)) : null + } + } + + private static String readAllInput(InputStream inputStream) { + try (Stream lines = + new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8)).lines()) { + return lines.collect(Collectors.joining("\n")); + } + } + + private ProcessBuilder getProcessBuilder(String... tasks) { + File initScript = new File(projectDir, "init.gradle") + ClasspathAddingInitScriptBuilder.build(initScript, getBuildPluginClasspathInjector().toList()) + List arguments = ["./gradlew", "--init-script", initScript.toPath().toString()] + Arrays.asList(tasks).forEach(arguments::add) + ProcessBuilder processBuilder = new ProcessBuilder() + .command(arguments) + .directory(projectDir) + .redirectErrorStream(true) + return processBuilder + } +} \ No newline at end of file