Configuration cache, part 1 (play nice with others)

[nedtwigg]

This PR establishes a few things:

• it confirms that our existing shipping version of Spotless 5.6.1 doesn't interfere with configuration cache for other tasks
• we then test to see "if a Spotless task gets eagerly configured, does that interfere with configuration cache for other tasks?
• the answer to this question is yes, because we use Gradle.buildFinished at configure time
• we fix this by converting the Gradle.buildFinished into a BuildService extends AutoCloseable

So now Spotless definitely doesn't interfere with configuration cache for other tasks. The cost is that we had to upgrade our required version from 5.4 to 6.1 in order to get BuildService.

Unresolved question This assumes that a BuildService is created anew at the beginning of every build, and then destroyed at the end of every build. I think that might not be the case - it might be persistent across builds so long as a "configuration cache" is in-effect, in which case this PR is broken.

[eskatos]

By default build services are created when they are used and shut down once no task are using them.

If you want your build service to be created at the beginning of the build and shut down at the end of the build there's some extra ceremony required:

• make it implements OperationCompletionListener, AutoCloseable
• register it with the BuildEventsListenerRegistry service which you can get injected

[nedtwigg]

Roger, thanks!

How does Gradle know if a task needs a build service?

We create our GitRatchetGradle build service exactly once like this, during project configuration.

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/RegisterDependenciesTask.java

Lines 86 to 91 in fa01f46

	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());
	gitRatchet = getProject().getGradle().getSharedServices().registerIfAbsent("GitRatchetGradle", GitRatchetGradle.class, unused -> {}).get();
	}

And then we use it both during configuration time and task execution. Because we use it at configuration time, we just pass the service as an argument to the tasks that need it, we don't inject it, but it is declared as an @Internal field. Will Gradle "find" that these tasks are using it?

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java

Lines 89 to 98 in fa01f46

	public void setupRatchet(GitRatchetGradle gitRatchet, String ratchetFrom) {
	ratchet = gitRatchet;
	rootTreeSha = gitRatchet.rootTreeShaOf(getProject(), ratchetFrom);
	subtreeSha = gitRatchet.subtreeShaOf(getProject(), rootTreeSha);
	}
	@Internal
	GitRatchetGradle getRatchet() {
	return ratchet;
	}

If the spec said "every BuildService which implements AutoCloseable will be closed when the build finishes" then I would know that we are okay. But it seems like Gradle is taking responsibility for knowing which tasks use the service, and I don't understand how I can signal to Gradle "I am using this object". Am I okay so long as I expose it with an @Internal getService() getter?

How does Gradle persist build services across runs of the configuration cache?

I don't understand how the registerIfAbsent(String, Class<?>, Action<?> someLambdaWhichDependsOnConfiguration> interacts with configuration cache. If the API was instead T ObjectFactory.onePerBuild(Class<T>), I would understand quickly that the service is created on demand from its constructor for each build. Because there is a configuration lambda, I don't understand how the lambda can work, which makes me think that I'm misunderstanding the API.

[eskatos]

1/ The task should have a @Internal Property<SomeBuildService> that you set with the service provider as shown in https://docs.gradle.org/current/userguide/build_services.html#using_a_build_service_from_a_task This is the supported way of declaring that a task make use of a build service. You can also .get() that service at configuration time if you need to.

2/ Build services are per build. The configuration cache stores the name, type and parameters configured in the action. The type alone isn't enough. It is the same pattern as with artifact transforms or worker actions.

[nedtwigg]

This is the supported way of declaring that a task make use of a build service.

Roger. So when tasks are restored from the configuration cache, a new BuildService is constructed, configured with the deserialized BuildParameter, and then that one instance is shared across every task which did the correct @Internal Property<SomeBuildService> ceremony.

That's a very cool, clever way to get properties from the buildscript execution into the BuildService, well done!

In our case, and I would assume others, all we need is a build-lifetime singleton, which is a special case of the more general thing which you have built. You have provided the API we need, but fwiw, I would much prefer T ObjectFactory.onePerBuild(Class<T>). It seems like you could implement it out of the parts you already have, and it's a lot shorter to document. I worry a lot about complicated docs, because people end up using them wrong. I think you've done the simplest possible thing if you need your build service to capture something from the buildscript, but it would be great if there were a "BuildService light" which doesn't need so much ceremony and documentation. Certainly not critical though, we are not blocked.

[eskatos]

