From d130b6fc224a0ed14936ce8616e922fbcc100560 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sat, 6 Nov 2021 20:29:55 -0700 Subject: [PATCH 01/14] Spotless dependencies are now resolved per-project. This side-steps all the root-project synchronization problems we were having. I'm keeping the hooks we would need to go back to centralized dep resolution in the future in case we decide to go back to that for some reason. --- .../gradle/spotless/FormatExtension.java | 2 +- .../gradle/spotless/GradleProvisioner.java | 39 ---------------- .../spotless/RegisterDependenciesTask.java | 44 +++++-------------- 3 files changed, 12 insertions(+), 73 deletions(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java index 5c7b20b82f..6446cdc512 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java @@ -75,7 +75,7 @@ public FormatExtension(SpotlessExtension spotless) { } protected final Provisioner provisioner() { - return spotless.getRegisterDependenciesTask().rootProvisioner; + return GradleProvisioner.forProject(spotless.project); } private String formatName() { diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java index c9d5d5fdfb..b6a3481726 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java @@ -15,19 +15,14 @@ */ package com.diffplug.gradle.spotless; -import java.io.File; import java.util.Collection; -import java.util.HashMap; -import java.util.Map; import java.util.Objects; -import java.util.Set; import java.util.logging.Level; import java.util.logging.Logger; import org.gradle.api.Project; import org.gradle.api.artifacts.Configuration; -import com.diffplug.common.base.Preconditions; import com.diffplug.common.collect.ImmutableList; import com.diffplug.spotless.Provisioner; @@ -35,40 +30,6 @@ class GradleProvisioner { private GradleProvisioner() {} - /** The provisioner used for the root project. */ - static class RootProvisioner implements Provisioner { - private final Project rootProject; - private final Map> cache = new HashMap<>(); - - RootProvisioner(Project rootProject) { - Preconditions.checkArgument(rootProject == rootProject.getRootProject()); - this.rootProject = rootProject; - } - - @Override - public Set provisionWithTransitives(boolean withTransitives, Collection mavenCoordinates) { - Request req = new Request(withTransitives, mavenCoordinates); - Set result; - synchronized (cache) { - result = cache.get(req); - } - if (result != null) { - return result; - } else { - synchronized (cache) { - result = cache.get(req); - if (result != null) { - return result; - } else { - result = GradleProvisioner.forProject(rootProject).provisionWithTransitives(req.withTransitives, req.mavenCoords); - cache.put(req, result); - return result; - } - } - } - } - } - static Provisioner forProject(Project project) { Objects.requireNonNull(project); return (withTransitives, mavenCoords) -> { diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/RegisterDependenciesTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/RegisterDependenciesTask.java index dc435a1f12..4b6b8699c7 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/RegisterDependenciesTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/RegisterDependenciesTask.java @@ -18,16 +18,12 @@ import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; -import java.util.ArrayList; -import java.util.List; import javax.inject.Inject; import org.gradle.api.DefaultTask; -import org.gradle.api.execution.TaskExecutionGraph; import org.gradle.api.provider.Property; import org.gradle.api.provider.Provider; -import org.gradle.api.tasks.Input; import org.gradle.api.tasks.Internal; import org.gradle.api.tasks.OutputFile; import org.gradle.api.tasks.TaskAction; @@ -35,7 +31,6 @@ import com.diffplug.common.base.Preconditions; import com.diffplug.common.io.Files; -import com.diffplug.spotless.FormatterStep; /** * NOT AN END-USER TASK, DO NOT USE FOR ANYTHING! @@ -50,26 +45,17 @@ public abstract class RegisterDependenciesTask extends DefaultTask { static final String TASK_NAME = "spotlessInternalRegisterDependencies"; - @Input - public List getSteps() { - List allSteps = new ArrayList<>(); - TaskExecutionGraph taskGraph = getProject().getGradle().getTaskGraph(); - tasks.stream() - .filter(taskGraph::hasTask) - .sorted() - .forEach(task -> allSteps.addAll(task.getSteps())); - return allSteps; - } - - private List tasks = new ArrayList<>(); - - @Internal - public List getTasks() { - return tasks; - } - void hookSubprojectTask(SpotlessTask task) { - tasks.add(task); + // TODO: in the future, we might use this hook to add an optional perf improvement + // spotlessRoot { + // java { googleJavaFormat('1.2') } + // ...etc + // } + // The point would be to reuse configurations from the root project, + // with the restriction that you have to declare every formatter in + // the root, and you'd get an error if you used a formatter somewhere + // which you didn't declare in the root. That's a problem for the future + // though, not today! task.dependsOn(this); } @@ -80,17 +66,9 @@ public File getUnitOutput() { return unitOutput; } - GradleProvisioner.RootProvisioner rootProvisioner; - - @Internal - public GradleProvisioner.RootProvisioner getRootProvisioner() { - return rootProvisioner; - } - void setup() { Preconditions.checkArgument(getProject().getRootProject() == getProject(), "Can only be used on the root project"); unitOutput = new File(getProject().getBuildDir(), "tmp/spotless-register-dependencies"); - rootProvisioner = new GradleProvisioner.RootProvisioner(getProject()); Provider gitRatchetProvider = getProject().getGradle().getSharedServices().registerIfAbsent("GitRatchetGradle", GitRatchetGradle.class, unused -> {}); getBuildEventsListenerRegistry().onTaskCompletion(gitRatchetProvider); getGitRatchet().set(gitRatchetProvider); @@ -99,7 +77,7 @@ void setup() { @TaskAction public void trivialFunction() throws IOException { Files.createParentDirs(unitOutput); - Files.write(Integer.toString(getSteps().size()), unitOutput, StandardCharsets.UTF_8); + Files.write(Integer.toString(1), unitOutput, StandardCharsets.UTF_8); } @Internal From afc1ff442108708392d3b24a0608c80d8cb59d64 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sat, 6 Nov 2021 20:33:50 -0700 Subject: [PATCH 02/14] Update changelog. --- plugin-gradle/CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin-gradle/CHANGES.md b/plugin-gradle/CHANGES.md index f3c72a325e..8aeccf8b88 100644 --- a/plugin-gradle/CHANGES.md +++ b/plugin-gradle/CHANGES.md @@ -4,7 +4,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format ( ## [Unreleased] ### Changed -* **BREAKING** Previously, many projects required `buildscript { repositories { mavenCentral() }}` at the top of their root project, because Spotless resolved its dependencies using the buildscript repositories. Spotless now resolves its dependencies from the normal project repositories of the root project, which means that you can remove the `buildscript {}` block, but you still need `repositories { mavenCentral() }` (or similar) in the root project. +* **BREAKING** Previously, many projects required `buildscript { repositories { mavenCentral() }}` at the top of their root project, because Spotless resolved its dependencies using the buildscript repositories. Spotless now resolves its dependencies from the normal project repositories of each project with a `spotless {...}` block. This means that you can remove the `buildscript {}` block, but you still need a `repositories { mavenCentral() }` (or similar) in each project which is using Spotless. * **BREAKING** `createIndepentApplyTask(String taskName)` now requires that `taskName` does not end with `Apply` * Bump minimum required Gradle from `6.1` to `6.1.1`. From 471b1f19bc87ae59899d45675630a9e7f6131c72 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sat, 6 Nov 2021 20:43:42 -0700 Subject: [PATCH 03/14] Put the SpotlessTaskService into FormatExtension where it belongs. --- .../diffplug/gradle/spotless/FormatExtension.java | 2 +- .../gradle/spotless/RegisterDependenciesTask.java | 13 +++++++++---- .../diffplug/gradle/spotless/SpotlessExtension.java | 3 --- .../gradle/spotless/SpotlessExtensionImpl.java | 11 +---------- 4 files changed, 11 insertions(+), 18 deletions(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java index 6446cdc512..b0bebd4a52 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java @@ -763,7 +763,7 @@ public SpotlessApply createIndependentApplyTask(String taskName) { Preconditions.checkArgument(!taskName.endsWith(SpotlessExtension.APPLY), "Task name must not end with " + SpotlessExtension.APPLY); // create and setup the task SpotlessTaskImpl spotlessTask = spotless.project.getTasks().create(taskName + SpotlessTaskService.INDEPENDENT_HELPER, SpotlessTaskImpl.class); - spotlessTask.init(spotless.getTaskService()); + spotlessTask.init(spotless.getRegisterDependenciesTask().getTaskService()); setupTask(spotlessTask); // enforce the clean ordering Task clean = spotless.project.getTasks().getByName(BasePlugin.CLEAN_TASK_NAME); diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/RegisterDependenciesTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/RegisterDependenciesTask.java index 4b6b8699c7..e8efc8fbfa 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/RegisterDependenciesTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/RegisterDependenciesTask.java @@ -23,7 +23,7 @@ import org.gradle.api.DefaultTask; import org.gradle.api.provider.Property; -import org.gradle.api.provider.Provider; +import org.gradle.api.services.BuildServiceRegistry; import org.gradle.api.tasks.Internal; import org.gradle.api.tasks.OutputFile; import org.gradle.api.tasks.TaskAction; @@ -69,9 +69,11 @@ public File getUnitOutput() { void setup() { Preconditions.checkArgument(getProject().getRootProject() == getProject(), "Can only be used on the root project"); unitOutput = new File(getProject().getBuildDir(), "tmp/spotless-register-dependencies"); - Provider gitRatchetProvider = getProject().getGradle().getSharedServices().registerIfAbsent("GitRatchetGradle", GitRatchetGradle.class, unused -> {}); - getBuildEventsListenerRegistry().onTaskCompletion(gitRatchetProvider); - getGitRatchet().set(gitRatchetProvider); + + BuildServiceRegistry buildServices = getProject().getGradle().getSharedServices(); + getTaskService().set(buildServices.registerIfAbsent("SpotlessTaskService", SpotlessTaskService.class, spec -> {})); + getGitRatchet().set(buildServices.registerIfAbsent("GitRatchetGradle", GitRatchetGradle.class, unused -> {})); + getBuildEventsListenerRegistry().onTaskCompletion(getGitRatchet()); } @TaskAction @@ -80,6 +82,9 @@ public void trivialFunction() throws IOException { Files.write(Integer.toString(1), unitOutput, StandardCharsets.UTF_8); } + @Internal + public abstract Property getTaskService(); + @Internal public abstract Property getGitRatchet(); diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java index d89acdd061..7959bdc71d 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java @@ -27,7 +27,6 @@ import org.gradle.api.Action; import org.gradle.api.GradleException; import org.gradle.api.Project; -import org.gradle.api.provider.Provider; import com.diffplug.spotless.LineEnding; @@ -47,8 +46,6 @@ protected SpotlessExtension(Project project) { this.project = requireNonNull(project); } - abstract Provider getTaskService(); - abstract RegisterDependenciesTask getRegisterDependenciesTask(); /** Line endings (if any). */ diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionImpl.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionImpl.java index 1a2a66422a..d55521c681 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionImpl.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionImpl.java @@ -19,13 +19,11 @@ import org.gradle.api.Project; import org.gradle.api.plugins.BasePlugin; import org.gradle.api.plugins.JavaBasePlugin; -import org.gradle.api.provider.Provider; import org.gradle.api.tasks.TaskContainer; import org.gradle.api.tasks.TaskProvider; public class SpotlessExtensionImpl extends SpotlessExtension { private final TaskProvider registerDependenciesTask; - private final Provider taskService; public SpotlessExtensionImpl(Project project) { super(project); @@ -54,13 +52,6 @@ public SpotlessExtensionImpl(Project project) { .configure(task -> task.dependsOn(rootCheckTask)); } }); - - taskService = project.getGradle().getSharedServices().registerIfAbsent("SpotlessTaskService", SpotlessTaskService.class, spec -> {}); - } - - @Override - Provider getTaskService() { - return taskService; } final TaskProvider rootCheckTask, rootApplyTask, rootDiagnoseTask; @@ -78,7 +69,7 @@ protected void createFormatTasks(String name, FormatExtension formatExtension) { // create the SpotlessTask String taskName = EXTENSION + SpotlessPlugin.capitalize(name); TaskProvider spotlessTask = tasks.register(taskName, SpotlessTaskImpl.class, task -> { - task.init(taskService); + task.init(getRegisterDependenciesTask().getTaskService()); task.setEnabled(!isIdeHook); // clean removes the SpotlessCache, so we have to run after clean task.mustRunAfter(cleanTask); From 1864623b654727ed3e8b1f617c759aba256a1742 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sat, 6 Nov 2021 21:02:01 -0700 Subject: [PATCH 04/14] Adjust test-cases now that repositories are per-project. --- .../gradle/spotless/MultiProjectAfterEvaluate.java | 2 +- .../gradle/spotless/RegisterDependenciesTaskTest.java | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/MultiProjectAfterEvaluate.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/MultiProjectAfterEvaluate.java index 47714860de..1e31c6a2ab 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/MultiProjectAfterEvaluate.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/MultiProjectAfterEvaluate.java @@ -25,13 +25,13 @@ class MultiProjectAfterEvaluate extends GradleIntegrationHarness { @Test void failureDoesntTriggerAll() throws IOException { setFile("settings.gradle").toLines("include 'sub'"); - setFile("build.gradle").toLines("repositories { mavenCentral() }"); setFile("sub/build.gradle") .toLines( "plugins {", " id 'com.diffplug.spotless'", " id 'java'", "}", + "repositories { mavenCentral() }", "spotless { java { googleJavaFormat() } }"); String output = gradleRunner().withArguments("spotlessApply", "--warning-mode", "all").build().getOutput().replace("\r\n", "\n"); Assertions.assertThat(output).doesNotContain("Using method Project#afterEvaluate(Action) when the project is already evaluated has been deprecated."); diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/RegisterDependenciesTaskTest.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/RegisterDependenciesTaskTest.java index 3f5fa30c9b..e1d8971a12 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/RegisterDependenciesTaskTest.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/RegisterDependenciesTaskTest.java @@ -25,12 +25,10 @@ class RegisterDependenciesTaskTest extends GradleIntegrationHarness { void registerDependencies() throws IOException { setFile("settings.gradle") .toLines("include 'sub'"); - setFile("build.gradle").toLines( - "plugins { id 'com.diffplug.spotless' }", - "repositories { mavenCentral() }"); setFile("sub/build.gradle").toLines( - "apply plugin: 'com.diffplug.spotless'", + "plugins { id 'com.diffplug.spotless' }", "", + "repositories { mavenCentral() }", "spotless {", " java {", " target 'src/main/java/**/*.java'", @@ -41,8 +39,7 @@ void registerDependencies() throws IOException { setFile("gradle.properties").toLines(); String newestSupported = gradleRunner().withArguments("spotlessCheck").build().getOutput(); Assertions.assertThat(newestSupported.replace("\r", "")) - .startsWith("> Task :spotlessCheck UP-TO-DATE\n" + - "> Task :spotlessInternalRegisterDependencies\n") + .startsWith("> Task :spotlessInternalRegisterDependencies\n") .contains("> Task :sub:spotlessJava\n" + "> Task :sub:spotlessJavaCheck\n" + "> Task :sub:spotlessCheck\n"); From 2ca0db16d0cec720bf192e91fb91cbc5ea41a454 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sat, 6 Nov 2021 21:02:44 -0700 Subject: [PATCH 05/14] Improve the error message for our new multi-resolution world. --- .../com/diffplug/gradle/spotless/GradleProvisioner.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java index b6a3481726..f81d405f95 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java @@ -43,10 +43,13 @@ static Provisioner forProject(Project project) { config.setTransitive(withTransitives); return config.resolve(); } catch (Exception e) { - String projName = project.getPath(); + String projName = project.getPath().substring(1).replace(':', '/'); + if (!projName.isEmpty()) { + projName = projName + "/"; + } logger.log( Level.SEVERE, - "You probably need to add a repository containing the '" + mavenCoords + "' artifact in the 'build.gradle' of the " + projName + " project.\n" + + "You need to add a repository containing the '" + mavenCoords + "' artifact in '" + projName + "build.gradle'.\n" + "E.g.: 'repositories { mavenCentral() }'", e); throw e; From 59b6f5d4452b7b8a4624b66b1ae2c497823cd83f Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sat, 6 Nov 2021 22:55:05 -0700 Subject: [PATCH 06/14] Create a failing testcase due to multiple configs with the same content in the same project. --- .../gradle/spotless/RegisterDependenciesTaskTest.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/RegisterDependenciesTaskTest.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/RegisterDependenciesTaskTest.java index e1d8971a12..b0da73e685 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/RegisterDependenciesTaskTest.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/RegisterDependenciesTaskTest.java @@ -22,7 +22,7 @@ class RegisterDependenciesTaskTest extends GradleIntegrationHarness { @Test - void registerDependencies() throws IOException { + void duplicateConfigs() throws IOException { setFile("settings.gradle") .toLines("include 'sub'"); setFile("sub/build.gradle").toLines( @@ -34,6 +34,10 @@ void registerDependencies() throws IOException { " target 'src/main/java/**/*.java'", " googleJavaFormat('1.2')", " }", + " format 'javaDupe', com.diffplug.gradle.spotless.JavaExtension, {", + " target 'src/boop/java/**/*.java'", + " googleJavaFormat('1.2')", + " }", "}"); setFile("gradle.properties").toLines(); From 9164ac1ec04b7eca40d8654a6c74516d94698e32 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sat, 6 Nov 2021 23:45:13 -0700 Subject: [PATCH 07/14] Fix the failing testcase. --- .../gradle/spotless/FormatExtension.java | 2 +- .../gradle/spotless/GradleProvisioner.java | 37 ++++++++++++++++++- .../gradle/spotless/SpotlessTaskService.java | 9 +++++ .../RegisterDependenciesTaskTest.java | 10 +++-- 4 files changed, 53 insertions(+), 5 deletions(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java index b0bebd4a52..5b43a2279f 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java @@ -75,7 +75,7 @@ public FormatExtension(SpotlessExtension spotless) { } protected final Provisioner provisioner() { - return GradleProvisioner.forProject(spotless.project); + return spotless.getRegisterDependenciesTask().getTaskService().get().provisionerFor(spotless.project); } private String formatName() { diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java index f81d405f95..4389ae10cd 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java @@ -15,8 +15,12 @@ */ package com.diffplug.gradle.spotless; +import java.io.File; import java.util.Collection; +import java.util.HashMap; +import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.logging.Level; import java.util.logging.Logger; @@ -30,7 +34,38 @@ class GradleProvisioner { private GradleProvisioner() {} - static Provisioner forProject(Project project) { + static Provisioner newDedupingProvisioner(Project project) { + return new DedupingProvisioner(project); + } + + static class DedupingProvisioner implements Provisioner { + private final Project project; + private final Map> cache = new HashMap<>(); + + DedupingProvisioner(Project project) { + this.project = project; + } + + @Override + public Set provisionWithTransitives(boolean withTransitives, Collection mavenCoordinates) { + Request req = new Request(withTransitives, mavenCoordinates); + Set result = cache.get(req); + if (result != null) { + return result; + } else { + result = cache.get(req); + if (result != null) { + return result; + } else { + result = forProject(project).provisionWithTransitives(req.withTransitives, req.mavenCoords); + cache.put(req, result); + return result; + } + } + } + } + + private static Provisioner forProject(Project project) { Objects.requireNonNull(project); return (withTransitives, mavenCoords) -> { try { diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java index a08e93bd12..5026a86536 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java @@ -23,6 +23,7 @@ import javax.inject.Inject; import org.gradle.api.DefaultTask; +import org.gradle.api.Project; import org.gradle.api.file.DirectoryProperty; import org.gradle.api.model.ObjectFactory; import org.gradle.api.provider.Property; @@ -32,6 +33,7 @@ import com.diffplug.common.base.Preconditions; import com.diffplug.common.base.Unhandled; +import com.diffplug.spotless.Provisioner; /** * Allows the check and apply tasks to coordinate @@ -42,6 +44,13 @@ public abstract class SpotlessTaskService implements BuildService { private final Map apply = Collections.synchronizedMap(new HashMap<>()); private final Map source = Collections.synchronizedMap(new HashMap<>()); + private final Map provisioner = Collections.synchronizedMap(new HashMap<>()); + + public Provisioner provisionerFor(Project project) { + return provisioner.computeIfAbsent(project.getPath(), unused -> { + return GradleProvisioner.newDedupingProvisioner(project); + }); + } public void registerSourceAlreadyRan(SpotlessTask task) { source.put(task.getPath(), task); diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/RegisterDependenciesTaskTest.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/RegisterDependenciesTaskTest.java index b0da73e685..480de4f01b 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/RegisterDependenciesTaskTest.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/RegisterDependenciesTaskTest.java @@ -43,9 +43,13 @@ void duplicateConfigs() throws IOException { setFile("gradle.properties").toLines(); String newestSupported = gradleRunner().withArguments("spotlessCheck").build().getOutput(); Assertions.assertThat(newestSupported.replace("\r", "")) - .startsWith("> Task :spotlessInternalRegisterDependencies\n") - .contains("> Task :sub:spotlessJava\n" + - "> Task :sub:spotlessJavaCheck\n" + + .startsWith( + "> Task :spotlessInternalRegisterDependencies\n") + .contains( + "> Task :sub:spotlessJava\n", + "> Task :sub:spotlessJavaCheck\n", + "> Task :sub:spotlessJavaDupe\n", + "> Task :sub:spotlessJavaDupeCheck\n", "> Task :sub:spotlessCheck\n"); } } From d322c6390704cb6e938043c2beb46ff83f74e56e Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sun, 7 Nov 2021 20:08:37 -0800 Subject: [PATCH 08/14] We now fail-fast if the user tries to use configuration-cache. --- .../gradle/spotless/SpotlessTaskImpl.java | 21 +++++---- .../gradle/spotless/SpotlessTaskService.java | 15 +++++-- .../spotless/ConfigurationCacheTest.java | 45 +++++++++++++------ .../spotless/GradleIntegrationHarness.java | 2 +- 4 files changed, 58 insertions(+), 25 deletions(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java index 2fc37fd741..1073f282a2 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java @@ -50,6 +50,7 @@ public abstract class SpotlessTaskImpl extends SpotlessTask { void init(Provider service) { getTaskService().set(service); getProjectDir().set(getProject().getProjectDir()); + service.get().registerSourceCreated(this); } @Inject @@ -68,17 +69,21 @@ public void performAction(InputChanges inputs) throws Exception { Files.createDirectories(outputDirectory.toPath()); } - try (Formatter formatter = buildFormatter()) { - for (FileChange fileChange : inputs.getFileChanges(target)) { - File input = fileChange.getFile(); - if (fileChange.getChangeType() == ChangeType.REMOVED) { - deletePreviousResult(input); - } else { - if (input.isFile()) { - processInputFile(formatter, input); + if (getTaskService().get().sourceWasCreatedThisBuild(this)) { + try (Formatter formatter = buildFormatter()) { + for (FileChange fileChange : inputs.getFileChanges(target)) { + File input = fileChange.getFile(); + if (fileChange.getChangeType() == ChangeType.REMOVED) { + deletePreviousResult(input); + } else { + if (input.isFile()) { + processInputFile(formatter, input); + } } } } + } else { + throw new GradleException("Spotless doesn't support configuration cache yet"); } } diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java index 5026a86536..c616e9ce19 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java @@ -42,21 +42,30 @@ * apply already did). */ public abstract class SpotlessTaskService implements BuildService { + private final Map sourceCreated = Collections.synchronizedMap(new HashMap<>()); private final Map apply = Collections.synchronizedMap(new HashMap<>()); private final Map source = Collections.synchronizedMap(new HashMap<>()); private final Map provisioner = Collections.synchronizedMap(new HashMap<>()); - public Provisioner provisionerFor(Project project) { + Provisioner provisionerFor(Project project) { return provisioner.computeIfAbsent(project.getPath(), unused -> { return GradleProvisioner.newDedupingProvisioner(project); }); } - public void registerSourceAlreadyRan(SpotlessTask task) { + void registerSourceCreated(SpotlessTask spotlessTask) { + sourceCreated.put(spotlessTask.getPath(), spotlessTask); + } + + boolean sourceWasCreatedThisBuild(SpotlessTask spotlessTask) { + return sourceCreated.containsKey(spotlessTask.getPath()); + } + + void registerSourceAlreadyRan(SpotlessTask task) { source.put(task.getPath(), task); } - public void registerApplyAlreadyRan(SpotlessApply task) { + void registerApplyAlreadyRan(SpotlessApply task) { apply.put(task.sourceTaskPath(), task); } diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/ConfigurationCacheTest.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/ConfigurationCacheTest.java index d8fd16023a..57c316e59b 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/ConfigurationCacheTest.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/ConfigurationCacheTest.java @@ -16,22 +16,16 @@ package com.diffplug.gradle.spotless; import java.io.IOException; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; +import org.gradle.testkit.runner.BuildResult; +import org.gradle.testkit.runner.GradleRunner; import org.junit.jupiter.api.Test; public class ConfigurationCacheTest extends GradleIntegrationHarness { - protected void runTasks(String... tasks) throws IOException { + @Override + protected GradleRunner gradleRunner() throws IOException { setFile("gradle.properties").toContent("org.gradle.unsafe.configuration-cache=true"); - List args = new ArrayList<>(); - args.addAll(Arrays.asList(tasks)); - gradleRunner() - .withGradleVersion(GradleVersionSupport.CONFIGURATION_CACHE.version) - .withArguments(args) - .forwardOutput() - .build(); + return super.gradleRunner().withGradleVersion(GradleVersionSupport.CONFIGURATION_CACHE.version); } @Test @@ -47,7 +41,7 @@ public void helpConfigures() throws IOException { " googleJavaFormat('1.2')", " }", "}"); - runTasks("help"); + gradleRunner().withArguments("help").build(); } @Test @@ -64,6 +58,31 @@ public void helpConfiguresIfTasksAreCreated() throws IOException { " }", "}", "tasks.named('spotlessJavaApply').get()"); - runTasks("help"); + gradleRunner().withArguments("help").build(); + } + + @Test + public void gjf() throws IOException { + setFile("build.gradle").toLines( + "plugins {", + " id 'com.diffplug.spotless'", + "}", + "repositories { mavenCentral() }", + "apply plugin: 'java'", + "spotless {", + " java {", + " target file('test.java')", + " googleJavaFormat('1.2')", + " }", + "}"); + + // first run works + setFile("test.java").toResource("java/googlejavaformat/JavaCodeUnformatted.test"); + gradleRunner().withArguments("spotlessApply").build(); + assertFile("test.java").sameAsResource("java/googlejavaformat/JavaCodeFormatted.test"); + + // but the second fails + BuildResult failure = gradleRunner().withArguments("spotlessApply").buildAndFail(); + failure.getOutput().contains("> Spotless doesn't support configuration cache yet"); } } diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/GradleIntegrationHarness.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/GradleIntegrationHarness.java index 89f7f25ffe..8d76850488 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/GradleIntegrationHarness.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/GradleIntegrationHarness.java @@ -83,7 +83,7 @@ void gitAttributes() throws IOException { setFile(".gitattributes").toContent("* text eol=lf"); } - protected final GradleRunner gradleRunner() throws IOException { + protected GradleRunner gradleRunner() throws IOException { return GradleRunner.create() .withGradleVersion(GradleVersionSupport.MINIMUM.version) .withProjectDir(rootFolder()) From f828147ff08e5173f43e06427b74bba364786795 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sun, 7 Nov 2021 22:00:19 -0800 Subject: [PATCH 09/14] Expand test coverage for the configuration cache, and better error message for failures. --- .../gradle/spotless/SpotlessTaskImpl.java | 3 ++- .../spotless/ConfigurationCacheTest.java | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java index 1073f282a2..37991c92d5 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java @@ -83,7 +83,8 @@ public void performAction(InputChanges inputs) throws Exception { } } } else { - throw new GradleException("Spotless doesn't support configuration cache yet"); + throw new GradleException("Spotless doesn't support configuration cache yet.\n" + + "Rerun with --no-configuration-cache"); } } diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/ConfigurationCacheTest.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/ConfigurationCacheTest.java index 57c316e59b..ae90930417 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/ConfigurationCacheTest.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/ConfigurationCacheTest.java @@ -15,7 +15,11 @@ */ package com.diffplug.gradle.spotless; +import java.io.File; import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Comparator; import org.gradle.testkit.runner.BuildResult; import org.gradle.testkit.runner.GradleRunner; @@ -84,5 +88,19 @@ public void gjf() throws IOException { // but the second fails BuildResult failure = gradleRunner().withArguments("spotlessApply").buildAndFail(); failure.getOutput().contains("> Spotless doesn't support configuration cache yet"); + + // and it will keep failing forever + gradleRunner().withArguments("spotlessApply").buildAndFail(); + + // until you delete the .gradlle/configuration-cache folder + File configCache = new File(super.rootFolder(), ".gradle/configuration-cache"); + Files.walk(configCache.toPath()) + .sorted(Comparator.reverseOrder()) + .map(Path::toFile) + .forEach(File::delete); + + // then it will work again (but only once) + gradleRunner().withArguments("spotlessApply").build(); + gradleRunner().withArguments("spotlessApply").buildAndFail(); } } From 4dfc4c896b2d2b80eeae11284bd474e2a8d35caf Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sun, 7 Nov 2021 22:08:48 -0800 Subject: [PATCH 10/14] Better encapsulation - force SpotlessTask clients to use getters. --- .../src/main/java/com/diffplug/gradle/spotless/IdeHook.java | 4 ++-- .../main/java/com/diffplug/gradle/spotless/SpotlessTask.java | 4 ++-- .../java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/IdeHook.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/IdeHook.java index 02f60c21e6..dd62cd523a 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/IdeHook.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/IdeHook.java @@ -42,8 +42,8 @@ static void performHook(SpotlessTaskImpl spotlessTask) { } if (spotlessTask.getTarget().contains(file)) { try (Formatter formatter = spotlessTask.buildFormatter()) { - if (spotlessTask.ratchet != null) { - if (spotlessTask.ratchet.isClean(spotlessTask.getProjectDir().get().getAsFile(), spotlessTask.rootTreeSha, file)) { + if (spotlessTask.getRatchet() != null) { + if (spotlessTask.getRatchet().isClean(spotlessTask.getProjectDir().get().getAsFile(), spotlessTask.getRootTreeSha(), file)) { dumpIsClean(); return; } diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java index 8c997153ec..eb7f2e7caf 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java @@ -69,9 +69,9 @@ public void setLineEndingsPolicy(LineEnding.Policy lineEndingsPolicy) { /*** API which performs git up-to-date tasks. */ @Nullable - GitRatchetGradle ratchet; + private GitRatchetGradle ratchet; /** The sha of the tree at repository root, used for determining if an individual *file* is clean according to git. */ - ObjectId rootTreeSha; + private ObjectId rootTreeSha; /** * The sha of the tree at the root of *this project*, used to determine if the git baseline has changed within this folder. * Using a more fine-grained tree (rather than the project root) allows Gradle to mark more subprojects as up-to-date diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java index 37991c92d5..1db93d4713 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java @@ -92,7 +92,7 @@ private void processInputFile(Formatter formatter, File input) throws IOExceptio File output = getOutputFile(input); getLogger().debug("Applying format to " + input + " and writing to " + output); PaddedCell.DirtyState dirtyState; - if (ratchet != null && ratchet.isClean(getProjectDir().get().getAsFile(), rootTreeSha, input)) { + if (getRatchet() != null && getRatchet().isClean(getProjectDir().get().getAsFile(), getRootTreeSha(), input)) { dirtyState = PaddedCell.isClean(); } else { dirtyState = PaddedCell.calculateDirtyState(formatter, input); From 312b25ea8eadfa8031dd069c9075a5260a436444 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sun, 7 Nov 2021 22:15:57 -0800 Subject: [PATCH 11/14] An easier way to detect that we are running with configuration-cache is to check to see if transient fields are null. --- .../com/diffplug/gradle/spotless/SpotlessTask.java | 10 +++++----- .../com/diffplug/gradle/spotless/SpotlessTaskImpl.java | 3 +-- .../diffplug/gradle/spotless/SpotlessTaskService.java | 9 --------- 3 files changed, 6 insertions(+), 16 deletions(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java index eb7f2e7caf..435afec1de 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java @@ -56,7 +56,7 @@ public void setEncoding(String encoding) { this.encoding = Objects.requireNonNull(encoding); } - protected LineEnding.Policy lineEndingsPolicy; + protected transient LineEnding.Policy lineEndingsPolicy; @Input public LineEnding.Policy getLineEndingsPolicy() { @@ -69,15 +69,15 @@ public void setLineEndingsPolicy(LineEnding.Policy lineEndingsPolicy) { /*** API which performs git up-to-date tasks. */ @Nullable - private GitRatchetGradle ratchet; + private transient GitRatchetGradle ratchet; /** The sha of the tree at repository root, used for determining if an individual *file* is clean according to git. */ - private ObjectId rootTreeSha; + private transient ObjectId rootTreeSha; /** * The sha of the tree at the root of *this project*, used to determine if the git baseline has changed within this folder. * Using a more fine-grained tree (rather than the project root) allows Gradle to mark more subprojects as up-to-date * compared to using the project root. */ - private ObjectId subtreeSha = ObjectId.zeroId(); + private transient ObjectId subtreeSha = ObjectId.zeroId(); public void setupRatchet(GitRatchetGradle gitRatchet, String ratchetFrom) { ratchet = gitRatchet; @@ -139,7 +139,7 @@ public File getOutputDirectory() { return outputDirectory; } - protected List steps = new ArrayList<>(); + protected transient List steps = new ArrayList<>(); @Input public List getSteps() { diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java index 1db93d4713..517632ecec 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java @@ -50,7 +50,6 @@ public abstract class SpotlessTaskImpl extends SpotlessTask { void init(Provider service) { getTaskService().set(service); getProjectDir().set(getProject().getProjectDir()); - service.get().registerSourceCreated(this); } @Inject @@ -69,7 +68,7 @@ public void performAction(InputChanges inputs) throws Exception { Files.createDirectories(outputDirectory.toPath()); } - if (getTaskService().get().sourceWasCreatedThisBuild(this)) { + if (lineEndingsPolicy != null) { try (Formatter formatter = buildFormatter()) { for (FileChange fileChange : inputs.getFileChanges(target)) { File input = fileChange.getFile(); diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java index c616e9ce19..47b8d4e382 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java @@ -42,7 +42,6 @@ * apply already did). */ public abstract class SpotlessTaskService implements BuildService { - private final Map sourceCreated = Collections.synchronizedMap(new HashMap<>()); private final Map apply = Collections.synchronizedMap(new HashMap<>()); private final Map source = Collections.synchronizedMap(new HashMap<>()); private final Map provisioner = Collections.synchronizedMap(new HashMap<>()); @@ -53,14 +52,6 @@ Provisioner provisionerFor(Project project) { }); } - void registerSourceCreated(SpotlessTask spotlessTask) { - sourceCreated.put(spotlessTask.getPath(), spotlessTask); - } - - boolean sourceWasCreatedThisBuild(SpotlessTask spotlessTask) { - return sourceCreated.containsKey(spotlessTask.getPath()); - } - void registerSourceAlreadyRan(SpotlessTask task) { source.put(task.getPath(), task); } From de2e1ece02e8d8a13c05eec9cf351ecc915005a0 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sun, 7 Nov 2021 23:28:44 -0800 Subject: [PATCH 12/14] Make SpotlessTaskService be the AutoCloseable BuildService, and let GitRatchetGradle be a POJO field of the service. Doesn't matter either way for now, but this sets us up for a refactor in the future. --- .../gradle/spotless/FormatExtension.java | 2 +- .../gradle/spotless/GitRatchetGradle.java | 12 +--------- .../spotless/RegisterDependenciesTask.java | 8 ++----- .../gradle/spotless/SpotlessTask.java | 12 ++++++---- .../gradle/spotless/SpotlessTaskImpl.java | 4 ---- .../gradle/spotless/SpotlessTaskService.java | 22 ++++++++++++++++++- 6 files changed, 33 insertions(+), 27 deletions(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java index 5b43a2279f..3ee9265971 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java @@ -737,7 +737,7 @@ protected void setupTask(SpotlessTask task) { spotless.getRegisterDependenciesTask().hookSubprojectTask(task); } if (getRatchetFrom() != null) { - task.setupRatchet(spotless.getRegisterDependenciesTask().getGitRatchet().get(), getRatchetFrom()); + task.setupRatchet(getRatchetFrom()); } } diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GitRatchetGradle.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GitRatchetGradle.java index 7517018d66..8c4f88e7da 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GitRatchetGradle.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GitRatchetGradle.java @@ -19,15 +19,10 @@ import javax.annotation.Nullable; -import org.gradle.api.services.BuildService; -import org.gradle.api.services.BuildServiceParameters; -import org.gradle.tooling.events.FinishEvent; -import org.gradle.tooling.events.OperationCompletionListener; - import com.diffplug.spotless.extra.GitRatchet; /** Gradle implementation of GitRatchet. */ -public abstract class GitRatchetGradle extends GitRatchet implements BuildService, OperationCompletionListener { +public class GitRatchetGradle extends GitRatchet { @Override protected File getDir(File project) { return project; @@ -37,9 +32,4 @@ protected File getDir(File project) { protected @Nullable File getParent(File project) { return project.getParentFile(); } - - @Override - public void onFinish(FinishEvent finishEvent) { - // NOOP - } } diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/RegisterDependenciesTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/RegisterDependenciesTask.java index e8efc8fbfa..6ba228e0b5 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/RegisterDependenciesTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/RegisterDependenciesTask.java @@ -72,8 +72,7 @@ void setup() { BuildServiceRegistry buildServices = getProject().getGradle().getSharedServices(); getTaskService().set(buildServices.registerIfAbsent("SpotlessTaskService", SpotlessTaskService.class, spec -> {})); - getGitRatchet().set(buildServices.registerIfAbsent("GitRatchetGradle", GitRatchetGradle.class, unused -> {})); - getBuildEventsListenerRegistry().onTaskCompletion(getGitRatchet()); + getBuildEventsListenerRegistry().onTaskCompletion(getTaskService()); } @TaskAction @@ -83,10 +82,7 @@ public void trivialFunction() throws IOException { } @Internal - public abstract Property getTaskService(); - - @Internal - public abstract Property getGitRatchet(); + abstract Property getTaskService(); @Inject protected abstract BuildEventsListenerRegistry getBuildEventsListenerRegistry(); diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java index 435afec1de..adf69b5e1d 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java @@ -29,6 +29,7 @@ import org.gradle.api.DefaultTask; import org.gradle.api.file.DirectoryProperty; import org.gradle.api.file.FileCollection; +import org.gradle.api.provider.Property; import org.gradle.api.tasks.Input; import org.gradle.api.tasks.InputFiles; import org.gradle.api.tasks.Internal; @@ -44,6 +45,9 @@ import com.diffplug.spotless.LineEnding; public abstract class SpotlessTask extends DefaultTask { + @Internal + abstract Property getTaskService(); + // set by SpotlessExtension, but possibly overridden by FormatExtension protected String encoding = "UTF-8"; @@ -79,11 +83,11 @@ public void setLineEndingsPolicy(LineEnding.Policy lineEndingsPolicy) { */ private transient ObjectId subtreeSha = ObjectId.zeroId(); - public void setupRatchet(GitRatchetGradle gitRatchet, String ratchetFrom) { - ratchet = gitRatchet; + public void setupRatchet(String ratchetFrom) { + ratchet = getTaskService().get().getRatchet(); File projectDir = getProjectDir().get().getAsFile(); - rootTreeSha = gitRatchet.rootTreeShaOf(projectDir, ratchetFrom); - subtreeSha = gitRatchet.subtreeShaOf(projectDir, rootTreeSha); + rootTreeSha = ratchet.rootTreeShaOf(projectDir, ratchetFrom); + subtreeSha = ratchet.subtreeShaOf(projectDir, rootTreeSha); } @Internal diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java index 517632ecec..e33e7f3ec6 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java @@ -26,7 +26,6 @@ import org.gradle.api.GradleException; import org.gradle.api.file.DirectoryProperty; import org.gradle.api.file.FileSystemOperations; -import org.gradle.api.provider.Property; import org.gradle.api.provider.Provider; import org.gradle.api.tasks.CacheableTask; import org.gradle.api.tasks.Internal; @@ -41,9 +40,6 @@ @CacheableTask public abstract class SpotlessTaskImpl extends SpotlessTask { - @Internal - abstract Property getTaskService(); - @Internal abstract DirectoryProperty getProjectDir(); diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java index 47b8d4e382..de016ae6d2 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskService.java @@ -30,6 +30,8 @@ import org.gradle.api.services.BuildService; import org.gradle.api.services.BuildServiceParameters; import org.gradle.api.tasks.Internal; +import org.gradle.tooling.events.FinishEvent; +import org.gradle.tooling.events.OperationCompletionListener; import com.diffplug.common.base.Preconditions; import com.diffplug.common.base.Unhandled; @@ -41,7 +43,7 @@ * duplicated work (e.g. no need for check to run if * apply already did). */ -public abstract class SpotlessTaskService implements BuildService { +public abstract class SpotlessTaskService implements BuildService, AutoCloseable, OperationCompletionListener { private final Map apply = Collections.synchronizedMap(new HashMap<>()); private final Map source = Collections.synchronizedMap(new HashMap<>()); private final Map provisioner = Collections.synchronizedMap(new HashMap<>()); @@ -60,6 +62,24 @@ void registerApplyAlreadyRan(SpotlessApply task) { apply.put(task.sourceTaskPath(), task); } + // + private final GitRatchetGradle ratchet = new GitRatchetGradle(); + + GitRatchetGradle getRatchet() { + return ratchet; + } + + @Override + public void onFinish(FinishEvent var1) { + // NOOP + } + + @Override + public void close() throws Exception { + ratchet.close(); + } + // + static String INDEPENDENT_HELPER = "Helper"; static abstract class ClientTask extends DefaultTask { From 3557f570fe50d6c980db9c892cab03291f69226e Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Mon, 8 Nov 2021 00:26:45 -0800 Subject: [PATCH 13/14] Every task should depend on `RegisterDependenciesTask` now, since it doesn't do anything and if it ever does do something it will be surpising if it's just some tasks. --- .../java/com/diffplug/gradle/spotless/FormatExtension.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java index 3ee9265971..77e0e4f7f7 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java @@ -733,9 +733,7 @@ protected void setupTask(SpotlessTask task) { } task.setSteps(steps); task.setLineEndingsPolicy(getLineEndings().createPolicy(getProject().getProjectDir(), () -> totalTarget)); - if (spotless.project != spotless.project.getRootProject()) { - spotless.getRegisterDependenciesTask().hookSubprojectTask(task); - } + spotless.getRegisterDependenciesTask().hookSubprojectTask(task); if (getRatchetFrom() != null) { task.setupRatchet(getRatchetFrom()); } From 33fba68033bcc9da68b288b1f1728c51caad6f72 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Mon, 8 Nov 2021 01:03:07 -0800 Subject: [PATCH 14/14] Fixup tests broken by the change to RegisterDependenciesTask. --- .../spotless/ErrorShouldRethrowTest.java | 31 ++++++++++--------- .../spotless/GradleIntegrationHarness.java | 23 +++++++++----- .../gradle/spotless/UpToDateTest.java | 2 +- 3 files changed, 34 insertions(+), 22 deletions(-) diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/ErrorShouldRethrowTest.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/ErrorShouldRethrowTest.java index ab1db7a632..d1ced01609 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/ErrorShouldRethrowTest.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/ErrorShouldRethrowTest.java @@ -27,7 +27,6 @@ import com.diffplug.common.base.CharMatcher; import com.diffplug.common.base.Splitter; -import com.diffplug.common.base.StringPrinter; import com.diffplug.spotless.LineEnding; /** Tests the desired behavior from https://github.com/diffplug/spotless/issues/46. */ @@ -70,7 +69,7 @@ void anyExceptionShouldFail() throws Exception { "> Task :spotlessMisc FAILED\n" + "Step 'no swearing' found problem in 'README.md':\n" + "No swearing!\n" + - "java.lang.RuntimeException: No swearing!\n"); + "java.lang.RuntimeException: No swearing!"); } @Test @@ -80,7 +79,7 @@ void unlessEnforceCheckIsFalse() throws Exception { " enforceCheck false", "} // spotless"); setFile("README.md").toContent("This code is fubar."); - runWithSuccess("> Task :compileJava NO-SOURCE"); + runWithSuccess("> Task :processResources NO-SOURCE"); } @Test @@ -101,7 +100,7 @@ void unlessExemptedByPath() throws Exception { " } // format", "} // spotless"); setFile("README.md").toContent("This code is fubar."); - runWithSuccess("> Task :spotlessMisc", + runWithSuccess("> Task :spotlessMisc\n" + "Unable to apply step 'no swearing' to 'README.md'"); } @@ -116,26 +115,30 @@ void failsIfNeitherStepNorFileExempted() throws Exception { runWithFailure("> Task :spotlessMisc FAILED\n" + "Step 'no swearing' found problem in 'README.md':\n" + "No swearing!\n" + - "java.lang.RuntimeException: No swearing!\n"); + "java.lang.RuntimeException: No swearing!"); } - private void runWithSuccess(String... messages) throws Exception { + private void runWithSuccess(String expectedToStartWith) throws Exception { BuildResult result = gradleRunner().withArguments("check").build(); - assertResultAndMessages(result, TaskOutcome.SUCCESS, messages); + assertResultAndMessages(result, TaskOutcome.SUCCESS, expectedToStartWith); } - private void runWithFailure(String... messages) throws Exception { + private void runWithFailure(String expectedToStartWith) throws Exception { BuildResult result = gradleRunner().withArguments("check").buildAndFail(); - assertResultAndMessages(result, TaskOutcome.FAILED, messages); + assertResultAndMessages(result, TaskOutcome.FAILED, expectedToStartWith); } - private void assertResultAndMessages(BuildResult result, TaskOutcome outcome, String... messages) { - String expectedToStartWith = StringPrinter.buildStringFromLines(messages).trim(); + private void assertResultAndMessages(BuildResult result, TaskOutcome outcome, String expectedToStartWith) { + String output = result.getOutput(); + int register = output.indexOf(":spotlessInternalRegisterDependencies"); + int firstNewlineAfterThat = output.indexOf('\n', register + 1); + String useThisToMatch = output.substring(firstNewlineAfterThat); + int numNewlines = CharMatcher.is('\n').countIn(expectedToStartWith); - List actualLines = Splitter.on('\n').splitToList(LineEnding.toUnix(result.getOutput().trim())); + List actualLines = Splitter.on('\n').splitToList(LineEnding.toUnix(useThisToMatch.trim())); String actualStart = String.join("\n", actualLines.subList(0, numNewlines + 1)); Assertions.assertThat(actualStart).isEqualTo(expectedToStartWith); - Assertions.assertThat(result.tasks(outcome).size() + result.tasks(TaskOutcome.UP_TO_DATE).size() + result.tasks(TaskOutcome.NO_SOURCE).size()) - .isEqualTo(result.getTasks().size()); + Assertions.assertThat(outcomes(result, outcome).size() + outcomes(result, TaskOutcome.UP_TO_DATE).size() + outcomes(result, TaskOutcome.NO_SOURCE).size()) + .isEqualTo(outcomes(result).size()); } } diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/GradleIntegrationHarness.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/GradleIntegrationHarness.java index 8d76850488..45db3f008e 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/GradleIntegrationHarness.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/GradleIntegrationHarness.java @@ -138,17 +138,26 @@ private void taskIsUpToDate(String task, boolean upToDate) throws IOException { pauseForFilesystem(); BuildResult buildResult = gradleRunner().withArguments(task).build(); - TaskOutcome expected = upToDate ? TaskOutcome.UP_TO_DATE : TaskOutcome.SUCCESS; - TaskOutcome notExpected = upToDate ? TaskOutcome.SUCCESS : TaskOutcome.UP_TO_DATE; - - boolean everythingAsExpected = !buildResult.tasks(expected).isEmpty() && - buildResult.tasks(notExpected).isEmpty() && - buildResult.getTasks().size() == buildResult.tasks(expected).size(); + List expected = outcomes(buildResult, upToDate ? TaskOutcome.UP_TO_DATE : TaskOutcome.SUCCESS); + List notExpected = outcomes(buildResult, upToDate ? TaskOutcome.SUCCESS : TaskOutcome.UP_TO_DATE); + boolean everythingAsExpected = !expected.isEmpty() && notExpected.isEmpty() && buildResult.getTasks().size() - 1 == expected.size(); if (!everythingAsExpected) { - fail("Expected all tasks to be " + expected + ", but instead was\n" + buildResultToString(buildResult)); + fail("Expected all tasks to be " + (upToDate ? TaskOutcome.UP_TO_DATE : TaskOutcome.SUCCESS) + ", but instead was\n" + buildResultToString(buildResult)); } } + protected static List outcomes(BuildResult build, TaskOutcome outcome) { + return build.taskPaths(outcome).stream() + .filter(s -> !s.equals(":spotlessInternalRegisterDependencies")) + .collect(Collectors.toList()); + } + + protected static List outcomes(BuildResult build) { + return build.getTasks().stream() + .filter(t -> !t.getPath().equals(":spotlessInternalRegisterDependencies")) + .collect(Collectors.toList()); + } + static String buildResultToString(BuildResult result) { return StringPrinter.buildString(printer -> { for (BuildTask task : result.getTasks()) { diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/UpToDateTest.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/UpToDateTest.java index c3966fe24f..9b7eccdfff 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/UpToDateTest.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/UpToDateTest.java @@ -88,7 +88,7 @@ void testPathologicalCase() throws IOException { // the format task is UP-TO-DATE (same inputs), but the apply tasks will run again pauseForFilesystem(); BuildResult buildResult = gradleRunner().withArguments("spotlessApply").build(); - Assertions.assertThat(buildResult.taskPaths(TaskOutcome.UP_TO_DATE)).containsExactly(":spotlessMisc"); + Assertions.assertThat(buildResult.taskPaths(TaskOutcome.UP_TO_DATE)).containsExactly(":spotlessInternalRegisterDependencies", ":spotlessMisc"); Assertions.assertThat(buildResult.taskPaths(TaskOutcome.SUCCESS)).containsExactly(":spotlessMiscApply", ":spotlessApply"); assertFile("README.md").hasContent("abc");