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

Migrate to AGP 8 APIs and off of legacy variants API #1136

Closed
wants to merge 10 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,21 @@ package app.cash.paparazzi.gradle

import app.cash.paparazzi.gradle.utils.artifactViewFor
import app.cash.paparazzi.gradle.utils.artifactsFor
import com.android.build.api.artifact.SingleArtifact
import com.android.build.api.artifact.impl.ArtifactsImpl
import com.android.build.api.variant.AndroidComponentsExtension
import com.android.build.api.variant.ApplicationAndroidComponentsExtension
import com.android.build.api.variant.DynamicFeatureAndroidComponentsExtension
import com.android.build.api.variant.HasUnitTest
import com.android.build.api.variant.LibraryAndroidComponentsExtension
import com.android.build.api.variant.Variant
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 👏

import com.android.build.gradle.internal.api.TestedVariant
import com.android.build.gradle.internal.dsl.BaseAppModuleExtension
import com.android.build.gradle.internal.dsl.DynamicFeatureExtension
import com.android.build.gradle.internal.publishing.AndroidArtifacts.ArtifactType
import com.android.build.gradle.tasks.MergeSourceSetFolders
import com.android.build.gradle.internal.scope.InternalArtifactType
import org.gradle.api.DefaultTask
import org.gradle.api.DomainObjectSet
import org.gradle.api.Plugin
import org.gradle.api.Project
import org.gradle.api.Task
import org.gradle.api.artifacts.component.ProjectComponentIdentifier
import org.gradle.api.artifacts.type.ArtifactTypeDefinition
import org.gradle.api.artifacts.type.ArtifactTypeDefinition.ARTIFACT_TYPE_ATTRIBUTE
Expand Down Expand Up @@ -74,7 +76,7 @@ class PaparazziPlugin : Plugin<Project> {
}

project.plugins.withId("com.android.library") {
setupPaparazzi(project, project.extensions.getByType(LibraryExtension::class.java).libraryVariants)
setupPaparazzi(project, project.extensions.getByType(LibraryAndroidComponentsExtension::class.java))
}
} else {
val supportedPlugins = listOf("com.android.application", "com.android.library", "com.android.dynamic-feature")
Expand All @@ -86,20 +88,17 @@ class PaparazziPlugin : Plugin<Project> {

supportedPlugins.forEach { plugin ->
project.plugins.withId(plugin) {
val variants = when (val extension = project.extensions.getByType(TestedExtension::class.java)) {
is LibraryExtension -> extension.libraryVariants
is BaseAppModuleExtension -> extension.applicationVariants
is DynamicFeatureExtension -> extension.applicationVariants
// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still necessary?

Is there maybe a one-to-one mapping between plugin ID and androidComponents { } type we can rely on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a TestAndroidComponentsExtension option out there, and possibly others. This was just a straightforward reimpl of the previous check

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍🏻

"${extension.javaClass.name} from $plugin is not supported in Paparazzi"
}
setupPaparazzi(project, variants)
setupPaparazzi(project, extension)
}
}
}
}

private fun <T> setupPaparazzi(project: Project, variants: DomainObjectSet<T>) where T : BaseVariant, T : TestedVariant {
private fun setupPaparazzi(project: Project, extension: AndroidComponentsExtension<*, *, *>) {
project.addTestDependency()
val nativePlatformFileCollection = project.setupNativePlatformDependency()

Expand All @@ -113,115 +112,149 @@ class PaparazziPlugin : Plugin<Project> {
it.description = "Record golden images for all variants"
}

variants.all { variant ->
val variantSlug = variant.name.capitalize(Locale.US)
val testVariant = variant.unitTestVariant ?: return@all

val mergeResourcesOutputDir = variant.mergeResourcesProvider.flatMap { it.outputDir }
val mergeAssetsProvider =
project.tasks.named("merge${variantSlug}Assets") as TaskProvider<MergeSourceSetFolders>
val mergeAssetsOutputDir = mergeAssetsProvider.flatMap { it.outputDir }
val projectDirectory = project.layout.projectDirectory
val buildDirectory = project.layout.buildDirectory
val gradleUserHomeDir = project.gradle.gradleUserHomeDir
val reportOutputDir = project.extensions.getByType(ReportingExtension::class.java).baseDirectory.dir("paparazzi/${variant.name}")
val snapshotOutputDir = project.layout.projectDirectory.dir("src/test/snapshots")

val localResourceDirs = project
.files(variant.sourceSets.flatMap { it.resDirectories })

// https://android.googlesource.com/platform/tools/base/+/96015063acd3455a76cdf1cc71b23b0828c0907f/build-system/gradle-core/src/main/java/com/android/build/gradle/tasks/MergeResources.kt#875

val moduleResourceDirs = variant.runtimeConfiguration
.artifactsFor(ArtifactType.ANDROID_RES.type) { it is ProjectComponentIdentifier }
.artifactFiles

val aarExplodedDirs = variant.runtimeConfiguration
.artifactsFor(ArtifactType.ANDROID_RES.type) { it !is ProjectComponentIdentifier }
.artifactFiles

val localAssetDirs = project
.files(variant.sourceSets.flatMap { it.assetsDirectories })

// https://android.googlesource.com/platform/tools/base/+/96015063acd3455a76cdf1cc71b23b0828c0907f/build-system/gradle-core/src/main/java/com/android/build/gradle/tasks/MergeResources.kt#875

val moduleAssetDirs = variant.runtimeConfiguration
.artifactsFor(ArtifactType.ASSETS.type) { it is ProjectComponentIdentifier }
.artifactFiles

val aarAssetDirs = variant.runtimeConfiguration
.artifactsFor(ArtifactType.ASSETS.type) { it !is ProjectComponentIdentifier }
.artifactFiles

val packageAwareArtifactFiles = variant.runtimeConfiguration
.artifactsFor(ArtifactType.SYMBOL_LIST_WITH_PACKAGE_NAME.type)
.artifactFiles

val writeResourcesTask = project.tasks.register(
"preparePaparazzi${variantSlug}Resources",
PrepareResourcesTask::class.java
) { task ->
val android = project.extensions.getByType(BaseExtension::class.java)
val nonTransitiveRClassEnabled =
(project.findProperty("android.nonTransitiveRClass") as? String)?.toBoolean() ?: true

task.packageName.set(android.packageName())
task.artifactFiles.from(packageAwareArtifactFiles)
task.nonTransitiveRClassEnabled.set(nonTransitiveRClassEnabled)
task.mergeResourcesOutputDir.set(buildDirectory.asRelativePathString(mergeResourcesOutputDir))
task.targetSdkVersion.set(android.targetSdkVersion())
task.compileSdkVersion.set(android.compileSdkVersion())
task.mergeAssetsOutputDir.set(buildDirectory.asRelativePathString(mergeAssetsOutputDir))
task.projectResourceDirs.from(localResourceDirs)
task.moduleResourceDirs.from(moduleResourceDirs)
task.aarExplodedDirs.from(aarExplodedDirs)
task.projectAssetDirs.from(localAssetDirs.plus(moduleAssetDirs))
task.aarAssetDirs.from(aarAssetDirs)
task.paparazziResources.set(buildDirectory.file("intermediates/paparazzi/${variant.name}/resources.txt"))
}

val testVariantSlug = testVariant.name.capitalize(Locale.US)
extension.onVariants { variant ->
configureVariant(
variant,
project,
recordVariants,
verifyVariants,
nativePlatformFileCollection
)
}
}

project.plugins.withType(JavaBasePlugin::class.java) {
project.tasks.named("compile${testVariantSlug}JavaWithJavac")
.configure { it.dependsOn(writeResourcesTask) }
private fun configureVariant(
variant: Variant,
project: Project,
recordVariants: TaskProvider<Task>,
verifyVariants: TaskProvider<Task>,
nativePlatformFileCollection: FileCollection
) {
val variantSlug = variant.name.capitalize(Locale.US)
val testVariant = (variant as? HasUnitTest)?.unitTest ?: return

val mergeAssetsProvider = variant.artifacts.get(SingleArtifact.ASSETS)
// TODO use public API when one's available: https://issuetracker.google.com/issues/304746899
val mergeResourcesProvider = (variant.artifacts as ArtifactsImpl).get(InternalArtifactType.MERGED_RES)
val projectDirectory = project.layout.projectDirectory
val buildDirectory = project.layout.buildDirectory
val gradleUserHomeDir = project.gradle.gradleUserHomeDir
val reportOutputDir =
project.extensions.getByType(ReportingExtension::class.java).baseDirectory.dir("paparazzi/${variant.name}")
val snapshotOutputDir = project.layout.projectDirectory.dir("src/test/snapshots")

val localResourceDirs = variant.sources.res?.all

// https://android.googlesource.com/platform/tools/base/+/96015063acd3455a76cdf1cc71b23b0828c0907f/build-system/gradle-core/src/main/java/com/android/build/gradle/tasks/MergeResources.kt#875

val moduleResourceDirs = variant.runtimeConfiguration
.artifactsFor(ArtifactType.ANDROID_RES.type) { it is ProjectComponentIdentifier }
.artifactFiles

val aarExplodedDirs = variant.runtimeConfiguration
.artifactsFor(ArtifactType.ANDROID_RES.type) { it !is ProjectComponentIdentifier }
.artifactFiles

val localAssetDirs = variant.sources.assets?.all

// https://android.googlesource.com/platform/tools/base/+/96015063acd3455a76cdf1cc71b23b0828c0907f/build-system/gradle-core/src/main/java/com/android/build/gradle/tasks/MergeResources.kt#875

val moduleAssetDirs = variant.runtimeConfiguration
.artifactsFor(ArtifactType.ASSETS.type) { it is ProjectComponentIdentifier }
.artifactFiles

val aarAssetDirs = variant.runtimeConfiguration
.artifactsFor(ArtifactType.ASSETS.type) { it !is ProjectComponentIdentifier }
.artifactFiles

val packageAwareArtifactFiles = variant.runtimeConfiguration
.artifactsFor(ArtifactType.SYMBOL_LIST_WITH_PACKAGE_NAME.type)
.artifactFiles

val writeResourcesTask = project.tasks.register(
"preparePaparazzi${variantSlug}Resources",
PrepareResourcesTask::class.java
) { task ->
val android = project.extensions.getByType(BaseExtension::class.java)
val nonTransitiveRClassEnabled =
(project.findProperty("android.nonTransitiveRClass") as? String)?.toBoolean() ?: true

task.packageName.set(android.packageName())
task.artifactFiles.from(packageAwareArtifactFiles)
task.nonTransitiveRClassEnabled.set(nonTransitiveRClassEnabled)
task.targetSdkVersion.set(android.targetSdkVersion())
task.compileSdkVersion.set(android.compileSdkVersion())
task.mergeAssetsOutputDir.set(buildDirectory.asRelativePathString(mergeAssetsProvider))
task.mergeResourcesOutputDir.set(buildDirectory.asRelativePathString(mergeResourcesProvider))
localResourceDirs?.let {
task.projectResourceDirs.from(it)
}
task.moduleResourceDirs.from(moduleResourceDirs)
task.aarExplodedDirs.from(aarExplodedDirs)
localAssetDirs?.let {
task.projectAssetDirs.from(it)
}
task.projectAssetDirs.from(moduleAssetDirs)
task.aarAssetDirs.from(aarAssetDirs)
task.paparazziResources.set(buildDirectory.file("intermediates/paparazzi/${variant.name}/resources.txt"))
}

project.plugins.withType(KotlinMultiplatformPluginWrapper::class.java) {
val multiplatformExtension =
project.extensions.getByType(KotlinMultiplatformExtension::class.java)
check(multiplatformExtension.targets.any { target -> target is KotlinAndroidTarget }) {
"There must be an Android target configured when using Paparazzi with the Kotlin Multiplatform Plugin"
val testVariantSlug = testVariant.name.capitalize(Locale.US)

// TODO this task isn't actually produced by the JavaBasePlugin, this is a Kotlin task
project.plugins.withType(JavaBasePlugin::class.java) {
project.tasks
.matching {
it.name == "compile${testVariantSlug}JavaWithJavac"
}
Comment on lines +207 to 209
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about this

Suggested change
.matching {
it.name == "compile${testVariantSlug}JavaWithJavac"
}
.matching { it.name == "compile${testVariantSlug}JavaWithJavac" }

for consistency with other similar structures below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran spotless and checked in what it output. Not going to deviate from that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, interesting...

project.tasks.named("compile${testVariantSlug}KotlinAndroid")
.configure { it.dependsOn(writeResourcesTask) }
}
.configureEach { it.dependsOn(writeResourcesTask) }
}

project.plugins.withType(KotlinAndroidPluginWrapper::class.java) {
project.tasks.named("compile${testVariantSlug}Kotlin")
.configure { it.dependsOn(writeResourcesTask) }
project.plugins.withType(KotlinMultiplatformPluginWrapper::class.java) {
val multiplatformExtension =
project.extensions.getByType(KotlinMultiplatformExtension::class.java)
check(multiplatformExtension.targets.any { target -> target is KotlinAndroidTarget }) {
"There must be an Android target configured when using Paparazzi with the Kotlin Multiplatform Plugin"
}
project.tasks
.matching { it.name == "compile${testVariantSlug}KotlinAndroid" }
.configureEach { it.dependsOn(writeResourcesTask) }
Comment on lines +219 to +221
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL, this has a similar effect to afterEvaluate { tasks.named("...").configure {, just way way way better in terms of laziness, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

project.plugins.withType(KotlinAndroidPluginWrapper::class.java) {
project.tasks
.matching { it.name == "compile${testVariantSlug}Kotlin" }
.configureEach { it.dependsOn(writeResourcesTask) }
}

val recordTaskProvider = project.tasks.register("recordPaparazzi$variantSlug", PaparazziTask::class.java) {
val recordTaskProvider =
project.tasks.register("recordPaparazzi$variantSlug", PaparazziTask::class.java) {
it.group = VERIFICATION_GROUP
it.description = "Record golden images for variant '${variant.name}'"
}
recordVariants.configure { it.dependsOn(recordTaskProvider) }
val verifyTaskProvider = project.tasks.register("verifyPaparazzi$variantSlug", PaparazziTask::class.java) {
recordVariants.configure { it.dependsOn(recordTaskProvider) }
val verifyTaskProvider =
project.tasks.register("verifyPaparazzi$variantSlug", PaparazziTask::class.java) {
it.group = VERIFICATION_GROUP
it.description = "Run screenshot tests for variant '${variant.name}'"
}
verifyVariants.configure { it.dependsOn(verifyTaskProvider) }
verifyVariants.configure { it.dependsOn(verifyTaskProvider) }

val isRecordRun = project.objects.property(Boolean::class.java)
val isVerifyRun = project.objects.property(Boolean::class.java)
val isRecordRun = project.objects.property(Boolean::class.java)
val isVerifyRun = project.objects.property(Boolean::class.java)

project.gradle.taskGraph.whenReady { graph ->
isRecordRun.set(recordTaskProvider.map { graph.hasTask(it) })
isVerifyRun.set(verifyTaskProvider.map { graph.hasTask(it) })
}
project.gradle.taskGraph.whenReady { graph ->
isRecordRun.set(recordTaskProvider.map { graph.hasTask(it) })
isVerifyRun.set(verifyTaskProvider.map { graph.hasTask(it) })
}

val testTaskProvider = project.tasks.named("test$testVariantSlug", Test::class.java) { test ->
val testTaskName = "test$testVariantSlug"
project.tasks
.matching {
it is Test && it.name == testTaskName
}
.configureEach { rawTest ->
val test = rawTest as Test
test.systemProperties["paparazzi.test.resources"] =
writeResourcesTask.flatMap { it.paparazziResources.asFile }.get().path
test.systemProperties["paparazzi.project.dir"] = projectDirectory.toString()
Expand All @@ -235,8 +268,8 @@ class PaparazziPlugin : Plugin<Project> {
test.inputs.property("paparazzi.test.record", isRecordRun)
test.inputs.property("paparazzi.test.verify", isVerifyRun)

test.inputs.dir(mergeResourcesOutputDir)
test.inputs.dir(mergeAssetsOutputDir)
test.inputs.dir(mergeAssetsProvider)
test.inputs.dir(mergeResourcesProvider)
test.inputs.files(nativePlatformFileCollection)
.withPropertyName("paparazzi.nativePlatform")
.withPathSensitivity(PathSensitivity.NONE)
Expand All @@ -259,8 +292,11 @@ class PaparazziPlugin : Plugin<Project> {
}
}

recordTaskProvider.configure { it.dependsOn(testTaskProvider) }
verifyTaskProvider.configure { it.dependsOn(testTaskProvider) }
recordTaskProvider.configure {
it.dependsOn(testTaskName)
}
verifyTaskProvider.configure {
it.dependsOn(testTaskName)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import org.gradle.api.provider.Property
import org.gradle.api.tasks.CacheableTask
import org.gradle.api.tasks.Input
import org.gradle.api.tasks.InputFiles
import org.gradle.api.tasks.Optional
import org.gradle.api.tasks.OutputFile
import org.gradle.api.tasks.PathSensitive
import org.gradle.api.tasks.PathSensitivity
Expand All @@ -35,6 +36,7 @@ abstract class PrepareResourcesTask : DefaultTask() {

@Deprecated("legacy resource loading, to be removed in a future release")
@get:Input
@get:Optional
abstract val mergeResourcesOutputDir: Property<String>

@get:Input
Expand Down Expand Up @@ -104,7 +106,9 @@ abstract class PrepareResourcesTask : DefaultTask() {
.use {
it.write(mainPackage)
it.newLine()
it.write(mergeResourcesOutputDir.get())
if (mergeResourcesOutputDir.isPresent) {
it.write(mergeResourcesOutputDir.get())
}
it.newLine()
it.write(targetSdkVersion.get())
it.newLine()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,9 @@ class PaparazziPluginTest {
val secondResourceFile = File(fixtureRoot, "src/test/resources/colors2.xml")

// Original resource
firstResourceFile.copyTo(destResourceFile, overwrite = false)
if (!destResourceFile.exists()) {
firstResourceFile.copyTo(destResourceFile, overwrite = false)
}

// Take 1
val firstRunResult = gradleRunner
Expand Down Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

assertThat(resourceFileContents[7]).isEqualTo("../module1/build/intermediates/packaged_res/debug,../module2/build/intermediates/packaged_res/debug")
assertThat(resourceFileContents[8]).matches("^caches/transforms-3/[0-9a-f]{32}/transformed/external/res\$")
}
Expand All @@ -852,7 +854,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")
assertThat(resourceFileContents[7]).isEqualTo("../module1/build/intermediates/packaged_res/debug,../module2/build/intermediates/packaged_res/debug")
assertThat(resourceFileContents[8]).matches("^caches/transforms-3/[0-9a-f]{32}/transformed/external/res\$")
}
Expand Down