SpotlessTaskService (towards configuration-cache) #982
Conversation
…ng-mapping in SpotlessTaskService.
…er than just assuming UTF-8.
…perations where it was easy to do so.
…all `getProject()` except IdeHook.
…hashCode` without serialization." This reverts commit 84101b9.
nedtwigg
changed the title
| */ | ||
| public abstract class SpotlessTaskService implements BuildService<BuildServiceParameters.None> { | ||
| private final Map<String, SpotlessApply> apply = Collections.synchronizedMap(new HashMap<>()); | ||
| private final Map<String, SpotlessTask> source = Collections.synchronizedMap(new HashMap<>()); |
There was a problem hiding this comment.
Have you considered using ConcurrentMap over synchronized maps here? I ask because if we ever need to loop, stream, or otherwise do some iteration operation over either of these maps (such an operation may not be obvious), then the map would need to be synchronized first to prevent concurrency problems.
There was a problem hiding this comment.
I think it's gonna stay pretty simple.
There was a problem hiding this comment.
Let me clarify -> our tasks don't need to coordinate across projects, they only coordinate within a single project. If we could specify a SpotlessTaskService per-project, then we wouldn't need it to be synchronized. So concurrent access is possible, but I see no risk of deadlock nor of inconsistent state, because the parts that need to coordinate are single-threaded.
There was a problem hiding this comment.
Ah, gotcha. I'd still argue that ConcurrentMap is the better solution, but it looks like we'll have to agree to disagree. So I'm happy to defer to you once more. :)
| @@ -59,6 +60,10 @@ | |||
| } | |||
| } | |||
|
|
|||
| public static <T> Provider<T> providerOf(T value) { | |||
| return org.gradle.api.internal.provider.Providers.of(value); | |||
There was a problem hiding this comment.
I remember having used the following in a Gradle build script recently:
val someList = provider {
listOf(1, 2, 3)
}So I was wondering if this provider function is available outside of the Gradle DSL. If so, it would mean we wouldn't need to use an internal API here.
There was a problem hiding this comment.
This is a good question, and the answer is super surprising -interface org.gradle.api.provider.Provider is public, and has quite a few methods. But there are literally no implementations (abstract or not) in an api package, only internal (that I could find at least).
If you take the time to implement it yourself (which I did), then you actually get a ClassCastException! Because somewhere in Gradle's guts, they cast it to org.gradle.internal.ProviderImpl (I forget the exact name), which means you can't implement it yourself.
We have a code style rule that barfs at any .internal. imports, which is why I spelled-out the package inline like I did. I'm still a huge gradle fan, but I was pretty bummed when I found out that you can't actually implement Provider 😞
There was a problem hiding this comment.
That is surprising! Maybe the Gradle developers wanted something like sealed classes for Provider but couldn't get it for the version of Java they're constrained to.
I would've also guessed that Provider was a single-method interface, where all the other methods were default. But maybe it's because the minimum Java version that Gradle runs on isn't Java 8 yet.
Having done some research just now, I see that provider { } is indeed only available in the DSL, specifically on Project.getProviders(), Settings.getProviders(), and through @Injecting a ProviderFactory into a plugin or extension. I suppose if we weren't constrained by the configuration cache we'd be able to use one of them, but alas.
Thanks for explaining!
nedtwigg
changed the title
|
Released in plugin-gradle |
This PR is a step towards support for the configuration-cache, but it is not the full thing. It is intended to produce no changes in behavior, and just move us off of the forbidden APIs.
IMO it's a bummer.
ObjectFactorydoes fileTrees, andFileSystemOperationsdoes deleting, and if you want to know your project directory you've got to break code navigation / dataflow analysis and store it into your own bespoke property and hope that the name you pick isn't too different from what future PR contributors expect. I did quite a bit of refactoring to try to simplify as much as I could, but this PR basically just adds 300 lines of boilerplate.