Enable gradle worker api by default on detekt tasks #6913

BraisGabin · 2024-01-26T23:02:57Z

Closes #6887
Closes #2957

To get the reasoning behind this change read the related issue.

detekt-ci · 2024-01-26T23:03:14Z

	Warnings
⚠️	It looks like this PR contains functional changes without a corresponding test.
⚠️	It looks like you're editing the un-versioned copy of our website. This affects only users on the 'next' version of detekt, and it's correct only if you intend to document a future change or feature. If you intended to make a change also for the current version of detekt, please make sure you edit the equivalent file inside `website/versioned_docs/` as well.

	Messages
📖	Thank you very much for making our website better ❤️!

Generated by 🚫 dangerJS against 83bcd45

codecov · 2024-01-26T23:05:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.79%. Comparing base (e78a5e1) to head (83bcd45).
Report is 31 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

3flex

Maybe hold off, see #6964 and gradle/gradle#28034

3flex · 2024-02-25T23:14:53Z

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.

BraisGabin · 2024-03-01T11:41:28Z

I was checking thy this failed on jdk11 and jdk8. And I found this: https://ge.detekt.dev/s/6lrnem2j7n3qs/tests/overview?outcome=FAILED

~~Two tests failed because of, I assume, gradle/gradle#28034. So it seems that gradle/gradle#28034 is related with the JDK version.~~

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 7.6 or fix the problem and support worker API on 6.8.3.

BraisGabin · 2024-03-01T13:30:16Z

I need to find a way to disable Worker API for the merge tests otherwise they are not going to pass.

detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektGenerateConfigTask.kt

detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektCreateBaselineTask.kt

detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/Detekt.kt

...gradle-plugin/src/functionalTest/kotlin/io/gitlab/arturbosch/detekt/DetektReportMergeSpec.kt

BraisGabin · 2024-03-08T11:43:43Z

detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektPlugin.kt

