Task configuration avoidance (WIP) (#269)

[jbduncan]

This is a WIP PR to show how task configuration avoidance may look like in the Spotless Gradle plugin (see issue #269).

No profiling against Spotless itself or against an external project has been done yet. Those are the next things which need to be done.

Feedback is more than welcome!

Resolves #269.

---

Please make sure that your PR allows edits from maintainers. Sometimes its faster for us to just fix something than it is to describe how to fix it.

After creating the PR, please add a commit that adds a bullet-point under the -SNAPSHOT section of CHANGES.md and plugin-gradle/CHANGES.md which includes:

• a summary of the change
• a link to the newly created PR

If your change only affects a build plugin, and not the lib, then you only need to update the CHANGES.md for that plugin.

If your change affects lib in an end-user-visible way (fixing a bug, updating a version) then you need to update CHANGES.md for both the lib and the build plugins. Users of a build plugin shouldn't have to refer to lib to see changes that affect them.

This makes it easier for the maintainers to quickly release your changes :)

[nedtwigg]

This commit (e6818c5) looks great. I like the API gradle has provided.

[jbduncan]

I can't find a way locally of consistently reproducing the Travis CI error, so I'll try pushing a minor polishing change and see what happens.

[jbduncan]

@nedtwigg It seems that Travis CI complains with the following message:

> Task :plugin-gradle:spotlessCheck FAILED
Picked up _JAVA_OPTIONS: -Xmx2048m -Xms512m
Error: Could not find or load main class com.diffplug.gradle.spotless.SelfTestCheck

But it's unclear to me why

1. task :plugin-gradle:spotlessCheck would apparently be attempting to load SelfTestCheck, even if it is a class with a "main" method,
2. and what could cause this class to not be found. (Is the java command line tool being called by spotlessCheck somehow?)

Do you have any thoughts regarding this?

[nedtwigg]

At least in 2.14.1, it was not possible to build a plugin, and then apply the plugin you built to its own sourcecode. I burned a bunch of time trying to make that work. So the workaround was this:

spotless/plugin-gradle/build.gradle

Lines 29 to 42 in 2ac756d

	/////////////////////
	// SPOTLESS (fake) //
	/////////////////////
	task spotlessCheck(type: JavaExec) {
	classpath sourceSets.test.runtimeClasspath
	main = 'com.diffplug.gradle.spotless.SelfTestCheck'
	}
	check.dependsOn(spotlessCheck)
	task spotlessApply(type: JavaExec) {
	classpath sourceSets.test.runtimeClasspath
	main = 'com.diffplug.gradle.spotless.SelfTestApply'
	}
	test { testLogging.showStandardStreams = true }

So that's why it's attempting to load SelfTestCheck. As for why it can't be found, maybe gradle 4.9 changes what's on sourceSets.test.runtimeClasspath?

[jbduncan]

@nedtwigg Sorry for the kind of slow response!

When I check the output produced by the debug statements I introduced in 18b70f0 (see https://travis-ci.org/diffplug/spotless/builds/412790310 for the Travis log), it apparently doesn't differ much to when I apply the same debug statements to plugin-gradle/build.gradle on the master branch locally on my machine.

The only difference I can see is that the Travis log has some artifacts whose names contain the term gradle-4.9, whereas on my local master those same statements contain gradle-4.6 instead. (Which makes sense since I upgraded to Gradle 4.9 as part of this PR.)

Thus I wonder if you have any other thoughts which may allow me to get unstuck and move forward with this?

[jbduncan]

Sorry, I should have clarified that these debug statements specifically print the names of the files contained by sourceSets.test.runtimeClasspath. :)

[nedtwigg]

The only difference I can see is that the Travis log has some artifacts whose names contain the term gradle-4.9, whereas on my local master those same statements contain gradle-4.6 instead. (Which makes sense since I upgraded to Gradle 4.9 as part of this PR.)

This doesn't make sense to me. If you want to reproduce on your local machine, then it should match the CI server in every way possible. I think I'm misunderstanding something...

[jbduncan]

Oops, sorry, I wasn't clear! And also I realise that I'm a bit confused in general, so I'll attempt to explain what I've understood so far.

(Warning: heavy wall-of-text below.)

When working from the jbduncan/task-config-avoidance branch (this PR's branch), then indeed on both Travis and my local machine, when I run any Gradle command e.g. ./gradlew help, then the debug statements print the same output (except that since my computer runs Windows 7 rather than Linux, it prints all file names as C:\full\path\to\file.txt instead of full/path/to/file.txt).

Where I expected things to differ in a helpful way was if, rather than running ./gradlew help with the debug statements in plugin-gradle/build.gradle in my local copy of branch jbduncan/task-config-avoidance, I instead copied and pasted those debug statements to the same places in the version of plugin-gradle/build.gradle stored in my local master branch and ran ./gradle help again.

You should find that if you clone my fork of Spotless (https://github.com/jbduncan/spotless), and compare the output of the debug statements in master and then branch task-config-avoidance, that the output is just a bit different:

1. On master, the debug statements print some lines containing the substring "gradle-4.6" (which I guess is because the version of the Gradle wrapper used on the master branch is still 4.6).
2. On task-config-avoidance, these same lines are printed except that they contain "gradle-4.9" instead (which I guess is because I upgraded the Gradle wrapper to version 4.9 in commit df01002).

However, this isn't helpful for me, because it doesn't explain to me why sourceSets.test.runtimeClasspath contains SelfCheckTest on Gradle 4.6 whereas on Gradle 4.9 it doesn't.

I hope this clears things up.

[jbduncan]

However, this isn't helpful for me, because it doesn't explain to me why sourceSets.test.runtimeClasspath contains SelfCheckTest on Gradle 4.6 whereas on Gradle 4.9 it doesn't.

Sorry, I meant to say: it doesn't explain to me why Gradle can find SelfTestCheck on master but not on this PR's branch.

[nedtwigg]

The travis error in the latest PR is:

A problem occurred evaluating project ':ide'.
> A problem occurred configuring project ':plugin-gradle'.
   > Cannot change dependencies of configuration ':plugin-gradle:testCompile' after it has been included in dependency resolution.

I don't know why that's happening, but it doesn't jive with the earlier issues. I would revert that commit and get back to the original error. The heuristic I use when I'm stuck on something like this is to reduce the variables. Right now we have two: a new version of gradle, and code changes.

Here's an experiment:

• checkout master somewhere on your box
• gradlew clean, gradlew check should work
• How about if you update to 4.7?
• 4.8?
• 4.9?

If any of those break, then we've narrowed down which version of gradle caused the problem, and have a better chance of making progress. If all of those work, then we've narrowed it down to your code changes. I can't imagine how they could change the classpath, so I'm fairly confident that one of the gradle upgrades caused the problem, and it will be easier to fix if we know that the only thing that changed was the gradle version.

[jbduncan]

The travis error in the latest PR is: [...] I don't know why that's happening, but it doesn't jive with the earlier issues.

Agreed!

I would revert that commit and get back to the original error.

Yep, I'll do just that before retiring for tonight. :)

Here's an experiment:
[...]
If any of those break, then we've narrowed down which version of gradle caused the problem, and have a better chance of making progress. If all of those work, then we've narrowed it down to your code changes. I can't imagine how they could change the classpath, so I'm fairly confident that one of the gradle upgrades caused the problem, and it will be easier to fix if we know that the only thing that changed was the gradle version.

Sounds very sensible to me! Thanks again for your help; I'll get back to you with the result of this experiment when I find the time to tackle this PR again. :)

[JLLeitschuh]

So, this new API is only available in Gradle 4.9+
This will officially end support for older versions of Gradle. Are we okay with that?

If we are, this change should prompt a major version bump for the plugin at least.

[nedtwigg]

Here's the summary:

• Spotless is already aggressively lazy, so I'm not sure that we'll see much benefit
• Whether it speeds up Spotless or not, because it attaches to check, it causes all other check tasks to configure, so there might still be benefit

If the only benefit is delayed attachment to check, then we might be able to pull off something with reflection to allow it to work on pre-4.9 and post-4.9 builds. If we see substantial benefit, then we'll evaluate other options. It all depends on what the profiler tells us.

[jbduncan]

@nedtwigg It seems that there's another variable at play here.

When I freshly clone https://github.com/jbduncan/spotless and checkout master (which at time of writing is even with diffplug:master), and run the experiment above, the results are as follows:

• Gradle version 4.6: The following test fails (which I'm certain is because I'm running Windows):
  • com.diffplug.spotless.extra.groovy.GrEclipseFormatterStepTest#testSupportedVersions
  • and if I fix the test above as I did in commit 9271ed3, then the following test class sometimes fails (I've yet to find a way of consistently reproducing the exact failure):
    • com.diffplug.spotless.kotlin.KtLintStepTest
• Gradle version 4.7: Same as 4.6.
• Gradle version 4.8: The following error message is reported if I attempt to run clean, check or any other Gradle task, even if I try to limit it to a subproject as with gradlew :plugin-gradle:clean:

FAILURE: Build failed with an exception.

* Where:
Build file 'C:\Users\Jonathan\dev\Java\IntelliJ\spotless\ide\build.gradle' line: 11

* What went wrong:
A problem occurred evaluating project ':ide'.
> A problem occurred configuring project ':plugin-gradle'.
   > Cannot configure the 'publishing' extension after it has been accessed.

• Gradle version 4.9: Same as 4.8.

My next port of call is to try running the experiment off of an Ubuntu VM.

WDYT about the results so far?

[nedtwigg]

I'm confused :) As of now, your PR is passing CI, which is great! By removing the debug statements, resolving the configuration is delayed. If the PR as-it-is is passing CI on Travis and on your box, then it seems to me that we don't have a problem.

[jbduncan]

If the PR as-it-is is passing CI on Travis and on your box, then it seems to me that we don't have a problem.

But sadly, that apparently isn't the case. On Travis, things are fine, but on my Windows box, things point to the supernatural (or so I like to think 😉).

Let's see if I can get any better results by running the experiment off of my Ubuntu VM. Stay tuned.

[jbduncan]

I finally found the time to tackle this PR again.

Here are the results of running ./gradlew clean; ./gradlew check --parallel on my Ubuntu 18.04 VM:

• Gradle 4.6:

  • Passed

• Gradle 4.7:

  • Passed

• Gradle 4.8:

  • Even just running ./gradlew clean fails with the following message:

FAILURE: Build failed with an exception.

* Where:
Build file '/home/jonathan/dev/spotless/ide/build.gradle' line: 11

* What went wrong:
A problem occurred evaluating project ':ide'.
> A problem occurred configuring project ':plugin-gradle'.
   > Cannot configure the 'publishing' extension after it has been accessed.

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

Deprecated Gradle features were used in this build, making it incompatible with Gradle 5.0.
See https://docs.gradle.org/4.8/userguide/command_line_interface.html#sec:command_line_warnings

BUILD FAILED in 1s

• Gradle 4.9 ([...]):
  • Passed

[jbduncan]

🤦‍♂️ I think you're right @thc202!

I'll reopen this for now. I hope to have the time to benchmark things again over the next few days.

And whilst I'm at it, I'll see if I can benchmark things directly on my Windows setup rather than in a VM; that may reduce the skew of the timings a bit.

[jbduncan]

Alright. New benchmarks, which show some promise!

1. Against jupiter-collection-testers, branch migrate-task-config-avoidance, Spotless 3.15.0-SNAPSHOT built from Task configuration avoidance (WIP) (#269) #277:

   • gradle-profile parameters:
     • `gradle-profiler --benchmark --project-dir= --output-dir=profile-spotless-3.15.0-SNAPSHOT --scenario-file=performance.scenarios clean_spotlessCheck
     • performance.scenarios:

     clean_spotlessCheck {
         tasks = ["spotlessCheck"]
         cleanup-tasks = ["clean"]
     }

   • Result:
     • Mean: 2257.70 ms
     • Min: 2146.00 ms
     • Max: 2364.00 ms
     • HTML result: benchmark.html.txt (remove ".txt" before opening)

2. Against jupiter-collection-testers, branch migrate-task-config-avoidance, Spotless 3.14.0:

   • gradle-profile parameters:
     • `gradle-profiler --benchmark --project-dir= --output-dir=profile-spotless-3.14.0 --scenario-file=performance.scenarios clean_spotlessCheck
     • performance.scenarios:

     clean_spotlessCheck {
         tasks = ["spotlessCheck"]
         cleanup-tasks = ["clean"]
     }

   • Result:
     • Mean: 3071.90 ms
     • Min: 3023.00 ms
     • Max: 3159.00 ms
     • HTML result: benchmark.html.txt

[jbduncan]

@nedtwigg Combining my new benchmarks with @litpho's report over at #269 (comment), I'm now inclined to attempt to get Spotless to use Task Configuration Avoidance in a Gradle-2.14.1-compatible way. WDYT?

[nedtwigg]

• A 30% improvement is significant
• Your experiment shows it's a pretty small change to the code

It seems that it would be a small amount of reflection involved to make this work, and that it would be a noticeable improvement for users of cutting-edge gradle. I'm all for it!

[JLLeitschuh]

@jbduncan Don't hate me for this suggestion, but if you wrote this reflection as a third party library to support the future API and the old API side by side, I'd totally use it in my own plugins. I think you'd find that there are other API consumers that would also love it as well.

You'd probably get such a library featured by the Gradle team's release notes or in the newsletter.
Unless, @eriwen, do you know of anyone who's undertaken this endeavor already?

[jbduncan]

@jbduncan Don't hate me for this suggestion, but if you wrote this reflection as a third party library to support the future API and the old API side by side, I'd totally use it in my own plugins. I think you'd find that there are other API consumers that would also love it as well.

@JLLeitschuh Nice idea! Although, I don't know if or when I'll have the confidence and time to do so, so please don't hold your breath. :)

And probably more importantly, I'll need to think about how I'll even approach implementing it, since I'm still rather inexperienced with reflection.

@nedtwigg @JLLeitschuh If either of you have any thoughts that may allow me to get started faster, I'd love to hear them.

[nedtwigg]

My easiest idea is to do it this way (doesn't involve much reflection)

• Make a new interface SpotlessTaskSetup, which will have two implementations: SpotlessTaskSetupLegacy and SpotlessTaskSetupConfigAvoidance.
• Extract all the task-related code out of SpotlessPlugin and into SpotlessTaskSetup
• SpotlessPlugin will then have a field private final SpotlessTaskSetup taskSetup = SpotlessTaskSetup.detectAppropriateImplementation()

interface SpotlessTaskSetup {
  public static SpotlessTaskSetup detectAppropriateImplementation() {
    try {
      Class.forName("org.gradle.api.tasks.TaskProvider")
      return new com.diffplug.gradle.spotless.SpotlessTaskSetupConfigAvoidance();
    } catch (ClassNotFoundException e) {
      return new com.diffplug.gradle.spotless.SpotlessTaskSetupLegacy();
    }
  }

  void someMethod();
  void someOtherMethod();
}

A couple key things for this to work:

• SpotlessTaskSetupConfigAvoidance can use all the latest API
• SpotlessTaskSetupLegacy needs to not reference any classes which were added after 2.14
• When spotless is used in a legacy environment, we need to make sure that a classloader never tries to load SpotlessTaskSetupConfigAvoidance. That's why we use its fully qualified name in SpotlessTaskSetup::getInstance - to make sure it isn't loaded by a classloader unless org.gradle.api.tasks.TaskProvider exists

A couple key things to make this easy:

• It's okay for SpotlessTaskSetup to have state, and it's okay for it to have an ugly API - making this reusable will be hard. The whole thing can (and probably should) be package-private.
• We'll need to update our project's gradle to the latest - that's fine.
• We'll need to have an integration test to make sure we don't accidentally break compat for old versions.

[jbduncan]

@nedtwigg Wow, thank you very much for such a detailed response (and sorry for only responding just now)!

Since I have time again this weekend, I'll have a go soon at implementing your suggestion and see where that leads me.

Stay tuned!

[jbduncan]

So work has been keeping me busy over the last two weeks, but I've finally found the time to push my most recent changes!

I'm now stuck with understanding how to test my changes, so @nedtwigg @JLLeitschuh if either of you have any thoughts as to how I could do so, I'd love to hear them. :)

[jbduncan]

Specifically, I need help understanding how to test my changes on both Gradle 2.14.1 and Gradle 4.9.

[eriwen]

I've found GradleTest to a useful tool for testing multiple versions of Gradle to verify Gradle plugins.

[nedtwigg]

This looks fantastic! Very cleanly done. Looks to me that it's just a few changes away from being merged and released.

[nedtwigg]

I would actually keep this as withGradleVersion("2.14.1"). That's how we make sure that our legacy support doesn't break.

Then, I would make a new GradleTaskConfigurationAvoidanceTest, which uses gradle 4.9, and asserts that tasks are actually created lazily. I don't see a need to introduce the GradleTest plugin, but maybe @eriwen knows about value that I'm missing.

[JLLeitschuh]

@nedtwigg @jbduncan Is there an easy way to run all the tests twice once with each version?

[nedtwigg]

There might be, but our gradle tests are already pretty darn slow. None of our steps rely on Gradle, so it's really just the SpotlessExtension logic that we need to test. We compile against 4.9, so there's no risk that we'll use API that doesn't exist in 4.9. But we might accidentally use API that was added after 2.14, and that's what we'll catch by always running against 2.14 - problems with 4.9 will be caught at compile-time.

For gradle == 4.9, it's a good idea to test and make sure that the task avoidance is doing what we intend it to do. That will have the added bonus of being an integration test for the 4.9+ logic in general.

[jbduncan]

That all sounds good to me. I will revert this change and create GradleTaskConfigurationAvoidanceTest as requested. :)

[nedtwigg]

I'd like to cut a release by Monday to release the WTP/CSS integration #311 that @fvgh just merged in. No obligation, we can always cut a release later with this PR, just an FYI for any parties interested in finishing up this PR.

[jbduncan]

Hey @nedtwigg, thanks for following up on this! Sadly I won't be ready to meet the Monday release, as my new job has been taking energy away from me, and I'm not sure when I'll be able to work on this PR again, so please go ahead. :)

[jbduncan]

@nedtwigg I've finally managed to find some time again to tackle this PR, so I'll respond to your earlier comments as soon as I can.

@eriwen Thank you very much for the suggestion to use Gradle-Test! However I think @nedtwigg's suggestion makes more sense since it would be more achievable given the amount of free time I have nowadays. :)

[jbduncan]

Hmm. The Gradle Runner-dependent tests seem tightly coupled to expecting that the Gradle version run is >= 4.9. When I revert back to 2.14.1, the tests go a bit berserk and go red in many places.

I know that at least some of those tests go red because I changed them so that they'd pass when run against Gradle 4.9, so they don't pass any longer against 2.14.1. But I don't think all the tests are going red for that reason (and also my brain's a bit tired), so I'm stumped as to how to fix all the tests...

I probably won't be able to tackle any more of this tonight, but @nedtwigg if you have any thoughts that might allow me to move forward when I next tackle this, then I'd love to hear them. :)

[JLLeitschuh]

@jbduncan Feel free to come bother me over in the Gradle Community Slack chat if you have any issues.

[jbduncan]

Cheers @JLLeitschuh!

Wow, okay. It's been a long time since I last looked at this PR.

I'm sorry to say that I no longer have the time and interest to tackle it any further, but I am more than happy to help out anyone who's interested to take on it further!

So, to summarise: the tests fail because they assume that they're running against Gradle 4.9, whereas they really need to be changed so that they can run against Gradle 2.14.1.

[jbduncan]

To anyone who wants to work on this, feel free to copy the contents of this PR into your own branch, and if you need pointers, feel free to tag me with "@jbduncan", and I'll point you in the right direction. :)