Add support to Kotlin Multiplatform Projects #3453

cortinico · 2021-02-07T23:18:34Z

This PR adds support to generate detekt tasks for Kotlin Multiplatform projects. Specifically, tasks for jvm and android targets have Type Resolution enabled, while js and native don't (as I still haven't found a way to properly invoke the compiler with the correct classpath/dependency).

For a KMM project like this one:

kotlin {
    android()
    ios()
    jvm("jvmDesktop")
    jvm("jvmBackend")
    js()
}

The following tasks will be added to the graph:

detektAndroidDebug - EXPERIMENTAL: Run detekt analysis for target android and source set debug with type resolution.
detektAndroidDebugAndroidTest - EXPERIMENTAL: Run detekt analysis for target android and source set debugAndroidTest with type resolution.
detektAndroidDebugUnitTest - EXPERIMENTAL: Run detekt analysis for target android and source set debugUnitTest with type resolution.
detektAndroidRelease - EXPERIMENTAL: Run detekt analysis for target android and source set release with type resolution.
detektAndroidReleaseUnitTest - EXPERIMENTAL: Run detekt analysis for target android and source set releaseUnitTest with type resolution.
detektIosArm64Main - Run detekt analysis for target iosArm64 and source set main
detektIosArm64Test - Run detekt analysis for target iosArm64 and source set test
detektIosX64Main - Run detekt analysis for target iosX64 and source set main
detektIosX64Test - Run detekt analysis for target iosX64 and source set test
detektJsMain - Run detekt analysis for target js and source set main
detektJsTest - Run detekt analysis for target js and source set test
detektJvmBackendMain - EXPERIMENTAL: Run detekt analysis for target jvmBackend and source set main with type resolution.
detektJvmBackendTest - EXPERIMENTAL: Run detekt analysis for target jvmBackend and source set test with type resolution.
detektJvmDesktopMain - EXPERIMENTAL: Run detekt analysis for target jvmDesktop and source set main with type resolution.
detektJvmDesktopTest - EXPERIMENTAL: Run detekt analysis for target jvmDesktop and source set test with type resolution.

Alongside detektBaseline* tasks for each of them.

I still haven't managed to write documentation for this, since I'd like to kickoff the discussion to understand if this is going in the right direction.

Also with the KMM ecosystem growing quickly, I believe this could be a great addition to the ecosystem.

Related to #3414
Closes #3385

codecov · 2021-02-07T23:30:09Z

Codecov Report

Merging #3453 (8649576) into master (1926972) will increase coverage by 0.45%.
The diff coverage is 88.70%.

@@             Coverage Diff              @@
##             master    #3453      +/-   ##
============================================
+ Coverage     80.26%   80.72%   +0.45%     
- Complexity     2785     2804      +19     
============================================
  Files           454      455       +1     
  Lines          8418     8461      +43     
  Branches       1609     1616       +7     
============================================
+ Hits           6757     6830      +73     
+ Misses          789      758      -31     
- Partials        872      873       +1

Impacted Files	Coverage Δ	Complexity Δ
...ab/arturbosch/detekt/cli/DetektProgressListener.kt	`83.33% <ø> (ø)`	`4.00 <0.00> (ø)`
...io/gitlab/arturbosch/detekt/core/KtFileModifier.kt	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...hub/detekt/metrics/processors/AbstractProcessor.kt	`83.33% <ø> (ø)`	`3.00 <0.00> (ø)`
...trics/processors/AbstractProjectMetricProcessor.kt	`83.33% <ø> (ø)`	`3.00 <0.00> (ø)`
...detekt/metrics/processors/PackageCountProcessor.kt	`91.66% <ø> (ø)`	`3.00 <0.00> (ø)`
...h/detekt/rules/style/MultilineLambdaItParameter.kt	`85.71% <80.00%> (-5.96%)`	`9.00 <2.00> (+3.00)`	⬇️
.../arturbosch/detekt/internal/DetektMultiplatform.kt	`90.90% <90.90%> (ø)`	`3.00 <3.00> (?)`
...kotlin/io/gitlab/arturbosch/detekt/DetektPlugin.kt	`86.66% <100.00%> (+5.71%)`	`10.00 <1.00> (+1.00)`
...bosch/detekt/rules/complexity/MethodOverloading.kt	`92.85% <0.00%> (+1.19%)`	`4.00% <0.00%> (-1.00%)`	⬆️
... and 4 more

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 1926972...abc88ce. Read the comment docs.

chao2zhang

Looking good overall. I have been wondering if you have an example project to do end-to-end test.

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

chao2zhang · 2021-02-08T02:58:07Z

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