-internal const val USE_WORKER_API = "detekt.use.worker.api"
+
+internal fun ProviderFactory.isWorkerApiEnabled(): Boolean {
+    val defaultValue = isGradleVersionAtLeast(major = 7, minor = 6).toString()


Gradle worker API doesn't work on any version below 7.6 so we don't enable it by default for those versions.

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

Yes, I was testing and 7.6 was the first one that work. No idea why it doesn't work on prior versions.

Probably we should look at this and fix it. But I think that is a different issue.

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.

Should be fixed by #7036. I've also rebased #6489 on top of #7036 to prove that it works (functional tests used to fail because of a similar issue).

This code was removed, now Worker API is always enabled by default.

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.

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

detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektPlugin.kt

3flex · 2024-03-12T21:20:15Z

Annoying the build is still failing. I saw this in the log:

    w: Runtime JAR files in the classpath should have the same version. These files were found in the classpath:
        /home/runner/.gradle/wrapper/dists/gradle-6.8.3-bin/7ykxq50lst7lb7wx1nijpicxn/gradle-6.8.3/lib/kotlin-stdlib-1.4.20.jar (version 1.4)
        /home/runner/.gradle/wrapper/dists/gradle-6.8.3-bin/7ykxq50lst7lb7wx1nijpicxn/gradle-6.8.3/lib/kotlin-stdlib-common-1.4.20.jar (version 1.4)
        /home/runner/.gradle/wrapper/dists/gradle-6.8.3-bin/7ykxq50lst7lb7wx1nijpicxn/gradle-6.8.3/lib/kotlin-stdlib-jdk7-1.4.20.jar (version 1.4)
        /home/runner/.gradle/wrapper/dists/gradle-6.8.3-bin/7ykxq50lst7lb7wx1nijpicxn/gradle-6.8.3/lib/kotlin-stdlib-jdk8-1.4.20.jar (version 1.4)
        /home/runner/.gradle/wrapper/dists/gradle-6.8.3-bin/7ykxq50lst7lb7wx1nijpicxn/gradle-6.8.3/lib/kotlin-reflect-1.4.20.jar (version 1.4)
        /home/runner/work/detekt/detekt/detekt-gradle-plugin/build/tmp/functionalTestMinSupportedGradle/work/.gradle-test-kit/caches/jars-8/06130c2f1c60b901150c43a397642dca/kotlin-stdlib-1.9.20.jar (version 1.9)
        /home/runner/work/detekt/detekt/detekt-gradle-plugin/build/tmp/functionalTestMinSupportedGradle/work/.gradle-test-kit/caches/jars-8/a369b37b4f997a63d005d4a5f51fd41f/kotlin-reflect-1.9.20.jar (version 1.9)

So my fix is incomplete but it's strange that this is only occurring in this PR and not on main. I'll look into it.

3flex · 2024-03-16T04:36:28Z

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.

3flex

I'd unfortunately consider this blocked until gradle/gradle#28478 is fixed in Gradle.

BraisGabin · 2024-03-17T10:05:16Z

After #7060 I'll benchmark these again to see how it behaves.

BraisGabin · 2024-03-22T12:03:40Z

I move to draft this again, I need to perform benchmarks again (Note for myself)

detekt-ci added the gradle-plugin label Jan 26, 2024

BraisGabin changed the title ~~Enable gradle worker api~~ Enable gradle worker api by default on detekt tasks Jan 26, 2024

BraisGabin force-pushed the enable-worker-api-by-default branch from d4e9bb4 to ba09253 Compare January 26, 2024 23:05

detekt-ci added the documentation label Jan 26, 2024

vercel bot deployed to Preview January 26, 2024 23:13 View deployment

3flex added this to the 2.0.0 milestone Jan 27, 2024

3flex added the notable changes Marker for notable changes in the changelog label Jan 27, 2024

BraisGabin force-pushed the enable-worker-api-by-default branch from ba09253 to b88e93e Compare February 2, 2024 12:12

vercel bot deployed to Preview February 2, 2024 12:21 View deployment

3flex requested changes Feb 11, 2024

View reviewed changes

BraisGabin force-pushed the enable-worker-api-by-default branch from b88e93e to 23bdc52 Compare March 1, 2024 12:09

vercel bot deployed to Preview March 1, 2024 12:15 View deployment

BraisGabin force-pushed the enable-worker-api-by-default branch from 23bdc52 to 04c42a3 Compare March 1, 2024 13:29

vercel bot deployed to Preview March 1, 2024 13:35 View deployment

BraisGabin marked this pull request as draft March 8, 2024 09:49

BraisGabin force-pushed the enable-worker-api-by-default branch from 04c42a3 to 107ff27 Compare March 8, 2024 10:16

vercel bot deployed to Preview March 8, 2024 10:22 View deployment

github-advanced-security bot found potential problems Mar 8, 2024

View reviewed changes

vercel bot deployed to Preview March 8, 2024 10:58 View deployment

BraisGabin force-pushed the enable-worker-api-by-default branch from 2b13fc3 to 9065dd9 Compare March 8, 2024 10:58

github-advanced-security bot found potential problems Mar 8, 2024

View reviewed changes

vercel bot deployed to Preview March 8, 2024 11:04 View deployment

BraisGabin force-pushed the enable-worker-api-by-default branch from 9065dd9 to d7aead3 Compare March 8, 2024 11:41

BraisGabin commented Mar 8, 2024

View reviewed changes

vercel bot deployed to Preview March 8, 2024 11:47 View deployment

BraisGabin marked this pull request as ready for review March 8, 2024 11:52

BraisGabin requested a review from 3flex March 8, 2024 11:52

3flex reviewed Mar 9, 2024

View reviewed changes

...gradle-plugin/src/testFixtures/kotlin/io/gitlab/arturbosch/detekt/testkit/DslGradleRunner.kt Outdated Show resolved Hide resolved

BraisGabin requested a review from 3flex March 9, 2024 17:40

vercel bot deployed to Preview March 9, 2024 17:44 View deployment

3flex reviewed Mar 11, 2024

View reviewed changes

detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektPlugin.kt Outdated Show resolved Hide resolved

This was referenced Mar 11, 2024

Split Gradle functional tests by Gradle version #7036

Merged

Allow setting default autocorrect mode with Gradle property #7040

Draft

BraisGabin added 5 commits March 12, 2024 12:27

DRY

4ed0987

Allow to define gradle properties

3c04bd9

Enable gradle worker api by default

ec37d5b

Set properties using cli args

ca6d0c1

Always enable WorkerApi

83bcd45

BraisGabin force-pushed the enable-worker-api-by-default branch from 9794130 to 83bcd45 Compare March 12, 2024 11:30

BraisGabin requested a review from 3flex March 12, 2024 11:33

vercel bot deployed to Preview March 12, 2024 11:37 View deployment

3flex mentioned this pull request Mar 16, 2024

Running detekt with Worker API does not work on older Gradle versions #7058

Closed

3flex requested changes Mar 17, 2024

View reviewed changes

This was referenced Mar 17, 2024

Reduce repetition on how we check if the Worker Api is enabled #7064

Merged

Allow to easily add gradle arguments #7065

Merged

BraisGabin marked this pull request as draft March 22, 2024 12:02

cortinico added the blocked label Mar 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable gradle worker api by default on detekt tasks #6913

Enable gradle worker api by default on detekt tasks #6913

BraisGabin commented Jan 26, 2024 •

edited

detekt-ci commented Jan 26, 2024 •

edited

codecov bot commented Jan 26, 2024 •

edited

3flex left a comment •

edited

3flex commented Feb 25, 2024

BraisGabin commented Mar 1, 2024 •

edited

BraisGabin commented Mar 1, 2024

BraisGabin Mar 8, 2024

3flex Mar 9, 2024

BraisGabin Mar 9, 2024

BraisGabin Mar 9, 2024

3flex Mar 11, 2024 •

edited

3flex Mar 11, 2024

BraisGabin Mar 12, 2024

3flex Mar 14, 2024 •

edited

BraisGabin Mar 14, 2024

3flex commented Mar 12, 2024 •

edited

3flex commented Mar 16, 2024

3flex left a comment

BraisGabin commented Mar 17, 2024

BraisGabin commented Mar 22, 2024

Enable gradle worker api by default on detekt tasks #6913

Are you sure you want to change the base?

Enable gradle worker api by default on detekt tasks #6913

Conversation

BraisGabin commented Jan 26, 2024 • edited

detekt-ci commented Jan 26, 2024 • edited

codecov bot commented Jan 26, 2024 • edited

Codecov Report

3flex left a comment • edited

Choose a reason for hiding this comment

3flex commented Feb 25, 2024

BraisGabin commented Mar 1, 2024 • edited

BraisGabin commented Mar 1, 2024

BraisGabin Mar 8, 2024

Choose a reason for hiding this comment

3flex Mar 9, 2024

Choose a reason for hiding this comment

BraisGabin Mar 9, 2024

Choose a reason for hiding this comment

BraisGabin Mar 9, 2024

Choose a reason for hiding this comment

3flex Mar 11, 2024 • edited

Choose a reason for hiding this comment

3flex Mar 11, 2024

Choose a reason for hiding this comment

BraisGabin Mar 12, 2024

Choose a reason for hiding this comment

3flex Mar 14, 2024 • edited

Choose a reason for hiding this comment

BraisGabin Mar 14, 2024

Choose a reason for hiding this comment

3flex commented Mar 12, 2024 • edited

3flex commented Mar 16, 2024

3flex left a comment

Choose a reason for hiding this comment

BraisGabin commented Mar 17, 2024

BraisGabin commented Mar 22, 2024

BraisGabin commented Jan 26, 2024 •

edited

detekt-ci commented Jan 26, 2024 •

edited

codecov bot commented Jan 26, 2024 •

edited

3flex left a comment •

edited

BraisGabin commented Mar 1, 2024 •

edited

3flex Mar 11, 2024 •

edited

3flex Mar 14, 2024 •

edited

3flex commented Mar 12, 2024 •

edited