Non-reproducible behavior around dependency verification with Spotless plugin

[lptr]

In the Gradle build, when I execute :file-watching:check (using commit a6ca78d99a8ec4448b4eb1ee802ab763bce40b43) I sometimes get a passing build, but sometimes it fails with a dependency verification issue (that is wrapped in a snapshotting error message because of #13276 (comment), but that's besides the point):

Failure 1 of 1The :spotlessInternalRegisterDependencies task failed.View task in console log
7 other builds with similar failures in last 7 daysView failure history
Unable to store input properties for task ':spotlessInternalRegisterDependencies'. Property 'steps' with value '[com.diffplug.spotless.FormatterStepImpl$Standard@cfa6a6ec]' cannot be serialized.
> Dependency verification failed for configuration ':detachedConfiguration1'
  One artifact failed verification: error_prone_annotations-2.3.2.jar (com.google.errorprone:error_prone_annotations:2.3.2) from repository Gradle Central Plugin Repository
  If the artifacts are trustworthy, you will need to update the gradle/verification-metadata.xml file by following the instructions at https://docs.gradle.org/7.0-20210309210224+0000/userguide/dependency_verification.html#sec:troubleshooting-verification

• Passing build: https://ge.gradle.org/s/albciu7t2v24a
• Failing build right after the previous with no modification in the build: https://ge.gradle.org/s/2yz6ewye5e7ni

Adding a few empty lines to one of the source files (DefaultFileWatcherRegistry) caused the build to pass after a failure, too.

It looks like Spotless does something strange wrt adding dependencies during execution time in here:

https://github.com/diffplug/spotless/blob/gradle/5.10.2/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java#L63-L84

...but nothing that says the result should be non-deterministic.

---

cc: @gradle/execution

[nedtwigg]

Spotless resolves all dependencies (including for formatters used in subprojects) in the root project. The legacy chain which leads to this is:

1. We want to resolve deps from buildscript repositories, not from project repositories spotless loads dependency from project dependencies diffplug/spotless#99
2. Gradle doesn't allow subprojects to resolve new dependencies from the buildscript repositories, so Spotless subproject tasks register themselves with the root project for their deps to be resolved Fix Gradle 6+ diffplug/spotless#503

A FormatterStep will resolve its dependencies on an equality check, because two steps are equal only if they used the same dependencies. We cause a step to resolve its dependencies by exposing it to Gradle as an input property, but only the steps which are going to run get exposed as inputs and thus resolved.

All built-in Spotless tasks can be serialized, but some custom steps containing arbitrary closures cannot. In this way users can "opt-out" of fast up-to-date checks if the benefit of custom closures are worth it. So I'm guessing that the build in question uses a custom without bumpThisNumberIfACustomStepChanges.

This design arose organically from something that worked well in Gradle 2.x, which has been adapted over time so that our users have never had to change their Spotless blocks even as Gradle improved/strict-ified the underlying infrastructure. This design is an obstacle to supporting Configuration Cache, so we may have to give up on resolving dependencies from buildscript repositories, and instead declare spotlessFoo configurations in subprojects which resolve their dependencies from project repositories. I am open to PRs for this, and will do it myself when/if it is necessary to keep Spotless from being a laggard, but it's not high on my priority list.

If our weirdness was helpful in exposing a corner-case that you care about, then you're welcome! Or if it's a corner-case which you would prefer to just make illegal, that's fine too, and we can eventually adapt to it :)

[lptr]

Thanks @nedtwigg for the explanation!

There are a few things here:

1. I think dropping the "weirdness" and using project-space configurations to resolve whatever Spotless needs is going to be necessary. I think some work should be done here regardless, as the behavior I've seen is that Spotless resolves dependencies required for Eclipse WTP support—not something that our build requires at all, so it's all wasted effort. Removing this redundant work could make builds faster.

2. About the up-to-date checks for FormatterStep... The error discussed here is not a serialization problem, it just masquerades as one. It's is annoying, and is caused by the dependency verification exception being thrown while we are snapshotting the inputs of FormatterStep. We'll fix this in Resolve dependencies outside of snapshotting build operations #13276 and the confusion will be removed. Hopefully soon...

3. Now that you mention the closures, I'm not sure how useful they are anymore. Since Gradle 2.x we've done a lot to make up-to-date checks faster. Especially with Gradle 7.0, where a hot daemon will only be snapshotting inputs that have actually changed between builds (this is due to Gradle automatically watching the file system now, instead of re-snapshotting every input with every build). As a result snapshotting (=up-to-date checking) times for incremental changes should be minimal even for large projects. I wonder if keeping around this custom feature is worth it anymore. I'd be happy to talk about it here, or on the community Slack if you want.

[nedtwigg]

Spotless resolves dependencies required for Eclipse WTP support—not something that our build requires at all

I'm confident that Spotless isn't downloading stuff that the user isn't asking for. Spotless integrates with a huge number of versions of a large number of formatters (which is why we bother to download deps dynamically at all). If we were accidentally downloading all of them willy-nilly it would be as slow as our full test suite, which is very very slow because of so many deps. Are you sure the string eclipseWtp isn't lurking in a .gradle somewhere?

We will probably eventually use project-space resolution for the purpose of Configuration Cache, but it ought to be strictly slower than the current approach, unless maybe you do inter-project memoization for subprojects which request the same deps, in which case it will be the same speed. That's all that we are doing - inter-project memoization of dep resolution. We're doing it because we had no other choice, but it's a speedup regardless.

I wonder if keeping around this custom feature is worth it anymore.

I use it a lot! I've discussed this with other Gradle folks - it's incredibly useful to be able to specify a function as an up-to-dateable input. So it'll definitely stick around, but either way it sounds like it isn't the main thing happening here.

The error discussed here is not a serialization problem

Ah, perhaps a reprise of #11752. It looks like the way we download dependencies is triggering a race-condition in gradle's dep resolution. If you'd like another example, this test is ignored because resolving arbitrary dependencies has worked and broken back and forth between different gradle versions: diffplug/spotless#429

Gradle has APIs for f(maven coordinates) = files, which is required for "code-as-data". Over time, the rules for calling that f have gotten more restrictive, which probably allows you to do optimizations, which is fine. But I think it's important to have some general-purpose escape hatch, even if it is slow. "Code-as-data" is incredibly useful, and if Gradle makes it impossible to treat code as data unless that code can be declared in a specific way at a specific phase in the build, then Gradle isn't a place that allows "code-as-data" anymore, which is a bummer since it has been the lone JVM place for that.

[nedtwigg]

This might be fixed in Spotless 5.12.4 (the only change is converting from detached configurations to named configurations diffplug/spotless#848).