+                        else -> false
+                    }
+
+                    val inputSource = compilation.allKotlinSourceSets.map {


The original documentation on KotlinCompilation is missing, so I read the code would like to confirm the logic with the logic at line 26:

allKotlinSourceSets seem to include both the target source set and all its dependants (which I imagine is common). This means we will be running detekt on both target source set + common set in this target-specific task.

Please correct me if my understanding is inaccurate or incorrect.

Yes it's correct. That's how sourcesets are composed:

Task detektAndroidDebug - SourceSets: [ androidDebug commonMain androidMain ] Task detektAndroidDebugAndroidTest - SourceSets: [ androidDebugAndroidTest commonTest androidAndroidTest androidAndroidTestDebug ] Task detektAndroidDebugUnitTest - SourceSets: [ androidDebugUnitTest commonTest androidTest androidTestDebug ] Task detektAndroidRelease - SourceSets: [ androidRelease commonMain androidMain ] Task detektAndroidReleaseUnitTest - SourceSets: [ androidReleaseUnitTest commonTest androidTest androidTestRelease ] Task detektIosArm64Main - SourceSets: [ iosArm64Main commonMain iosMain ] Task detektIosArm64Test - SourceSets: [ iosArm64Test commonTest iosTest ] Task detektIosX64Main - SourceSets: [ iosX64Main commonMain iosMain ] Task detektIosX64Test - SourceSets: [ iosX64Test commonTest iosTest ] Task detektJsMain - SourceSets: [ jsMain commonMain ] Task detektJsTest - SourceSets: [ jsTest commonTest ] Task detektJvmBackendMain - SourceSets: [ jvmBackendMain commonMain ] Task detektJvmBackendTest - SourceSets: [ jvmBackendTest commonTest ] Task detektJvmDesktopMain - SourceSets: [ jvmDesktopMain commonMain ] Task detektJvmDesktopTest - SourceSets: [ jvmDesktopTest commonTest ]

So this means that if there is a finding in the common code, it will be reported for all the tasks until it gets fixed.

The alternative would be to remove the filter from line 26 and make sure that every task is excluding the common* source sets from the input (I had troubles here with type resolution).

The result is that we will have 3 extra tasks that so composed:

Task detektMetadataCommonMain - SourceSets: [ ] Task detektMetadataIosMain - SourceSets: [ ] Task detektMetadataMain - SourceSets: [ commonMain ]

That turns to be really confusing (at least for me) as KMM creates a target called "metadata" for the common code with 3 compilations (main, commonMain and iosMain).

KMM creates a target called "metadata" for the common

If there are tasks already created with the name like compileMetadataMain, I feel detekt should follow the convention.
I have also received some feedback (only a few) that certain rules are doubly reported, and reporting the same issue more than twice seems quite unsatisfactory to our users.

Do you know anyone else with KMM experience? We could seek their opinion if possible.

Do you know anyone else with KMM experience? We could seek their opinion if possible.

I wrote here on Slack https://kotlinlang.slack.com/archives/C3PQML5NU/p1612899305397200 Let's see if we can get someone with KMM experience onboard

Given that KMM is still in Alpha, I think simplicity (not filtering common seems simpler) would make future migration easier since the tooling can be changed dramatically.

I think simplicity (not filtering common seems simpler) would make future migration easier since the tooling can be changed dramatically.

Just to make sure I got you right, you're suggesting we remove filter { it.platformType != common } but we still add it to all the tasks (given we use allKotlinSourceSets)?

I would vote for filter { it.platformType != common } and use kotlinSourceSets instead of allKotlinSourceSets. But I am open to merge this in as-is and get inputs from other KMM experts or wait for feedback. I bet the API may get changed in the near future so it's probably not worth deciding everything at the moment.

I would vote for filter { it.platformType != common } and use kotlinSourceSets instead of allKotlinSourceSets.

Removed the filter + changed. Just as a side note, kotlinSourceSets for Android still includes the common source set, while this is not true for the other targets (that's also one of the reason why I switched to allKotlinSourceSets as we would have had the common targets everywhere.

Here is what kotlinSourceSets and allKotlinSourceSets look like for all the tasks:

### Task detektAndroidDebug allKotlinSourceSets = [androidDebug, commonMain, androidMain] kotlinSourceSets = [androidDebug, commonMain, androidMain] ### Task detektAndroidDebugAndroidTest allKotlinSourceSets = [androidDebugAndroidTest, commonTest, androidAndroidTest, androidAndroidTestDebug] kotlinSourceSets = [androidDebugAndroidTest, commonTest, androidAndroidTest, androidAndroidTestDebug] ### Task detektAndroidDebugUnitTest allKotlinSourceSets = [androidDebugUnitTest, commonTest, androidTest, androidTestDebug] kotlinSourceSets = [androidDebugUnitTest, commonTest, androidTest, androidTestDebug] ### Task detektAndroidRelease allKotlinSourceSets = [androidRelease, commonMain, androidMain] kotlinSourceSets = [androidRelease, commonMain, androidMain] ### Task detektAndroidReleaseUnitTest allKotlinSourceSets = [androidReleaseUnitTest, commonTest, androidTest, androidTestRelease] kotlinSourceSets = [androidReleaseUnitTest, commonTest, androidTest, androidTestRelease] ### Task detektIosArm64Main allKotlinSourceSets = [iosArm64Main, commonMain, iosMain] kotlinSourceSets = [iosArm64Main] ### Task detektIosArm64Test allKotlinSourceSets = [iosArm64Test, commonTest, iosTest] kotlinSourceSets = [iosArm64Test] ### Task detektIosX64Main allKotlinSourceSets = [iosX64Main, commonMain, iosMain] kotlinSourceSets = [iosX64Main] ### Task detektIosX64Test allKotlinSourceSets = [iosX64Test, commonTest, iosTest] kotlinSourceSets = [iosX64Test] ### Task detektJsMain allKotlinSourceSets = [jsMain, commonMain] kotlinSourceSets = [jsMain] ### Task detektJsTest allKotlinSourceSets = [jsTest, commonTest] kotlinSourceSets = [jsTest] ### Task detektJvmBackendMain allKotlinSourceSets = [jvmBackendMain, commonMain] kotlinSourceSets = [jvmBackendMain] ### Task detektJvmBackendTest allKotlinSourceSets = [jvmBackendTest, commonTest] kotlinSourceSets = [jvmBackendTest] ### Task detektJvmDesktopMain allKotlinSourceSets = [jvmDesktopMain, commonMain] kotlinSourceSets = [jvmDesktopMain] ### Task detektJvmDesktopTest allKotlinSourceSets = [jvmDesktopTest, commonTest] kotlinSourceSets = [jvmDesktopTest]

nrobi144 · 2021-02-10T05:38:39Z

@cortinico not sure if it complicates things, but would be nice to have a macosX64 target as well 🙂

cortinico · 2021-02-10T08:22:43Z

@cortinico not sure if it complicates things, but would be nice to have a macosX64 target as well 🙂

That will be added as soon as you add the macosX64() target (it follows the same convention as the ios() one).

cortinico added the gradle-plugin label Feb 7, 2021

Add support to Kotlin Multiplatform Projects

5d1dd4e

cortinico force-pushed the nc/kmp branch from 49c69e8 to 5d1dd4e Compare February 7, 2021 23:24

cortinico mentioned this pull request Feb 7, 2021

Make gradle plugin defaults consider multiplatform sourcesets #3385

Closed

chao2zhang reviewed Feb 8, 2021

View reviewed changes

nrobi144 mentioned this pull request Feb 10, 2021

Try out the KMM version of detekt halcyonmobile/MultiplatformPlayground#12

Open

chao2zhang mentioned this pull request Feb 10, 2021

Set up publishing for Maven Central linkedin/test-butler#109

Merged

cortinico added 2 commits February 11, 2021 16:22

Include PR feedback

b10943b

Fix detekt failures

abc88ce

chao2zhang approved these changes Feb 12, 2021

View reviewed changes

chao2zhang mentioned this pull request Feb 23, 2021

Refactor Gradle integration tests #3489

Merged

chao2zhang merged commit 8350ce2 into detekt:master Feb 23, 2021

cortinico added this to the 1.16.0 milestone Mar 3, 2021

This was referenced Mar 18, 2021

Bump detekt-formatting from 1.15.0 to 1.16.0 Mick8556241/mvpActivitys#2

Closed

Bump detekt-formatting from 1.15.0 to 1.16.0 u2takey/intellij-go-converter#2

Closed

Bump detekt-formatting from 1.15.0 to 1.16.0 getomni/jetbrains#2

Merged

petertrr mentioned this pull request May 12, 2021

The 'detekt' Gradle task does not do anything in Kotlin multi-platform projects #3663

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to Kotlin Multiplatform Projects #3453

Add support to Kotlin Multiplatform Projects #3453

cortinico commented Feb 7, 2021 •

edited

Loading

codecov bot commented Feb 7, 2021 •

edited

Loading

chao2zhang left a comment

chao2zhang Feb 8, 2021

cortinico Feb 8, 2021

chao2zhang Feb 9, 2021

cortinico Feb 9, 2021

chao2zhang Feb 10, 2021

cortinico Feb 10, 2021

chao2zhang Feb 10, 2021

cortinico Feb 11, 2021

nrobi144 commented Feb 10, 2021

cortinico commented Feb 10, 2021

Add support to Kotlin Multiplatform Projects #3453

Add support to Kotlin Multiplatform Projects #3453

Conversation

cortinico commented Feb 7, 2021 • edited Loading

codecov bot commented Feb 7, 2021 • edited Loading

Codecov Report

chao2zhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrobi144 commented Feb 10, 2021

cortinico commented Feb 10, 2021

cortinico commented Feb 7, 2021 •

edited

Loading

codecov bot commented Feb 7, 2021 •

edited

Loading