Support Android Gradle Plugin 9.0#273
Conversation
https://developer.android.com/build/releases/agp-9-0-0-release-notes 1. In AGP 9.0, the old variants API is removed. The new `androidComponents.onVariants` API is callback-based and `Property` values cannot be accessed during configuration. We now compute applicationTargetPackageId by using `defaultConfig.applicationId`, accounting for `applicationIdSuffix` from build types, and we then use `applicationTargetPackageId` to infer the default test variant ID. (We prefer the debug build type suffix since that's typically what's used for testing.) Note: This doesn't account for flavor-specific `applicationIds` or complex flavor dimension combinations, but users can override by explicitly setting `applicationPackageId` in their `testify` extension. 2. `android.defaultConfig { ... }` takes a lambda with receiver, not a lambda with a parameter, so we can remove `it.resValue` and just call `resValue`. 3. In AGP 9.0, `adb` is accessed via `androidComponents.sdkComponents.adb`, and `sdkComponents` isn't available during configuration time. We now defer resolution until execution time.
We now use `AndroidSourceDirectorySet.directories` instead of the deprecated `AndroidSourceDirectorySet.srcDirs`.
The AGP 9 change made `Project.android` use only `ApplicationExtension`, but the `Samples` also include library modules (e.g. :FlixLibrary, :ComposeExtensions). Those use `LibraryExtension`, so `findByType(ApplicationExtension::class.java)` was null and the plugin threw “Gradle project must contain an android closure” during configuration (e.g. when running ./gradlew dependencies). (Library modules are required to set `applicationPackageId` in their `testify` block, so it's OK that `applicationTargetPackageId` returns null in that case.)
|
I've developed a "stacked" PR dfabulich#1 that depends on this PR, that attempts to use the new |
DanielJette
left a comment
There was a problem hiding this comment.
I greatly appreciate your contributions. I'll give this a thorough review and testing this evening, but at a glance, the changes look very good.
| val baseApplicationId = appExtension.defaultConfig.applicationId ?: return null | ||
|
|
||
| // Prefer the debug variant | ||
| if (this.android is AppExtension) { |
There was a problem hiding this comment.
I feel like this might be the opportunity I've been looking for to support all of the variants properly. Instead of trying to infer/guess a single task to extend, it's probably overdue that we evolve to the more modern approach. Handling onVariant and building separate tasks (as appropriate) for each variant is pretty standard in most gradle plugins and I've always felt it kind of weird for the way Testify used to insist on supporting only a single variant.
I haven't really looked at your chained PR, but I'm thinking we just go with a breaking change and modify the plugin to support multiple variants
There was a problem hiding this comment.
I'm already pretty near the edge of my level of understanding for Gradle development, so I'm not sure I've wrapped my head around how to approach coding that.
If we do want to use onVariants to support multiple variants, I think we'd want to do it starting with my stacked PR dfabulich#1
What are you imagining we'd do with this PR? Options include:
- Merge it as-is
- Merge my stacked PR refactor to fetch variants/packageIds from an async store dfabulich/android-testify#1 to my PR branch and then merge the whole thing, which does include
onVariantssupport, but only to maintain the status quo - Don't merge anything until Testify can support multiple variants. (Would you work on that? I'm not sure I'm going to do it…)
There was a problem hiding this comment.
I'll definitely help with accomplishing this. I think the best strategy is to merge this pr, then you can open the stacked PR against main.
Then, I can pull that down and help work on the multi-variant support
There was a problem hiding this comment.
I don't mind have main in an intermediate state where it's not ready for publishing, but is something we can work on collaboratively. Once it's in a good state, I'll bump the versions and release as a 5.0
There was a problem hiding this comment.
OK, it sounds like the next step is for you to do the merge. When that happens, I'll retarget my PR. (LMK if you need anything from me.)
| fun init(project: Project) { | ||
| adbPath = project.android.adbExecutable.absolutePath | ||
| ?: throw GradleException("adb not found. Have you defined an `android` block?") | ||
| adbPathProvider = { |
There was a problem hiding this comment.
I was verifying the changes and found that Plugin:test is failing when run against an emulator. I noticed that the CI verification fails to launch an emulator and so is an incomplete test. This allows for CI to pass, but hides a problem.
The ConfigurationCacheTest fails for all but the first test with the error:
Execution failed for task ':LegacySample:hidePasswords'.
> Failed to create service 'com.android.build.gradle.internal.SdkComponentsBuildService_3e57380e-18fd-4cba-a41e-a52928287d47'.
> Could not create an instance of type com.android.build.gradle.internal.SdkComponentsBuildService.
> Build-scoped services has been closed.
It seems like the adbPathProvider is capturing a stale project and breaking the configuration cache.
I'll try to fix it
There was a problem hiding this comment.
I have a fix here (#274) but I need your changes on main before I can open my PR. So, I'm going to approve and merge your changes.
| val dstFile = if (File(dst).isAbsolute) { | ||
| File(dst) | ||
| } else { | ||
| File(project.projectDir, dst) |
There was a problem hiding this comment.
I tracked down the configuration break to this line. In order to support gradle configuration caching, the project can only be accessed in the provideInput() method. I've fixed this in my follow-up PR here: b900551
What does this change accomplish?
Fixes #271
How have you achieved it?
This upgrade was kinda tricky. You might want to review it commit by commit rather than file by file.
https://developer.android.com/build/releases/agp-9-0-0-release-notes
In my first commit, I upgraded to Kotlin 2.2.0 for compatibility with Gradle 9.1.0 (not the Android Gradle Plugin, Gradle itself).
In my second commit, I upgraded
com.android.tools.build:gradleto the latest in the8.xseries. This gives us full access to the new API, which is mandatory in AGP 9, while still having access to the old API and classes.In my third commit, I adopted the new public interfaces. This introduces some regressions in functionality.
In AGP 9.0, the old variants API is removed. The new
androidComponents.onVariantsAPI is callback-based andPropertyvalues cannot be accessed during configuration.Previously,
GradleProjectExtensions.ktusedthis.android.testVariantsto find the test variant, returning itsapplicationId, falling back to using theapplicationTargetPackageIdif we couldn't find atestVariant. We can't do that any more in AGP 9.applicationTargetPackageIdwas computed fromappExtension.applicationVariants, finding an app variant named "debug" or whose name ends with "debug." We can't do that in AGP 9 anymore, either.applicationTargetPackageIdhad a fallback tothis.android.defaultConfig.applicationId. We now always computeapplicationTargetPackageIdby usingdefaultConfig.applicationId, accounting forapplicationIdSuffixfrom build types, and we then always useapplicationTargetPackageIdto infer the default test variant ID.Note: This doesn't account for flavor-specific
applicationIdsor complex flavor dimension combinations, but users can override by explicitly settingapplicationPackageIdin theirtestifyextension.android.defaultConfig { ... }takes a lambda with receiver, not a lambda with a parameter, so we can removeit.resValueand just callresValueinTestifyPlugin.kt.In AGP 9.0,
adbis accessed viaandroidComponents.sdkComponents.adb, andsdkComponentsisn't available during configuration time. We now defer resolution until execution time.In my fourth commit, I found that
getInstallDebugAndroidTestTask(project)could return null at runtime, so I added null handling. (This could be a separate PR, but, meh.)In my fifth commit, I found that
ScreenshotPullTask.ktwas using the wrong directory. (I'm not sure why.) I ensured that the File path was relative to the projectDir, and that fixed it.In my sixth commit, I fixed a new deprecation warning in
TestifyExtension.kt. AndroidSourceDirectorySet now deprecatessrcDirsin favor ofdirectories, so I'm using that instead.I'd originally submitted this PR with those six commits, but that broke the build, specifically the
FlixLibrarysample was no longer working. In my seventh commit, I fixed library projects, too. (Notably we now can't computeapplicationTargetPackageIdat all for library projects, but it appears that this is OK, because they're required to explicitly setapplicationPackageIdin theirtestifyblock.Scope of Impact and Testing instructions
I tested these changes using my https://github.com/dfabulich/testify-setup-repro repository. I updated
settings.gradle.ktswith this:and changed the root
build.gradle.ktsto remove the version numberI switched back and forth between the
agp-8-repro-with-testsbranch and theagp-9-repro-with-testsbranch, ensuring that I could./gradlew clean assembleDebug app:screenshotRecordin both branches equally well.Having said that, the regressions in functionality mean that folks with tricky Gradle setups will break if we merge this PR and release it.
I couldn't find a good alternative. (Maybe you can?)EDIT: I've developed a "stacked" PR dfabulich#1 that depends on this PR, that attempts to use the new
onVariantsAPI as Google intended it, but it's a much more sweeping refactor. I'm not confident in my ability to test that version of the PR, but I wanted to make you aware of it, in case you'd prefer me to just merge it into this PR and review it all in one go.