autonomousapps / dependency-analysis-gradle-plugin

Gradle plugin for JVM projects written in Java, Kotlin, Groovy, or Scala; and Android projects written in Java or Kotlin. Provides advice for managing dependencies and other applied plugins
Apache License 2.0
1.75k stars 117 forks source link

Kotlin 2 bug is redeclaring dependencies on child configurations, leading to erroneous advice to remove or change dependencies that don't exist in build scripts #1239

Open ianbrandt opened 1 month ago

ianbrandt commented 1 month ago

Build scan link https://scans.gradle.com/s/lozbs26qkgomy

Plugin version 1.33.0

Gradle version 8.9 and 8.10

JDK version 11.0.24

(Optional) Kotlin and Kotlin Gradle Plugin (KGP) version 2.0.20-Beta2, 2.0.20-RC, and 2.0.20-RC2.

(Optional) Android Gradle Plugin (AGP) version N/A

(Optional) reason output for bugs relating to incorrect advice Numerous, but for example:

> Task :subprojects:time-logger:reason

----------------------------------------
You asked about the dependency 'org.springframework:spring-context:6.1.12 (libs.spring.context)'.
You have been advised to change this dependency to 'integrationTestImplementation' from 'integrationTestApi'.
----------------------------------------

Shortest path from :subprojects:time-logger to org.springframework:spring-context:6.1.12 (libs.spring.context) for integrationTestCompileClasspath:
:subprojects:time-logger
\--- org.springframework:spring-context:6.1.12

Shortest path from :subprojects:time-logger to org.springframework:spring-context:6.1.12 (libs.spring.context) for integrationTestRuntimeClasspath:
:subprojects:time-logger
\--- org.springframework:spring-context:6.1.12

Shortest path from :subprojects:time-logger to org.springframework:spring-context:6.1.12 (libs.spring.context) for compileClasspath:
:subprojects:time-logger
\--- org.springframework:spring-context:6.1.12

Shortest path from :subprojects:time-logger to org.springframework:spring-context:6.1.12 (libs.spring.context) for runtimeClasspath:
:subprojects:time-logger
\--- org.springframework:spring-context:6.1.12

Shortest path from :subprojects:time-logger to org.springframework:spring-context:6.1.12 (libs.spring.context) for testCompileClasspath:
:subprojects:time-logger
\--- org.springframework:spring-context:6.1.12

Shortest path from :subprojects:time-logger to org.springframework:spring-context:6.1.12 (libs.spring.context) for testRuntimeClasspath:
:subprojects:time-logger
\--- org.springframework:spring-context:6.1.12

Source: main
------------
* Exposes 1 class: org.springframework.context.annotation.Configuration (implies api).

Source: test
------------
(no usages)

Source: integrationTest
-----------------------
* Uses 2 classes: org.springframework.context.annotation.Configuration, org.springframework.context.annotation.Import (implies integrationTestImplementation).

Describe the bug Per #1026, I've been using the following in the precompiled script plugins for my custom JVM Test Suite Plugin suites:

configure<DependencyAnalysisSubExtension> {
    abi {
        exclusions {
            excludeSourceSets(testSuiteName) // E.g. "integrationTest"
        }
    }
}

With Kotlin 2.0.20-Beta1 and prior this works, and I don't get any advice to change <testSuiteName>Api dependencies to <testSuiteName>Implementation.

If I upgrade from 2.0.20-Beta1 to 2.0.20-Beta2, 2.0.20-RC, or 2.0.20-RC2, with no other changes, all that advice returns as if I wasn't filtering my test suite source sets from ABI analysis.

To Reproduce

  1. Run gradlew buildHealth.

2.0.20-Beta2 reproducer:

https://github.com/sdkotlin/sd-kotlin-spring-talks/tree/f55c327ee46061982ceebdd745070cc78d5fa831

It does not reproduce with Kotlin 2.0.20-Beta1:

https://github.com/sdkotlin/sd-kotlin-spring-talks/tree/bcc1b9538b173a4b4be8ad7490808feb258d733f

It does not reproduce with a different but similarly-configured project when it's upgraded to Kotlin 2.0.20-Beta2:

https://github.com/sdkotlin/sd-kotlin-talks/tree/0b57a33323aa1d9209762d5a8ab4c652605e877a

Expected behavior No <testSuiteName>Api to <testSuiteName>Implementation advice when ABI filtering the corresponding source sets.

Additional context This seems to be at least a partial regression of #1026 with Kotlin 2.0.20-Beta2+.

ianbrandt commented 1 month ago

I posted a Kotlin Slack thread to ask whether there were any notable changes to how KGP handles source sets in 2.0.20: https://kotlinlang.slack.com/archives/C0KLZSCHF/p1723827452933009.

ianbrandt commented 3 weeks ago

I tested the just-released 2.0.20. No change.

ianbrandt commented 2 weeks ago

Just FYI, I tested with DAGP 2.0.0 and Kotlin 2.0.20. No change.

autonomousapps commented 3 days ago

I am confused! When you use abi.exclusions.excludeSourceSet("integrationTest"), you're telling DAGP that the integrationTest source set does not have an ABI. Therefore, no dependency can be declared on the integrationTestApi configuration, because there functionally is no such configuration from DAGP's perspective.

It sounds like this was broken before but is working now 🤔

The documentation on this seems to be misleading. I can see how it would appear as if the DSL is saying "exclude from ABI analysis, which therefore means we don't say one way or the other about the ABI and *Api configurations," but that simply is not how it works.

Please let me know if I'm misreading the situation. Thanks!

ianbrandt commented 3 days ago

Thank you for looking into this!

I too am confused, because I'm not declaring anything as an integrationTestApi dependency. I've only declared integrationTestImplementation dependencies.

