New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable gradle worker api by default on detekt tasks #6913
base: main
Are you sure you want to change the base?
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6913 +/- ##
=========================================
Coverage 83.79% 83.79%
Complexity 3941 3941
=========================================
Files 579 579
Lines 12119 12119
Branches 2511 2511
=========================================
Hits 10155 10155
Misses 710 710
Partials 1254 1254 ☔ View full report in Codecov by Sentry. |
d4e9bb4
to
ba09253
Compare
ba09253
to
b88e93e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe hold off, see #6964 and gradle/gradle#28034
Do you want to update the report merge docs to say that the worker API should be disabled if using the report merge feature? Then we can merge. |
I was checking thy this failed on jdk11 and jdk8. And I found this: https://ge.detekt.dev/s/6lrnem2j7n3qs/tests/overview?outcome=FAILED
And the other one seems that we don't support the worker API on old versions of gradle. Regarding these we have two options: increase or minimum supported version of gradle to |
b88e93e
to
23bdc52
Compare
23bdc52
to
04c42a3
Compare
I need to find a way to disable Worker API for the merge tests otherwise they are not going to pass. |
04c42a3
to
107ff27
Compare
detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektGenerateConfigTask.kt
Fixed
Show fixed
Hide fixed
detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektGenerateConfigTask.kt
Fixed
Show fixed
Hide fixed
detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektCreateBaselineTask.kt
Fixed
Show fixed
Hide fixed
detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektCreateBaselineTask.kt
Fixed
Show fixed
Hide fixed
detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/Detekt.kt
Fixed
Show fixed
Hide fixed
detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/Detekt.kt
Fixed
Show fixed
Hide fixed
2b13fc3
to
9065dd9
Compare
...gradle-plugin/src/functionalTest/kotlin/io/gitlab/arturbosch/detekt/DetektReportMergeSpec.kt
Fixed
Show fixed
Hide fixed
...gradle-plugin/src/functionalTest/kotlin/io/gitlab/arturbosch/detekt/DetektReportMergeSpec.kt
Fixed
Show fixed
Hide fixed
...gradle-plugin/src/functionalTest/kotlin/io/gitlab/arturbosch/detekt/DetektReportMergeSpec.kt
Fixed
Show fixed
Hide fixed
...gradle-plugin/src/functionalTest/kotlin/io/gitlab/arturbosch/detekt/DetektReportMergeSpec.kt
Fixed
Show fixed
Hide fixed
9065dd9
to
d7aead3
Compare
internal const val USE_WORKER_API = "detekt.use.worker.api" | ||
|
||
internal fun ProviderFactory.isWorkerApiEnabled(): Boolean { | ||
val defaultValue = isGradleVersionAtLeast(major = 7, minor = 6).toString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gradle worker API doesn't work on any version below 7.6
so we don't enable it by default for those versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What changed in 7.6? Or did you discover that through testing? There's nothing in the 7.6 release notes about workers: https://docs.gradle.org/7.6/release-notes.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was testing and 7.6 was the first one that work. No idea why it doesn't work on prior versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we should look at this and fix it. But I think that is a different issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm 99% sure this is an issue with our test setup and not a problem with this PR. Gradle is passing a Gradle plugin variant that's compatible with Gradle 8.2+ which fails in some scenarios on this earlier Gradle version. I ran into this in #7025 and #6489.
I'll open a new PR to address it, then I'm confident you can remove this check and rebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code was removed, now Worker API is always enabled by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might need to restore this check.
To isolate the issue I tried using 1.23.5 in a standalone Gradle project running 6.8.3. That failed. The earliest version that worked for me is 7.5, though in tests it only works if version is 7.6. So there's an existing issue unrelated to the Gradle version test split, and not directly related to this PR, and also some inconsistency in TestKit test vs running in a separate build.
I think enabling where possible with this check restored makes sense but it's odd that it's not working on older versions. The whole point of the process isolation mode is that the worker's classpath is isolated from Gradle's, so the Gradle version shouldn't matter here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must say that I'm a bit lost on this regard. I don't fully understand all the picture so it's difficult to me to know what's going on.
I'm going to restore it using 7.5
as the minimum version then :)
...gradle-plugin/src/testFixtures/kotlin/io/gitlab/arturbosch/detekt/testkit/DslGradleRunner.kt
Outdated
Show resolved
Hide resolved
detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektPlugin.kt
Outdated
Show resolved
Hide resolved
9794130
to
83bcd45
Compare
Annoying the build is still failing. I saw this in the log:
So my fix is incomplete but it's strange that this is only occurring in this PR and not on |
I think I've had a fundamental misunderstanding of what process isolation actually achieves when using workers. The worker still has Gradle runtime in its classpath, which is why there's different behaviour when running with different Gradle versions. In many cases things work, because the Kotlin version on the Gradle classpath doesn't conflict with detekt's, but in other cases there's an issue because of Kotlin runtime behaviour changes and Gradle 6.8 embeds Kotlin 1.4.20. Worker classloader isolation makes it possible to run tasks with different libraries to the buildscript classpath. Process isolation extends this to allow running tasks with different JDK or system properties to those used for Gradle itself. Neither of these quite describes what we need to execute detekt. TL;DR: The current worker implementation includes Gradle's runtime which conflicts with detekt's runtime. The worker implementation needs to change before we can use it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd unfortunately consider this blocked until gradle/gradle#28478 is fixed in Gradle.
After #7060 I'll benchmark these again to see how it behaves. |
I move to draft this again, I need to perform benchmarks again (Note for myself) |
Closes #6887
Closes #2957
To get the reasoning behind this change read the related issue.