-
Notifications
You must be signed in to change notification settings - Fork 467
KGP 2.2.10 + AGP: Use Android components to determine if source-sets are 'publishable' #4231
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
Conversation
rename file to match function name Update test.
whyoleg
left a comment
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.
Is there a way to somehow test (I mean write a test), that it will be enough to support AGP with builtin Kotlin?
dokka-runners/dokka-gradle-plugin/src/main/kotlin/adapters/KotlinAdapter.kt
Outdated
Show resolved
Hide resolved
dokka-runners/dokka-gradle-plugin/src/main/kotlin/adapters/KotlinAdapter.kt
Outdated
Show resolved
Hide resolved
dokka-runners/dokka-gradle-plugin/src/main/kotlin/adapters/KotlinAdapter.kt
Outdated
Show resolved
Hide resolved
dokka-runners/dokka-gradle-plugin/src/main/kotlin/adapters/KotlinAdapter.kt
Show resolved
Hide resolved
dokka-runners/dokka-gradle-plugin/src/main/kotlin/adapters/KotlinAdapter.kt
Show resolved
Hide resolved
|
This change is only necessary to support AGP with Kotlin built-in, which is only supported in AGP v9+. AGP 9 isn't released yet, so we don't need to include this PR in the DGP 2.1.0 release. |
whyoleg
left a comment
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.
Nice!
I do have some top-level comments:
- Please change the PR name and so commit message, as this PR is about a different thing.
- It would be nice to check in which AGP version those new APIs were added - just in case.
- Is it right that this logic will stay the same when we support AGP9, but will be executed only for AGP8?
dokka-runners/dokka-gradle-plugin/src/main/kotlin/adapters/KotlinAdapter.kt
Show resolved
Hide resolved
dokka-runners/dokka-gradle-plugin/src/main/kotlin/adapters/KotlinAdapter.kt
Show resolved
Hide resolved
Sorry, I'm not following, could you write the title you want to see? I was going to update the commit message when squash-merging to use the issue body.
I believe the Components extension has been available since AGP 7.
Yes, this logic will work for AGP9. It will also work for AGP7 and AGP8. (Does this help? I'm not sure I understood your question.) |
I mean, the PR is not just about compatibility with KGP 2.2.10+, but mostly about the improvement of the association of kotlin compilations with AGP variants. And this helps Dokka to be compatible with KGP 2.2.10 at compile-time, as, as we discussed previously, at runtime we would never stumble on KotlinJvmAndroidCompilation.androidVariant having So maybe something like this would be a nice first line for the commit message (and PR name): Update DGP logic for understanding if the Android compilation is `publishable` based on Android componentsWDYT?
Nice, thanks! Though it's highlighted with strikethrough, is it deprecated in AGP9? In any case, it's not a blocker for this PR.
Got it, sorry for the confusion. For some reason, I was expecting that AGP9 would have some more incompatible APIs. From my side, nothing is blocking the PR from merging. Thanks for answering! |
Thanks! That helps. I'll update it.
EDIT: ah, I realised I linked to the wrong one. AGP7 has two |
dokka-runners/dokka-gradle-plugin/src/main/kotlin/internal/findExtensionLenient.kt
Outdated
Show resolved
Hide resolved
dokka-runners/dokka-gradle-plugin/src/main/kotlin/adapters/KotlinAdapter.kt
Show resolved
Hide resolved
| * - `com.android.application` - [com.android.build.api.variant.ApplicationVariant] | ||
| * - `com.android.library` - [com.android.build.api.variant.DynamicFeatureVariant] | ||
| * - `com.android.test` - [com.android.build.api.variant.LibraryVariant] | ||
| * - `com.android.dynamic-feature` - [com.android.build.api.variant.TestVariant] |
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.
We trigger this function with different plugins, like
withPlugin(PluginId.AndroidBase) { collectAndroidVariants(project, androidVariants) }
withPlugin(PluginId.AndroidApplication) { collectAndroidVariants(project, androidVariants) }
withPlugin(PluginId.AndroidLibrary) { collectAndroidVariants(project, androidVariants) }
however, we do not trigger it for the listed com.android.dynamic-feature or com.android.test, for example. Does it mean we cover those by com.android.base?
If yes, why just having com.android.base is not enough and we should explicitly react on com.android.application and com.android.library? The API for fetching variants seems lazy, so from my POV this causes collecting variants multiple times, but it has only a performance effect since duplicates are automatically excluded from the set constructed via androidVariants
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.
we do not trigger it for the listed
com.android.dynamic-featureorcom.android.test, for example. Does it mean we cover those bycom.android.base?
The behaviour by AGP isn't well specified, but I think the answer is 'no'.
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 then that this code does not cover some of the Android family plugins?
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.
Good question...
Thinking again, I think com.android.base is non-functional here. Only the actual plugins should matter. But I'd prefer to keep it, just to be safe.
For com.android.dynamic-feature and com.android.test: I didn't add them here because we haven't had any requests to support them, we don't have any tests for them, and I'm not sure if anyone would ever want to generate Dokka docs for them.
@whyoleg Do you have any opinion on whether DGP should support projects with com.android.dynamic-feature or com.android.test plugins?
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 see that KGP also does perform checks based on multiple ids (not using com.android.base), and I haven't found any tests which use com.android.dynamic-feature.
So I'm leaning more towards having the same behaviour as in KGP - react on all four plugins, even if we don't have tests for them.
Also, AFAIU, with AGP9, users will use kotlin built into those plugins, and this will be less of an issue in the future.
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.

Continuation of #4213
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.androidVariantto determine publishability of a source set. However, this is deprecated in KGP 2.2.10. 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
AndroidComponentsExtensionis 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.