Whereas no advice is given with 2.0.20-Beta1 and before, here is all the advice for my reproducer when I change nothing other than upgrading to 2.0.20-Beta2 or later:

Advice for :subprojects:app
Unused dependencies which should be removed:
  integrationTestImplementation(libs.kotlinx.coroutines.core.jvm)
  integrationTestImplementation(libs.log4j.api.kotlin)
  integrationTestImplementation(libs.springboot.autoconfigure)
  integrationTestImplementation(project(":subprojects:component-scanned-service"))
  integrationTestImplementation(project(":subprojects:time-logger"))
  integrationTestImplementation(project(":subprojects:time-service"))

Dependencies which should be removed or changed to runtime-only:
  integrationTestRuntimeOnly(libs.springboot) (was integrationTestImplementation)

Advice for :subprojects:child-context:domain-service
Existing dependencies which should be modified to be as indicated:
  integrationTestImplementation(libs.spring.context) (was integrationTestApi)
  integrationTestImplementation(libs.springboot) (was integrationTestApi)

Advice for :subprojects:child-context:rest-api
Unused dependencies which should be removed:
  integrationTestApi(libs.log4j.api.kotlin)
  integrationTestImplementation(libs.log4j.api.kotlin)

Existing dependencies which should be modified to be as indicated:
  integrationTestImplementation(project(":subprojects:child-context:domain-service")) (was integrationTestApi)

Dependencies which should be removed or changed to runtime-only:
  integrationTestRuntimeOnly(libs.spring.web) (was integrationTestApi)
  integrationTestRuntimeOnly(libs.spring.web) (was integrationTestImplementation)

Advice for :subprojects:time-logger
Unused dependencies which should be removed:
  integrationTestApi(libs.kotlinx.datetime.jvm)
  integrationTestApi(libs.log4j.api.kotlin)
  integrationTestImplementation(libs.kotlinx.datetime.jvm)
  integrationTestImplementation(libs.log4j.api.kotlin)

Existing dependencies which should be modified to be as indicated:
  integrationTestImplementation(libs.spring.context) (was integrationTestApi)
  integrationTestImplementation(project(":subprojects:time-service")) (was integrationTestApi)

Advice for :subprojects:time-service
Unused dependencies which should be removed:
  integrationTestImplementation(libs.kotlinx.datetime.jvm)

Existing dependencies which should be modified to be as indicated:
  integrationTestImplementation(libs.spring.context) (was integrationTestApi)
  testFixturesApi(libs.kotlinx.datetime.jvm) (was integrationTestApi)

Focusing in on:

Advice for :subprojects:child-context:domain-service
Existing dependencies which should be modified to be as indicated:
  integrationTestImplementation(libs.spring.context) (was integrationTestApi)
  integrationTestImplementation(libs.springboot) (was integrationTestApi)

...those particular dependencies are declared as api, not integrationTestApi:

dependencies {
    api(libs.spring.context)
    api(libs.springboot)
[...]

https://github.com/sdkotlin/sd-kotlin-spring-talks/blob/kotlin-2.0.20-Beta2/subprojects/child-context/domain-service/build.gradle.kts#L8-L9

Looking at:

Advice for :subprojects:app
Unused dependencies which should be removed:
  integrationTestImplementation(libs.kotlinx.coroutines.core.jvm)
  integrationTestImplementation(libs.log4j.api.kotlin)
  integrationTestImplementation(libs.springboot.autoconfigure)
  integrationTestImplementation(project(":subprojects:component-scanned-service"))
  integrationTestImplementation(project(":subprojects:time-logger"))
  integrationTestImplementation(project(":subprojects:time-service"))

Dependencies which should be removed or changed to runtime-only:
  integrationTestRuntimeOnly(libs.springboot) (was integrationTestImplementation)

...those are all implementation, not integrationTestImplementation:

https://github.com/sdkotlin/sd-kotlin-spring-talks/blob/kotlin-2.0.20-Beta2/subprojects/app/build.gradle.kts#L16-L26

Thank you for the explanation of how ABI exclusions work. I agree it's useful for DAGP to advise when something is declared in an *Api configuration for a source set that is declared as not having an ABI. It's particularly useful while KT-63285 is still outstanding, as we can fail the build if any of those KGP testSuiteApi configurations are accidentally used.

autonomousapps commented 3 days ago

...those particular dependencies are declared as api, not integrationTestApi ...those are all implementation, not integrationTestImplementation

wow, what a weird bug, in that case!

autonomousapps commented 3 days ago

I'm afraid this is due to some change in KGP -- I would argue it is a rather extreme bug. At this line, I dropped a breakpoint, and I've attached a screenshot below showing the dependencies that are declared on the integrationTestApi configuration.

Screenshot 2024-09-13 at 3 35 46 PM

As you know, you have not declared those dependencies yourself in your build scripts. My best guess is that KGP is re-declaring upstream dependencies on downstream/child configurations. So, you declare something on api or implementation, and because integrationTestApi (which shouldn't even exist! This is another old KGP bug they still haven't fixed) extends from those other configurations (logically if not literally), KGP is, I guess, re-declaring those dependencies?

There is literally nothing DAGP can do here. It's operating on the information that Gradle gives it, which is based on what the build scripts and plugins in a project are doing. I say it's an extreme bug because it almost just breaks DAGP for Kotlin 2. Plugins should be really reticent to add dependencies, and KGP is a, well, extreme offender in this case.

ianbrandt commented 2 days ago

It seems that may very well be the case. I forgot to post earlier that JetBrains created an issue to look into this:

https://youtrack.jetbrains.com/issue/KT-70871

I'll add a reference to these details there, and follow it for updates.