Plugin: Fix Gradle configuration cache by not keeping classloader as a nested task field

[mateuszkwiecinski]

Fixes #3563

I tried to reproduce the issue in tests, however I'm unable to trigger the cannot serialize object of type 'java.net.URLClassLoader', a subtype of 'java.lang.ClassLoader' error there :/ I noticed tests are passing the dry-run flag which calls simpler DryRunInvoker, but even after adding a test case with the DefaultCliInvoker the test still doesn't fail.
I'd love to hear some ideas how to cover the linked scenario in tests 👀

I verified manually on a failing project changes I'm suggesting work 🤷

[picklebento]

We would need to use DslGradleRunner which leverages the TestKit to pass in the additional --configuration-cache.

The change looks good to me though.

[codecov]

Codecov Report

Merging #3822 (4dbdf3e) into main (211f3f0) will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #3822   +/-   ##
=========================================
  Coverage     83.57%   83.58%           
  Complexity     3110     3110           
=========================================
  Files           456      456           
  Lines          8933     8931    -2     
  Branches       1749     1748    -1     
=========================================
- Hits           7466     7465    -1     
  Misses          551      551           
+ Partials        916      915    -1     

Impacted Files	Coverage Δ
...n/kotlin/io/github/detekt/report/html/HtmlUtils.kt	100.00% <0.00%> (+2.56%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 211f3f0...4dbdf3e. Read the comment docs.

[mateuszkwiecinski]

The test you mentioned is already there:

detekt/detekt-gradle-plugin/src/test/kotlin/io/gitlab/arturbosch/detekt/ConfigurationCacheTest.kt

Line 8 in 4a89a25

object ConfigurationCacheTest : Spek({

I expected that removing the dryRun() call there would invoke DefaultCliInvoker which would reveal the serialization issue, but that doesn't happen. Basically, I can't make the test to fail with linked issue on current main branch (I just pushed an updated test which I expected to fail)

[picklebento]

When inspecting the task output in the aforementioned test, I can even observe Stored cache entry for task ':detekt' with cache key beef5daf9ca25f7b5c981f92fb0b68d0, which makes me think if the configuration cache works for write. And test passes mean configuration cache works for read.

On second thought, we could use a real detekt task instead of dry run to verify the configuration cache behavior. We could create another issue for that.

[mateuszkwiecinski]

Wouldn't the

Stored cache entry for task

be a log for regular Gradle Build Cache? For some reason detekt tests use build cache.
AFAIU configuration cache prints different log messages, like "Calculating task graph as no configuration cache is available for tasks" or "Configuration cache entry stored" or "Reusing configuration cache" (and actually that's what the tests makes assertions on)

we could use a real detekt task instead of dry run to verify the configuration cache behavior. We could create another issue for that.

That's right, the real detekt task, aka DefaultCliInvoker breaks Configuration Cache. That's what gradle report points at in the linked issue. And that's why I added a real detekt task call to the test.

I expected it to fail without the fix I applied, but it doesn't - and that's my challenge here :/

I'll make this PR ready for review as it passed the build and the feature seems to be covered with tests, but if anyone feels up to extending them - feel free to use the work I;ve done here 😃

[cortinico]

You should use org.gradle.api.Project.hasProperty instead.

[mateuszkwiecinski]

Good catch 👍

This fortunately wasn't used:

I must have added this by accident