Remove afterEvaluate wrapper #4159
Conversation
DomainObjectContainer#all is reactive and applies configuration not just to objects currently in the collection, but any objects subsequently added to the collection. `forEach` only applies to objects currently in the collection, but not any new items subsequently added.
3flex
added
gradle-plugin
notable changes
Codecov Report
@@ Coverage Diff @@
## main #4159 +/- ##
=========================================
Coverage 83.44% 83.44%
Complexity 3177 3177
=========================================
Files 465 465
Lines 9104 9104
Branches 1775 1775
=========================================
Hits 7597 7597
Misses 572 572
Partials 935 935 Continue to review full report at Codecov.
|
| project.extensions.getByType(KotlinMultiplatformExtension::class.java).targets.all { target -> | ||
| target.compilations.all { compilation -> |
There was a problem hiding this comment.
Could you help verify that we are not breaking the behavior on KMM? I had a discussion on #4026 that if I remember correctly, @cortinico mentioned that it does not work without afterEvaluate.
There was a problem hiding this comment.
It might have been the use of forEach as mentioned in one of the commits, which did have a failing test until it was fixed.
I asked for cortinico's review already though as I'm aware he's done a lot of work on this part of the plugin and might have suggestions for extra test cases.
There was a problem hiding this comment.
Good call on the target.compilations.forEach() -> targe.compilations.all()
There was a problem hiding this comment.
Sorry folks for being late to the discussion (I was vactioning 🌴). I think that's a great change @3flex 👍 Thanks for looking into it. I could not fully understand what was causing it. Glad to see that the .forEach was the root cause.
| project.extensions.getByType(KotlinMultiplatformExtension::class.java).targets.all { target -> | ||
| target.compilations.all { compilation -> |
There was a problem hiding this comment.
Sorry folks for being late to the discussion (I was vactioning 🌴). I think that's a great change @3flex 👍 Thanks for looking into it. I could not fully understand what was causing it. Glad to see that the .forEach was the root cause.
This reverts commit 44b7660.
Removes usage of
afterEvaluatein the Gradle plugin.This causes race conditions (evidenced by the double use of
afterEvaluatein DetektMultiplatform class) and makes it more difficult for users to customise individual tasks created by the plugin.Generally
afterEvaluateshouldn't be required when Gradle's lazy configuration and task configuration avoidance APIs are used. The only tests that failed when I removed it is the multiplatform Android tests, but switching to reactive DomainObjectContainer#all instead of DomainObjectContainer#forEach fixes that.If there are other use cases that require using
afterEvaluateI'll investigate, and if the test coverage didn't pick up issues that removal will now cause, please let me know so I can address it.Would fix root issue raised in #3428