Skip to content
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

Kotlin 2: bug when using associateWith: 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 this issue Aug 15, 2024 · 12 comments

Comments

@ianbrandt
Copy link

ianbrandt commented Aug 15, 2024

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
Copy link
Author

ianbrandt commented Aug 16, 2024

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 added a commit to sdkotlin/sd-kotlin-spring-talks that referenced this issue Aug 19, 2024
@ianbrandt ianbrandt changed the title Source set ABI filtering seems to stop working with Kotlin 2.0.20-RC Source set ABI filtering seems to stop working with Kotlin 2.0.20-Beta2 Aug 19, 2024
@autonomousapps autonomousapps added bug Something isn't working toolchain:kotlin labels Aug 19, 2024
@autonomousapps autonomousapps added this to the next milestone Aug 19, 2024
@ianbrandt
Copy link
Author

I tested the just-released 2.0.20. No change.

@ianbrandt
Copy link
Author

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

@autonomousapps
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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

wow, what a weird bug, in that case!

@autonomousapps
Copy link
Owner

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.

@autonomousapps autonomousapps removed this from the next milestone Sep 13, 2024
@ianbrandt
Copy link
Author

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.

@autonomousapps autonomousapps changed the title Source set ABI filtering seems to stop working with Kotlin 2.0.20-Beta2 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 Sep 16, 2024
@jjohannes
Copy link
Collaborator

A workaround for this and similar issues is to remove the auto-added dependencies in a withDependencies {} block on the corresponding configuration.

Here is an example to remove the dependency that is automatically added by Gradle's test suites plugin:

testing.suites.withType<JvmTestSuite> {
    // remove automatically added compile time dependencies, as we define them explicitly
    configurations.getByName(sources.implementationConfigurationName) {
        withDependencies { removeIf { it.group == "org.junit.jupiter" && it.name == "junit-jupiter" } }
    }
}

org.example.gradle.feature.test.gradle.kts

I think I said this before in similar issues here: I think every plugin that automatically adds dependencies because it wants to provide some convenience should have an option to turn that off. But as we see on the above example, even the Gradle core plugins do not always offer that.

@autonomousapps autonomousapps changed the title 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 Kotlin 2: bug when using associateWith: redeclaring dependencies on child configurations, leading to erroneous advice to remove or change dependencies that don't exist in build scripts Sep 20, 2024
ianbrandt added a commit to sdkotlin/sd-kotlin-spring-talks that referenced this issue Sep 20, 2024
@ianbrandt
Copy link
Author

ianbrandt commented Sep 20, 2024

Thank you for the workaround idea, @jjohannes! In the case of any overlap, how could you know which implementation dependencies were added to integrationTestImplementation by KGP, and which were user-declared, in order to only remove the KGP-added ones?

gradlew buildHealth:

e: sd-kotlin-spring-talks/subprojects/time-service/src/it/kotlin/org/sdkotlin/springdemo/timeservice/conf/TimeServiceConfigurationIT.kt:3:16
Unresolved reference 'coroutines'.

@autonomousapps
Copy link
Owner

Can you use the whenObjectAdded callback on your integrationTestX configurations? Maybe if you add the callback before calling associateWith, you'll see the deps get added? It's all very order-dependent.

ianbrandt added a commit to sdkotlin/sd-kotlin-talks that referenced this issue Oct 2, 2024
ianbrandt added a commit to sdkotlin/sd-kotlin-spring-talks that referenced this issue Oct 2, 2024
@ianbrandt
Copy link
Author

Rather than trying to hack what KGP is doing, I've decided to just ignoreSourceSet("integrationTest") for now. It's unfortunate to not get DAGP advice for the test suites, but at least it still works for the main source sets, and I need to upgrade to 2.0.20 to work around another bug.

Hopefully JetBrains will reopen KT-70871, or at least respond to our questions there with something that offers a way forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants