Migrate to AGP 8 APIs and off of legacy variants API #1136
Conversation
| project.tasks | ||
| .matching { it.name == "compile${testVariantSlug}KotlinAndroid" } | ||
| .configureEach { it.dependsOn(writeResourcesTask) } |
There was a problem hiding this comment.
TIL, this has a similar effect to afterEvaluate { tasks.named("...").configure {, just way way way better in terms of laziness, right?
There was a problem hiding this comment.
yep it's a lazier approach and a little more correct, as afterEvaluate doesn't necessarily guarantee it since there can be multiple afterEvaluate levels
| import com.android.build.gradle.BaseExtension | ||
| import com.android.build.gradle.LibraryExtension | ||
| import com.android.build.gradle.TestedExtension | ||
| import com.android.build.gradle.api.BaseVariant |
There was a problem hiding this comment.
For historical context, this was tried before:
Related PR: #980
and comment: #960 (comment)
but it looks like you made it work with less hacking. 👏
paparazzi-gradle-plugin/src/main/java/app/cash/paparazzi/gradle/PaparazziPlugin.kt
Outdated
Show resolved
Hide resolved
| .matching { | ||
| it.name == "compile${testVariantSlug}JavaWithJavac" | ||
| } |
There was a problem hiding this comment.
What do you think about this
| .matching { | |
| it.name == "compile${testVariantSlug}JavaWithJavac" | |
| } | |
| .matching { it.name == "compile${testVariantSlug}JavaWithJavac" } |
for consistency with other similar structures below?
There was a problem hiding this comment.
I just ran spotless and checked in what it output. Not going to deviate from that
paparazzi-gradle-plugin/src/main/java/app/cash/paparazzi/gradle/PaparazziPlugin.kt
Outdated
Show resolved
Hide resolved
| }.configureEach { rawTest -> | ||
| val test = rawTest as Test |
There was a problem hiding this comment.
Did you know this also works? (yay Smart Casts!)
| }.configureEach { rawTest -> | |
| val test = rawTest as Test | |
| }.configureEach { test -> | |
| test as Test |
It might be simpler, but maybe too magical?
There was a problem hiding this comment.
I prefer the practical approach
| @@ -829,7 +831,7 @@ class PaparazziPluginTest { | |||
| assertThat(resourceFileContents[1]).isEqualTo("intermediates/merged_res/debug") | |||
| assertThat(resourceFileContents[4]).isEqualTo("intermediates/assets/debug") | |||
| assertThat(resourceFileContents[5]).isEqualTo("app.cash.paparazzi.plugin.test,com.example.mylibrary,app.cash.paparazzi.plugin.test.module1,app.cash.paparazzi.plugin.test.module2") | |||
| assertThat(resourceFileContents[6]).isEqualTo("src/main/res,src/debug/res") | |||
| assertThat(resourceFileContents[6]).isEqualTo("build/generated/res/resValues/debug,src/debug/res,src/main/res") | |||
There was a problem hiding this comment.
mind the order of debug vs main: https://github.com/cashapp/paparazzi/pull/980/files#r1254387829
It's important because of variant folder precedence, I think.
Related (depending on where the reversal happens): #980 (comment)
There was a problem hiding this comment.
main is not a flavor/build type, so I don't see why this matters? This order is determined by AGP's artifact API, not something we can change anyway
There was a problem hiding this comment.
@kevinzheng-ap, can you please explain in why the order matters (if it does)?
(From the earlier PR I understood it's because Paparazzi/layoutlib is emulating a full-on Android build/runtime, so the folder order determines which resources are loaded at runtime. Consider having a same named resource in src/main/res/values/x.xml and src/debug/res/values/x.xml and what'll be rendered on the screenshot vs on device.)
not something we can change anyway
It's up to Paparazzi how it interprets and uses the AGP (internal?) APIs.
| // exhaustive to avoid potential breaking changes in future AGP releases | ||
| else -> throw IllegalStateException("${extension.javaClass.name} from $plugin is not supported in Paparazzi") | ||
| val extension = project.extensions.getByType(AndroidComponentsExtension::class.java) | ||
| check(extension is LibraryAndroidComponentsExtension || extension is ApplicationAndroidComponentsExtension || extension is DynamicFeatureAndroidComponentsExtension) { |
There was a problem hiding this comment.
Is this still necessary?
Is there maybe a one-to-one mapping between plugin ID and androidComponents { } type we can rely on?
There was a problem hiding this comment.
There's a TestAndroidComponentsExtension option out there, and possibly others. This was just a straightforward reimpl of the previous check
There was a problem hiding this comment.
Sorry, I didn't include enough context in the highlight:
val supportedPlugins = listOf("com.android.application", "com.android.library", "com.android.dynamic-feature")
...
supportedPlugins.forEach { plugin ->
project.plugins.withId(plugin) {
val extension = project.extensions.getByType(AndroidComponentsExtension::class.java)
// At this point we can be quite sure extension is not `TestAndroidComponentsExtension`,
// because that would imply someone applied `com.android.test` plugin,
// which is mutually exclusive to all the others in `supportedPlugins`.anyway, for a 1:1 conversion 👍🏻
|
@ZacSweers mind: 1) rebasing and 2) inlining |
This migrates the plugin to newer AGP 8.0 APIs and using
AndroidComponentsAPIs. This offers a few benefits:Note that
MergeResourcesdoesn't yet have a public artifact, but does have an internal one. Since Paparazzi uses multiple other internal artifact APIs, I figured this was fair game to use.I did leave one extra note as well - there's a
compile*WithJavactask gated on a BaseJavaPlugin block, but this task is actually created by KGP. Didn't adjust it in this PR to reduce scope, but FYI.