-
Notifications
You must be signed in to change notification settings - Fork 466
Support AGP with Kotlin Built-in #4295
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
base: master
Are you sure you want to change the base?
Conversation
rename file to match function name Update test.
12c2bb1 to
4dd9e43
Compare
When using AGP 9, KGP is no longer required. AGP will be responsible for configuring Kotlin compilation. DGP needs to support Kotlin BuiltIn by extracting information from AGP instead of KGP. - Update KotlinAdapter and AndroidAdapter to handle AGP with Kotlin built-in. - Capture classpaths of variants from `AndroidComponentsExtension` instead of the Android extension. - Update captured AGP variant info to include classpaths. - Only apply KotlinAdapter and AndroidAdapter once per project (Remove the `exec()` function, only apply the plugin once using `KotlinAdapter.applyTo()`).
Preparation for testing AGP9. - Add min/max version to `@TestsAndroid` and `@TestsAndroidCompose` filter AGP versions. - Add SemVerRange to help with filtering. - Add AGP9 properties.
| * Need to double-wrap with [Provider] because AGP will only | ||
| * compute the boot classpath after the compilation options have been finalized. | ||
| * Otherwise, AGP throws an exception. |
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.
Does it mean you can't even acquire the Provider object until some point in time? Isn't it a bug?
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 think it's intentional AGP behaviour. The provider depends on compileSdkVersion, which does not use the Provider API.
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.
Actually, I think that comment is based on the old method of getting the bootClasspath from the Android extension.
dokka/dokka-runners/dokka-gradle-plugin/src/main/kotlin/adapters/AndroidAdapter.kt
Line 163 in 7903495
| return providers.provider { androidExt.bootClasspath } |
It doesn't seem to be applicable to AndroidComponentsExtension.
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 removed the double-provider and the AndroidProjectIT test failed:
AndroidExtensionWrapper: could not find AndroidComponentsExtension
java.lang.IllegalStateException: targetCompatibility is not yet finalized
at com.android.build.gradle.internal.CompileOptions.getTargetCompatibility(CompileOptions.kt:52)
at com.android.build.gradle.internal.CompileOptions$AgpDecorated_Decorated.getTargetCompatibility(Unknown Source)
at com.android.build.gradle.internal.tasks.factory.BootClasspathConfigImpl$1.invoke(BootClasspathConfigImpl.kt:57)
at com.android.build.gradle.internal.tasks.factory.BootClasspathConfigImpl$1.invoke(BootClasspathConfigImpl.kt:57)
at com.android.build.gradle.internal.tasks.factory.BootClasspathConfigImpl$bootClasspath$2.invoke(BootClasspathConfigImpl.kt:173)
at com.android.build.gradle.internal.tasks.factory.BootClasspathConfigImpl$bootClasspath$2.invoke(BootClasspathConfigImpl.kt:166)
at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:74)
at com.android.build.gradle.internal.tasks.factory.BootClasspathConfigImpl.getBootClasspath(BootClasspathConfigImpl.kt:166)
at org.gradle.api.internal.provider.DefaultProvider.calculateOwnValue(DefaultProvider.java:73)
at org.gradle.api.internal.provider.AbstractMinimalProvider.calculateOwnPresentValue(AbstractMinimalProvider.java:82)
at org.gradle.api.internal.provider.AbstractMinimalProvider.get(AbstractMinimalProvider.java:102)
at com.android.build.gradle.internal.dsl.SdkComponentsImpl.getBootClasspath(SdkComponentsImpl.kt:63)
at com.android.build.gradle.internal.dsl.SdkComponentsImpl_Decorated.getBootClasspath(Unknown Source)
at org.jetbrains.dokka.gradle.adapters.AndroidExtensionWrapper$Companion$forAndroidComponents$1.<init>(AndroidAdapter.kt:143)
at org.jetbrains.dokka.gradle.adapters.AndroidExtensionWrapper$Companion.forAndroidComponents(AndroidAdapter.kt:135)
at org.jetbrains.dokka.gradle.adapters.AndroidAdapterKt.AndroidExtensionWrapper(AndroidAdapter.kt:101)
at org.jetbrains.dokka.gradle.adapters.AndroidAdapterKt.access$AndroidExtensionWrapper(AndroidAdapter.kt:1)
at org.jetbrains.dokka.gradle.adapters.AndroidAdapter.apply(AndroidAdapter.kt:45)
at org.jetbrains.dokka.gradle.adapters.AndroidAdapter.apply(AndroidAdapter.kt:35)
I'll add the double-provider back.
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.
For me it looks like a bug in AGP. bootClasspath is a Provider which should be lazy, and until we get the value, it should not cause any early initialization and such problems :think\
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'll check with AGP devs...
dokka-runners/dokka-gradle-plugin/src/main/kotlin/adapters/AndroidAdapter.kt
Outdated
Show resolved
Hide resolved
| * If the buildscript classpath is inconsistent, it might not be possible for DGP | ||
| * to react to KGP because the [KotlinBasePlugin] class can't be loaded. | ||
| * If so, DGP will be lenient and not cause errors, | ||
| * but it must display a prominent warning to help users find the problem. |
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.
but what would be my actions as a user if I see this warning? Shall I report it? Shall I do something with the project?
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.
Users can try and fix the problem, workaround it, or report it.
There are too many potential causes and solutions to detail them in an error message.
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.
IMO it's hardly actionable at this point. Maybe there should be some doc page describing potential causes and solutions and we could link it here.
Also, we can describe the consequences of this – the Kotlin autoconfiguration won't work
…ndroidVariantInfo>.collectFrom`
dokka-runners/dokka-gradle-plugin/src/main/kotlin/adapters/AndroidVariantInfo.kt
Outdated
Show resolved
Hide resolved
dokka-runners/dokka-gradle-plugin/src/main/kotlin/adapters/JavaAdapter.kt
Outdated
Show resolved
Hide resolved
| * as well as AGP's kotlin-built-in plugin. | ||
| */ | ||
| internal fun applyTo(project: Project) { | ||
| findKotlinBasePlugins(project)?.all { |
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.
Question: AndroidAdapter additionally has project.plugins.withType<DokkaBasePlugin>().all { - should we have it here?
IMO, we shouldn't have it at all, as those are internal plugins which we apply from DokkaBasePlugin. Others shouldn't apply them anyway.
I'm just trying to be consistent, no more.
| * is applied. | ||
| * | ||
| * [KotlinBasePlugin] is the parent type for the Kotlin/JVM, Kotlin/Multiplatform, Kotlin/JS plugins, | ||
| * as well as AGP's kotlin-built-in plugin. |
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.
So, correct me if I'm wrong: AGP9 comes with dependency on kotlin-gradle-plugin-api of some version (e.g., 2.2.0) and so for the KotlinAdapter to be usable with AGP9 we can use only types from that artefact, right?
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 think in theory, yes, that's right. It's a little more tricky in practice however, since Gradle bundles Kotlin Gradle plugin.
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.
since Gradle bundles Kotlin Gradle plugin.
I don't think, though, it leaks into the classpath. E.g., without applying KGP, I can't use KGP classes.
It might leak when using kotlin-dsl, but only to the build script.
So I'm not sure what you mean here.
I'm only asking about it because I'm curious whether we can now depend solely on kotlin-gradle-plugin-api instead of kotlin-gradle-plugin in DGP, ensuring compatibility with both AGP and KGP. It's not really a big deal, as we have an IT that works fine, and we should test this.
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.
From what I can see, AGP brings the entire KGP (i.e. has dependency on org.jetbrains.kotlin:kotlin-gradle-plugin)
Gradle bundles Kotlin Gradle plugin.
It does not :)
As Oleg mentioned, kotlin-dsl brings KGP as a transitive dependency similarly to what AGP does
dokka-runners/dokka-gradle-plugin/src/main/kotlin/formats/DokkaFormatPlugin.kt
Outdated
Show resolved
Hide resolved
dokka-integration-tests/gradle/projects/it-android-kotlin-jvm-builtin/gradle.properties
Outdated
Show resolved
Hide resolved
dokka-integration-tests/gradle/projects/it-android-kotlin-mp-builtin/gradle.properties
Outdated
Show resolved
Hide resolved
dokka-integration-tests/gradle/projects/it-android-kotlin-mp-builtin/build.gradle.kts
Outdated
Show resolved
Hide resolved
dokka-integration-tests/gradle/projects/it-android-kotlin-mp-builtin/build.gradle.kts
Outdated
Show resolved
Hide resolved
| */ | ||
| @TestsDGPv2 | ||
| @TestsAndroid( | ||
| minAgpVersion = "9.0.0", |
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.
Documentation says, that it should be supported from 8.10.0 (https://developer.android.com/kotlin/multiplatform/plugin#:~:text=kotlin.,better%20integration%20with%20Android%20Studio.).
Can we try to test it also on AGP 8.13.0 (latest AGP8)?
…r to KotlinAdapter and AndroidAdapter
…m.android.kotlin.multiplatform.library` was added in 8.10.0
… (it's only necessary when KGP is present)
- Check based on whether a Variant has any publishable Components. - A Component is publishable if it's a Variant† AND `Variant.buildType == "release"`. --- † I think this is a bit confusing - why does a Variant have Components that might be Variants? Don't ask me...
…dComponentsExtension
| && name.startsWith(MAIN_SOURCE_SET_NAME) | ||
|
|
||
| internal fun applyTo(project: Project) { | ||
| project.pluginManager.apply(type = JavaAdapter::class) |
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.
minor (again, about consistency): we should probably react on java plugins here, and not just apply it uncoditionally. E.g. use project.plugins.withType<JavaBasePlugin>() as with kotlin/android adapters
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.
True - but let's not do it in this PR, since it's not related to AGP support.
| AndroidLibrary, | ||
| AndroidTest, | ||
| AndroidDynamicFeature, | ||
| AndroidKmpLibrary, |
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.
Curious: do we need it here? I mean, I thought, that with that plugin, we should have enough information coming from KGP, and so we should not apply AndroidAdapter in that case.
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 mean AndroidKmpLibrary?
See the test project:
https://github.com/Kotlin/dokka/blob/adam/support-agp-kotlin-builtin/dokka-integration-tests/gradle/projects/it-android-kotlin-mp-builtin/build.gradle.kts
The only AGP plugin is com.android.kotlin.multiplatform.library. If that plugin is present then AndroidAdapter is required.
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 mean AndroidKmpLibrary?
Yeah, I mean specifically this one.
Got it, for some reason, I thought that compilationClasspath from the kotlin compilation itself should be enough.
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.
Does it make sense to extract anything besides androidExt.bootClasspath() in this case in AndroidAdapter?
Don't we get the classpath from Kotlin compilations?
In which cases classpath from Kotlin compilation is not enough?
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.
In which cases classpath from Kotlin compilation is not enough?
If built-in-kotlin isn't enabled, or it's a non-Kotlin project (although DGP doesn't have tests for this).
| */ | ||
| @TestsDGPv2 | ||
| @TestsAndroid( | ||
| minAgpVersion = "8.13.0", |
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.
according to allAgpVersions for those tests, we don't test 8.13.0 - could you fix it please?
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.
thanks, done
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 now see failures for AndroidKotlinMultiplatformBuiltInTest:
FAILURE: Build failed with an exception.
* Where:
Build file '/private/var/folders/_3/ps18c9f56bx2j4fyxt0z7sn40000gp/T/gradle-test5023736801955594378/generate-dokka-HTML-DokkaGradleProjectRunner-/agp-8.13.0_dgp-2.1.0-SNAPSHOT_gradle-8.14.3_kgp-1.9.25/build.gradle.kts' line: 5
* What went wrong:
An exception occurred applying plugin request [id: 'com.android.kotlin.multiplatform.library', version: '8.13.0']
> Failed to apply plugin 'com.android.internal.kotlin.multiplatform.library'.
> Could not create plugin of type 'KotlinMultiplatformAndroidPlugin'.
> Could not generate a decorated class for type KotlinMultiplatformAndroidPlugin.
> org/jetbrains/kotlin/gradle/dsl/HasConfigurableKotlinCompilerOptions
I guess com.android.kotlin.multiplatform.library v8.13 doesn't work...
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.
Oh, it should, but it also needs KGP to be applied (unless built-in-kotlin is enabled)
https://developer.android.com/kotlin/multiplatform/plugin#prerequisites
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.
(unless built-in-kotlin is enabled)
But, AFAIK, it's an addon plugin for kotlin.multiplatform KGP plugin to add android target there - TBH, I don't see how it may work without KGP
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 now see failures for AndroidKotlinMultiplatformBuiltInTest:
The failure is because com.android.kotlin.multiplatform.library is incompatible with Kotlin 1.9, it requires 2.0.0 or higher. Also, note that the test already applies kotlin("multiplatform")
Though, I see in local run there's another failure in agp: 8.13.0, dgp: 2.1.0-SNAPSHOT, gradle: 8.14.3, kgp: 2.0.21 with differences in test data, like
- <div class="platform-hinted " data-platform-hinted="data-platform-hinted"><div class="content sourceset-dependent-content" data-active="" data-togglable=":/androidMain"><div class="symbol monospace"><span class="token keyword">class </span><a href="-integration-test-activity/index.html">IntegrationTestActivity</a> : <a href="https://developer.android.com/reference/kotlin/androidx/appcompat/app/AppCompatActivity.html">AppCompatActivity</a></div><div class="brief "><p class="paragraph">Some Activity implementing <a href="https://developer.android.com/reference/kotlin/androidx/appcompat/app/AppCompatActivity.html">AppCompatActivity</a> from android x</p></div></div></div>
+ <div class="platform-hinted " data-platform-hinted="data-platform-hinted"><div class="content sourceset-dependent-content" data-active="" data-togglable=":/androidMain"><div class="symbol monospace"><span class="token keyword">class </span><a href="-integration-test-activity/index.html">IntegrationTestActivity</a> : <span data-unresolved-link="androidx.appcompat.app/AppCompatActivity///PointingToDeclaration/">AppCompatActivity</span></div><div class="brief "><p class="paragraph">Some Activity implementing <span data-unresolved-link="androidx.appcompat.app/AppCompatActivity///PointingToDeclaration/">AppCompatActivity</span> from android x</p></div></div></div>
But, AFAIK, it's an addon plugin for kotlin.multiplatform KGP plugin to add android target there - TBH, I don't see how it may work without KGP
True
…are 'publishable' (#4231) DGP should only document 'publishable' (i.e. main, non-test) source sets. DGP needs to extract this information from KGP and AGP. For AGP projects, DPG currently uses `KotlinJvmAndroidCompilation.androidVariant` to determine publishability of a source set. However, [this is deprecated in KGP 2.2.10](https://github.com/JetBrains/kotlin/blob/v2.2.10/libraries/tools/kotlin-gradle-plugin/src/common/kotlin/org/jetbrains/kotlin/gradle/targets/jvm/KotlinJvmAndroidCompilation.kt#L38-L39). It will eventually be removed. This is because AGP will implement Kotlin compilation internally (AKA Kotlin built-in). This PR uses an alternative AGP utility: `AndroidComponentsExtension`. DGP extracts Components from this extension and stores it in a separate data structure (to avoid Configuration Cache issues with serialising too much data). If a Component has at least one Variant, it is considered 'publishable'. Each Component is associated with a single KotlinCompilation (matched by name), and each KotlinSourceSet is associated with a list of KotlinCompilations. While using `AndroidComponentsExtension` is required for AGP9, it also works for AGP7 and 8. To reduce complexity, this PR uses the same approach for all AGP versions. --- This PR is the first part of supporting AGP 9 in DGP. The next PR is #4295, which will support and test AGP 9 with and without Kotlin built-in.
| variants.all { | ||
| compileConfiguration.collect("jar") | ||
| //runtimeConfiguration.collect("jar") | ||
| /** Fetch all configuration names used by all variants. */ |
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.
Nit: they are not configuration names
When using AGP 9, KGP is no longer required. AGP will be responsible for configuring Kotlin compilation. DGP needs to support Kotlin Built-in by extracting information from AGP instead of KGP.
Summary of changes
AndroidComponentsExtensioninstead of the Android extension.exec()function, only apply the plugin once usingKotlinAdapter.applyTo()).it-android-kotlin-jvm-builtin- AGP & kotlin-built-in (no KGP plugin).it-android-kotlin-mp-builtin- AGP & kotlin-multiplatform.Integration tests: support AGP 9 testing and version filtering
@TestsAndroidand@TestsAndroidComposefilter AGP versions.