Indeed, the current build service API is about the basic building blocks. We will add conveniences over time.

[eskatos]

Hi Ned,
Any chance this PR could get merged eventually?
It would be great that the spotless plugin play nice with others as a first step.

[jbduncan]

@eskatos Am I right to think that to make this PR perfect, we'd need to ensure your earlier suggestions are all implemented? Specifically:

• Make GitRatchetGradle implement OperationCompletionListener, AutoCloseable as well.
• Register it with the BuildEventsListenerRegistry service, which Gradle can inject into Spotless for us.
• Use an @Internal Property<GitRatchetGradle> property in RegisterDependenciesTask.

[eskatos]

@jbduncan, yep 💯

[jbduncan]

@eskatos Great, thanks for confirming!

I'm focusing my free open source time on another project, so I trust that Ned will get back to you at some point.

[nedtwigg]

The published version of Spotless already plays nicely with configuration cache so long as you don't cause the Spotless tasks to be configured, thanks to configuration avoidance.

The cost of merging this PR is that our minimum Gradle bumps from 5.4 to 6.1, and the benefit is only for users that:

1. want to use configuration cache
2. but don't want to run Spotless
3. and are causing Spotless to configure anyway due to a sloppy build e.g. tasks.withType

If anybody is in that position and wants to finish this PR, I'd be happy to merge it and bump our minimum Gradle. But the benefit is too low for this to make it to the top of my todo.

I find configuration-cache to be most helpful for iterating on tests, and Spotless already lets that happen without conflict.

[eskatos]

I just found a case where configuring the spotless plugin is enough to trigger task creation thus configuration cache problems.

The following snippet makes running ./gradlew help report a configuration cache problem:

plugins {
    id("com.diffplug.spotless") version "<v>"
}

spotless {
    java {
        removeUnusedImports()
    }
}

removeUnusedImports() calls down to RegisterDependenciesTask.setup() which register the build listener.

Simplified stacktrace:

Caused by: org.gradle.api.InvalidUserCodeException: Listener registration 'Gradle.buildFinished' by build 'gradle' is unsupported.
  at org.gradle.configurationcache.initialization.DefaultConfigurationCacheProblemsListener.onBuildScopeListenerRegistration(ConfigurationCacheProblemsListener.kt:97)
  at org.gradle.invocation.DefaultGradle.notifyListenerRegistration(DefaultGradle.java:381)
  at org.gradle.invocation.DefaultGradle.buildFinished(DefaultGradle.java:359)
  at com.diffplug.gradle.spotless.RegisterDependenciesTask.setup(RegisterDependenciesTask.java:94)
  at org.gradle.api.internal.tasks.DefaultTaskContainer$TaskCreatingProvider.tryCreate(DefaultTaskContainer.java:676)
  at org.gradle.api.internal.DefaultNamedDomainObjectCollection$AbstractDomainObjectCreatingProvider.get(DefaultNamedDomainObjectCollection.java:915)
  at com.diffplug.gradle.spotless.SpotlessExtensionImpl.getRegisterDependenciesTask(SpotlessExtensionImpl.java:60)
  at com.diffplug.gradle.spotless.FormatExtension.provisioner(FormatExtension.java:77)
  at com.diffplug.gradle.spotless.JavaExtension.removeUnusedImports(JavaExtension.java:71)

[nedtwigg]

Roger. Happy to merge the PR if anyone has time to finish it. I've been using configuration-cache for a few weeks and calling Spotless only with --no-configuration-cache, and I haven't encountered this. removeUnusedImports() is inside a closure that only gets executed on task configuration, so I'm surprised that it's getting called.

[eskatos]

I just opened #937 which contains the changes described by @jbduncan in #720 (comment)

The PR targets this one.

[nedtwigg]

Thanks very much @eskatos. One last question for you (in the comments above) and then I'll merge and release.

[nedtwigg]

Just want to confirm that we don't need to call close() here? GitRatchet implements AutoCloseable, so if Gradle is promising to call close() then I guess we're good, but it's odd that there's also an onFinish() which seems like it would get called at the same time as close()?

[eskatos]

OperationCompletionListener.onFinish is called on each "task finish" event.
AutoCloseable.close is called at the end of the build.

My understanding is that we are good.

[nedtwigg]

Here is where we used to call close()

[nedtwigg]

Thanks @eskatos for getting this over the hump. Will be released shortly.

[nedtwigg]

Released in plugin-gradle 5.15.1.