Repository navigation
[AGP 9.1.0 Migration #7] Migrate the flutter-apk copy and per-ABI versionCode to the variant API - #193693
[AGP 9.1.0 Migration #7] Migrate the flutter-apk copy and per-ABI versionCode to the variant API#193693reidbaker-agent wants to merge 3 commits into
Conversation
The application path of the Flutter Gradle Plugin no longer uses the deprecated BaseVariant API (applicationVariants / ApkVariantOutput): - CopyFlutterApksTask copies the variant's SingleArtifact.APK outputs, read through BuiltArtifactsLoader, into build/outputs/flutter-apk under the unchanged names app[-abi][-flavor]-<mode>.apk. assemble<Variant> depends on it. The task has no Project or variant fields and uses an injected FileSystemOperations. - A single naming function in the task produces both the declared @OutputFiles and the copied file names. Its inputs are the ABIs of the variant's outputs (what AGP builds), the flavor, and the build mode. The task fails if AGP built an APK for an ABI that was not declared. - Per-ABI versionCode for --split-per-abi is a read-then-set on VariantOutput.versionCode in onVariants. This is a documented exception to the no-configuration-time-get rule, because the lazy form is circular. The read relies on AGP's compatibility mode (android.compatibility.enableLegacyApi, on by default through 9.x). - The applicationVariants.configureEach block and its @Suppress("DEPRECATION") markers are deleted. The add-to-app module path keeps using libraryVariants until the next phase. Behavior change: Flutter's onVariants callback runs before an app's own androidComponents.onVariants block, while ApkVariantOutput's versionCodeOverride was applied after it. Apps that transform output.versionCode there transform Flutter's offset value (for example arm64-v8a, build 42, x10000: 422000 -> 20420000). The migration doc and the breaking-change page draft describe this and give a recipe that keeps the 422000-style numbers. No divergence warning is emitted, because it would fire on the onVariants pattern that AGP recommends. Part of flutter#166550.
79ef973 to
e15544b
Compare
There was a problem hiding this comment.
Reviewers. This document feels AI generated but I have left it alone since it is not code and might help you review (things like the table for version codes).
| * [outputApks] instead. Those and the files the copy writes are both named by [apkFileName] from | ||
| * the same inputs, so the declared outputs cannot drift from what the task produces. | ||
| */ | ||
| @DisableCachingByDefault(because = "Copying local APKs is faster than unpacking cache archives") |
There was a problem hiding this comment.
How do I know this is true?
There was a problem hiding this comment.
You can't. It was an unmeasured claim, so I replaced it. The annotation now reads
@DisableCachingByDefault(because = "Not worth caching"). That is the same annotation and reason
Gradle puts on its own Copy and Sync tasks (checked with javap -v on
gradle-core-8.9.jar: org.gradle.api.tasks.Copy and Sync both carry
DisableCachingByDefault(because="Not worth caching")). The KDoc says that and makes no
performance claim.
| "Flutter could not read the APK metadata in $apkDir, so it cannot copy the " + | ||
| "APKs to ${destinationDir.get()}. Please file an issue at " + | ||
| "https://github.com/flutter/flutter/issues." | ||
| ) |
There was a problem hiding this comment.
What are some reasons that we would fail to be able to load the apk. I can think of 1) when our name building produces a name that does not match gradle 2) possibly a file permission issue. Anything else?
There was a problem hiding this comment.
BuiltArtifactsLoader.load(dir) returns null in exactly one case: dir/output-metadata.json
does not exist (AGP BuiltArtifactsLoaderImpl.loadFromFile: if (inputFile == null || !inputFile.exists()) return null).
- Name building: not involved.
loadruns before any name is computed, and the names come
from task inputs, not from files on disk. - File permissions: an unreadable file makes
readText()throw anIOException, and
malformed JSON throws too. Neither returns null. - Practical cause: AGP's packaging task always writes the metadata file. A transform that
usestoTransformMany(SingleArtifact.APK)withArtifactTransformationRequestgets it written
by AGP ("this object will abstract away having to deal with [BuiltArtifacts] and manually load
and write the metadata files",ArtifactTransformationRequestKDoc). So null means another
plugin or build script replaced the artifact with plaintoTransformand did not call
BuiltArtifacts.save().
I put this in a comment above the call. The error says "no output-metadata.json", names
SingleArtifact.APK, and asks the user to contact the maintainer of the plugin that transforms
it. It does not ask them to file a Flutter issue. CopyFlutterApksTaskTest asserts both of those.
| // means AGP built an APK that Flutter did not declare as an output. Fail rather than | ||
| // write a file that Gradle does not track. | ||
| if (abi !in declaredAbis) { |
There was a problem hiding this comment.
How would these get mismatched?
There was a problem hiding this comment.
With AGP alone they can't. AGP builds one APK per variant output, and the declared ABIs come from
those same outputs (a disabled output only makes the built set smaller, and that passes). The
check can fail only if another plugin or build script transforms SingleArtifact.APK into APKs
with different ABI filters. I kept it, because without it such a build writes an untracked file
into flutter-apk and up-to-date checks go wrong silently. The behavior is documented in three
places:
- The
CopyFlutterApksTaskclass KDoc. - The error message. It names
SingleArtifact.APKand points at the plugin that transforms it,
not at Flutter. website-page-draft.md, which says what a transform must keep.
I also looked for plugins Flutter developers use that touch the APK:
| Plugin | What it does to the APK | Trips the check? |
|---|---|---|
Walle (Meituan), used by Flutter devs through pub package_by_walle and in #28701 |
writes extra channel APKs after assemble (legacy variant.outputs) |
No |
| VasDolly (Tencent) | reads SingleArtifact.APK with BuiltArtifactsLoader, writes to its own dir |
No |
| packer-ng (archived) | writes extra APKs after assemble |
No |
| AndResGuard (Tencent), Redex | overwrite the APK in place after assemble |
No (metadata and filters unchanged) |
| Sentry, Firebase App Distribution, Gradle Play Publisher | read SingleArtifact.APK only |
No |
toTransformMany(SingleArtifact.APK) + ArtifactTransformationRequest (10 sampled build scripts, mostly renaming or signing) |
AGP writes the metadata | No |
I found no public plugin that uses plain toTransform(SingleArtifact.APK) without writing the
metadata, or that changes ABI filters. DexGuard, Crashlytics, AGConnect and the 360/Legu
hardening tools are closed source, so I couldn't verify them.
| * `packages/flutter_tools/lib/src/android/gradle.dart`, which is how the Flutter tool | ||
| * finds these files. | ||
| */ | ||
| internal fun apkFileName( |
There was a problem hiding this comment.
Is this the only place we build apk files names based on gradle parameters?
There was a problem hiding this comment.
In the Gradle plugin, yes. This PR deletes the other one, the filename += block in the
applicationVariants callback. The Flutter tool rebuilds the names on the reading side in three
places:
| Where | Used for | Name format |
|---|---|---|
CopyFlutterApksTask.apkFileName (FGP) |
writes flutter-apk | app[-abi][-flavor]-<mode>.apk |
listApkPaths, lib/src/android/gradle.dart:1278 |
flutter build apk for app projects |
same |
AndroidApk.fromAndroidProject, lib/src/android/application_package.dart:118-125 |
flutter run/install (no ABI) |
app[-flavor]-<mode>.apk |
_apkFilesFor, lib/src/android/gradle.dart:149 |
add-to-app host (host/outputs/apk) |
app[-flavor]-<abi>-<mode>.apk, which is AGP's own naming, not FGP's |
The FGP and tool sides are kept in sync only by tests on each side: CopyFlutterApksTaskTest,
the tool's gradle_test.dart, and integration tests that read flutter-apk. Nothing ties the two
together. Following the ratchet principle, I'm raising this here rather than changing it in this
PR.
| * seeds `output.versionCode` (with -1 when no versionCode is declared anywhere, as | ||
| * `BaseVariant.getVersionCode` did), so the check for an unset value is only a guard. |
There was a problem hiding this comment.
Temporal words describing previous behavior I thought were banned by the style guide?
There was a problem hiding this comment.
Fixed in 33f3987. The KDoc describes only the current behavior. The history (versionCodeOverride
ordering, manifest fallback) is in item 3, "Per-ABI versionCode", under "Features that must break" in
the migration doc.
| * `ApkVariantOutput.versionCodeOverride`, which this replaces, was applied after such | ||
| * blocks, so for those apps the resulting versionCode differs from earlier Flutter | ||
| * releases. See "Features that must break" in | ||
| * docs/platforms/android/Migrating-Flutter-Gradle-Plugin-to-AGP-public-API.md. |
There was a problem hiding this comment.
I would prefer linking to the website documentation if possible.
There was a problem hiding this comment.
The breaking-change page isn't published yet. Its source is
docs/platforms/android/website-page-draft.md, so the KDoc points at that section ("Setting
per-ABI or per-variant versionCode") and has a TODO(reidbaker) to switch to the docs.flutter.dev
URL, tracked in #193713. The migration doc links the
same issue.
| // The read relies on AGP's compatibility mode (`android.compatibility.enableLegacyApi`, | ||
| // on by default through AGP 9.x). With it off, AGP disallows unsafe reads of this |
There was a problem hiding this comment.
This is probably bad given that the goal is to no longer depend on the legacy api.
There was a problem hiding this comment.
Agreed, and fixed. The plugin no longer reads output.versionCode:
DslVersionCodessnapshotsdefaultConfig.versionCodeand each product flavor's
versionCodeinandroidComponents.finalizeDsl.forVariant(variant.productFlavors)resolves the base the way AGP merges it: the first flavor
in dimension order that sets one, thendefaultConfig.onVariantsonly callsoutput.versionCode.set(...).
Evidence:
-Pandroid.compatibility.enableLegacyApi=falseon AGP 9.3.1 builds and gives 1042/2042/4042.- Re-adding the
.orNullread fails with "Cannot query the value of this property because
configuration of project ':app' has not completed yet". - A strict-mode case was added to
flutter_build_apk_split_per_abi_test. - The unit-test
Propertymock stubs onlyset, so any read fails the test.
Trade-offs, now documented:
- A versionCode declared only in the manifest is not offset. Flutter logs a warning in that case.
- A
finalizeDslcallback that runs after Flutter's and changes versionCode is not seen.
|
|
||
| // Check that `flutter build apk --split-per-abi` generates a versionCode equal to abiIndex * 1000 + buildNumber | ||
| // Check that `flutter build apk --split-per-abi` generates a versionCode equal to | ||
| // abiIndex * 1000 + buildNumber (then multiplied by 10000 when [usingCustomAppGradleFile]), and |
There was a problem hiding this comment.
It comes from #174081 (gmackall), the regression test for #173917. In that issue an F-Droid user
needs versionCode * 10000 + abiCode * 1000 and set it with
output.versionCode.set(output.versionCode.get() * 10000 ...) in their own onVariants. The
fixture copies that transform. Its abiCodes map is left over from the issue snippet and is
only used as a non-null guard. The test checks that an app's onVariants change to versionCode
survives Flutter's per-ABI offset.
There was a problem hiding this comment.
Is there any way to keep the this behavior while migrating to the new api?
There was a problem hiding this comment.
Not without depending on the compatibility mode. The legacy result
(offset * 1000 + <the value the app's onVariants set>) needs Flutter to run after the app's
callback and read the value the app set. That read is exactly what AGP rejects when
enableLegacyApi=false. Gradle Property has no lazy "transform my current value" (the .map
form is circular), so no lazy workaround exists.
- Possible but not recommended: register Flutter's
onVariantsfrom inside its
finalizeDsl, which runs after the app's script has registered its callbacks, and read the
app's value only when compatibility mode is on. I haven't tested this. It keeps the old
numbers only on AGP 9 with the default setting. In strict mode Flutter would then run last and
overwrite absolute values set by the app, which is worse. - Why the order in this PR fits strict mode: with compatibility mode off, apps can't read
output.versionCodeeither, so they have to set absolute values. Flutter running first and the
app's value winning is what lets that work. - Covering [Android] Flutter 3.35.1 cannot customize the version code during compilation. #173917's user:
-Pforce-version-code-ignoring-abi=true(Implement version code overrides for split ABIs #187742) plus
output.versionCode.set(flutter.versionCode * 10000 + abiCode * 1000)gives exactly their
format, in both modes. - Docs: the migration doc and the website draft give the recipe and the before/after table.
There was a problem hiding this comment.
@gmackall I rejected the idea of using code that kept the old behavior when we could get the older apis and only do the version breaking on new AGP versions. I just thought that would be confusing for users.
If AGP 10 ends up allowing the opt out to keep working I am going to be sad that we didnt build a way to keep the old behavior. What do you think?
…ures - Take the per-ABI versionCode base from a finalizeDsl snapshot of the DSL versionCodes (DslVersionCodes) instead of reading output.versionCode, which AGP rejects during configuration when android.compatibility.enableLegacyApi=false. Warn and skip when the DSL declares no versionCode. - Add a strict-mode (enableLegacyApi=false) case to flutter_build_apk_split_per_abi_test. - CopyFlutterApksTask: use Gradle's Copy/Sync caching annotation and reason; explain when the metadata can be missing and when the ABI check can fail; point both errors at the plugin that transforms SingleArtifact.APK instead of asking for a Flutter issue. - Document what an APK transform must keep in website-page-draft.md. - Link the website-page-draft.md references to tracking issue flutter#193713. - Remove temporal wording from KDoc and docs.
reidbaker
left a comment
There was a problem hiding this comment.
meta comment. This pr seems to add quite verbose code comments. I have not reviewed all of them but make sure the code comments add value to future maintainers and are easy to understand or make the code easier to understand.
| * at Flutter. Plugins that only read the APK artifact, or that write extra APKs after `assemble` | ||
| * (as channel-packaging plugins do), do not cause either failure. | ||
| */ | ||
| @DisableCachingByDefault(because = "Not worth caching") |
There was a problem hiding this comment.
This reason is much worse than the reason you gave previously.
There was a problem hiding this comment.
Agreed. "Not worth caching" said nothing. The reason now states two facts you can check: the inputs are APKs already on local disk, and a build-cache entry for this task would store those same bytes again (Gradle caches a task's outputs, and the outputs here are the copied APKs). It makes no speed claim. Fixed in f668780.
| // or build script transforms the APK artifact into APKs with different ABI filters. | ||
| // Fail rather than write a file that Gradle does not track. | ||
| if (abi !in declaredAbis) { | ||
| throw GradleException( |
There was a problem hiding this comment.
duplicate error string? Consider pulling out into a variable.
There was a problem hiding this comment.
Done in f668780. Both errors use transformedApkError(problem, fix), so the shared guidance is written once.
There was a problem hiding this comment.
@gmackall I rejected the idea of using code that kept the old behavior when we could get the older apis and only do the version breaking on new AGP versions. I just thought that would be confusing for users.
If AGP 10 ends up allowing the opt out to keep working I am going to be sad that we didnt build a way to keep the old behavior. What do you think?
…er comments - CopyFlutterApksTask: state why caching is disabled (local copy; a cache entry would duplicate the APKs) and build both errors through one transformedApkError helper. - Trim comments and KDoc added by this PR to what the code does not already say.
|
Re the review note on verbose comments: thanks. I went through every comment this PR adds and kept only what a maintainer can't get from the code: why the snapshot exists (strict mode), the one case it misses (later |
There was a problem hiding this comment.
Code Review
This pull request migrates the Flutter Gradle Plugin's APK copying and per-ABI versionCode configuration to the public Android Gradle Plugin (AGP) variant APIs. It introduces a new CopyFlutterApksTask to copy and rename APKs using SingleArtifact.APK and BuiltArtifactsLoader, replacing the previous doLast copy block. It also adds DslVersionCodes to capture version codes in finalizeDsl and apply per-ABI offsets in onVariants, enabling split-per-abi builds to function when AGP's compatibility mode is disabled. Corresponding unit and integration tests, as well as migration documentation, are updated. Review feedback suggests using tasks.matching instead of tasks.configureEach when configuring the assemble task dependency to avoid configuration performance overhead.
| return | ||
| } | ||
| if (baseVersionCode == null) { | ||
| project.logger.warn( |
There was a problem hiding this comment.
Should this be an error or a warn? The default
flutter/packages/flutter_tools/lib/src/android/gradle.dart
Lines 567 to 569 in d7fe609
| // plugin instance that owns the resolved Flutter SDK and local engine paths. | ||
| val flutterGradlePlugin = this | ||
| val isApplicationProject = FlutterPluginUtils.isFlutterAppProject(projectToAddTasksTo) | ||
| // Set in finalizeDsl, which AGP runs before any onVariants callback. |
There was a problem hiding this comment.
Why not call this in the onVariants callback then?
Description
This is PR 7 of 11 in the AGP 9.1.0 / public
gradle-apimigration stack (#180137, #166550).This pr changes the way splitting by api version codes work. This can be a surprising change but I have not found a work around.
PR 8 adds add-to-app support and as a result deletes a bunch of code that exists to bridge the partial migration. See #19371 specifically https://github.com/flutter/flutter/pull/193717/changes/f668780fd47caca13c03828f49d21572074825d6..a6be16034c8a1a4a0456cd35194b5973cfd4dc75#r4185414230 which is a subset of changes.
Agent authored details
Changes
flutter-apk copy on the variant API.
CopyFlutterApksTask(copyFlutterApks<Variant>) copies the variant'sSingleArtifact.APKoutputs, read throughBuiltArtifactsLoader, intobuild/app/outputs/flutter-apk/. The names areapp[-abi][-flavor]-<mode>.apk, the formatlistApkPathsinlib/src/android/gradle.dartexpects.assemble<Variant>depends on the task.Projector variant fields. It uses injectedFileSystemOperationsand@InputDirectory @PathSensitive(RELATIVE).@DisableCachingByDefault(because = "Not worth caching"), the annotation and reason Gradle uses on its ownCopyandSynctasks.@OutputFiles). The sharedflutter-apkdirectory is not declared, because it would overlap between variants.CopyFlutterApksTask.apkFileName, produces both the declared outputs and the copied files. Its inputs are the ABIs of the variant's outputs, the flavor, and the build mode.Per-ABI versionCode without reading
output.versionCode. For--split-per-abi, the plugin setsoutput.versionCode = ABI_VERSION[abi] * 1000 + baseon eachApplicationVariantoutput with an ABI filter. It never reads the property, because AGP rejects that read during configuration whenandroid.compatibility.enableLegacyApi=false.DslVersionCodestakes a snapshot ofdefaultConfig.versionCodeand each product flavor'sversionCodeinandroidComponents.finalizeDsl.forVariant(variant.productFlavors)resolves the base the way AGP merges it: the first flavor, in dimension order, that sets a versionCode, otherwisedefaultConfig.APK transform errors point at the transform. The copy fails in two cases. Both can happen only if another plugin or build script transforms
SingleArtifact.APK:output-metadata.json;Both error messages name
SingleArtifact.APKand tell the user to contact the maintainer of the plugin that does the transform. They do not ask for a Flutter issue. The task KDoc andwebsite-page-draft.mdsay what a transform must keep. Plugins Flutter developers commonly use (Walle, VasDolly, packer-ng, AndResGuard, Sentry, Firebase App Distribution, Gradle Play Publisher) do not trigger either error; the evidence is in this review reply.Deleted the app-path
applicationVariants.configureEachblock andconfigureAbiVersionCodeOverride, together with their two@Suppress("DEPRECATION")markers and the unsafeas ApkVariantOutputcast. The add-to-app module path (addFlutterDepsForModule,libraryVariants) is left for PR 8.Structure follows the PR 6 review (@mboetger's point that
registerFlutterAssetTasksshould not register the compile task).onVariantscallsconfigureSplitPerAbiVersionCodesandregisterCopyFlutterApksTaskafterregisterFlutterAssetTasks, under the sameisApplicationProject && shouldCompileFlutterForVariantgate. Each helper does one job, and no helper takes a parameter only to pass it on.Docs:
Migrating-Flutter-Gradle-Plugin-to-AGP-public-API.md: decision record 4, the replacement map, and items 2 and 3 of "Features that must break".website-page-draft.md: versionCode recipes that do not readoutput.versionCode, and the rules for APK transforms.Tests:
CopyFlutterApksTaskTest(new, 5 tests).DslVersionCodesTest(new, 5 tests).FlutterPluginTestcases: defaultConfig and flavor offsets, the no-versionCode warning, no split, the force flag, copy wiring, profile/custom build type naming, and assemble wiring. The mockedVariantOutput.versionCodestubs onlyset, so any read fails the test.flutter_build_apk_split_per_abi_test.dart: a fourth test builds withandroid.compatibility.enableLegacyApi=false, and every test asserts each per-ABI APK is influtter-apk.Review response (commit
33f39876b92)Copy/Syncannotation and reason (checked withjavap).loadreturn null? / How would ABIs mismatch?output.versionCodedepends on compatibility modefinalizeDslsnapshot instead, with a compatibility-mode-off integration test.Why removing the deleted code is safe
The deleted code used
AbstractAppExtension.applicationVariants,ApkVariantOutput.versionCodeOverride/getFilter, andpackageApplicationProvider. AGP deprecated these, and they are not available with AGP 9'sandroid.newDsl=true. The replacements (ApplicationVariant.outputs,VariantOutput.versionCode/filters,SingleArtifact.APK,BuiltArtifactsLoader,DslLifecycle.finalizeDsl) are publiccom.android.build.apiAPIs available at the stack's AGP 8.11.1 floor.Behavioral and compatibility notes
versionCode for apps that transform it in their own
onVariants(breaking). Flutter registers itsonVariantscallback when the plugin is applied, so AGP runs it before an app'sandroidComponents.onVariantsblock.versionCodeOverridewas applied after that block. Apps that setversionCodeonly in theandroid {}DSL see no change. An app that multiplies by 10000 (compatibility mode on), built with--build-number 42:versionCodeOverridearmeabi-v7a(1)arm64-v8a(2)x86_64(4)Values stay distinct, keep ABI order, and only go up, so Play upgrades keep working. With compatibility mode off, apps can't read
output.versionCodeeither. For that case the docs give a recipe that sets an absolute value in the app'sonVariants, which replaces Flutter's:offset * 1000 + flutter.versionCode * 10000.A versionCode declared only in the manifest is not offset. The DSL snapshot can't see it, so Flutter logs a warning and leaves the variant's versionCodes unchanged.
A
finalizeDslcallback registered after Flutter's that changes versionCode is not seen. This is documented in theDslVersionCodesKDoc and the migration doc.copyFlutterApks<Variant>tasks appear ingradlew tasksand can be UP-TO-DATE.Two build types that map to the same Flutter mode share a file name. For example,
debugand a debuggablestagingboth writeapp-debug.apk; the deleted code had the same clash. Running both assembles in one./gradlewinvocation makes the two copy tasks alternate. Thefluttertool builds one variant per invocation, so it is unaffected.Integration test coverage of the changed paths
onVariantstransform, compatibility mode offflutter_build_apk_split_per_abi_test.dart(4 tests; checks versionCodes withapkanalyzerand the per-ABI files influtter-apk)android_gradle_asset_merging_test.dart,gradle_libapp_so_packaging_test.dart(incl. flavors),android_plugin_example_app_build_test.dart,isolated/native_assets_flutter_build_test.dart; devicelabflavors_test,gradle_desugar_classes_testandroid_run_flutter_gradle_plugin_tests_test.dartTests run locally
./gradlew testinpackages/flutter_tools/gradle(JDK 17): pass (33 suites, 0 failures)..editorconfigand baseline: clean.dart format/dart analyze --fatal-infoson the changed Dart test: clean.flutter_build_apk_split_per_abi_teston33f39876b92: 4/4 pass.android_gradle_asset_merging_test(4),gradle_libapp_so_packaging_test(4),android_run_flutter_gradle_plugin_tests_test(2) all pass.enableLegacyApi=false(AGP 9.3.1): versionCodes 1042/2042/4042. A negative control that re-adds the read fails with "configuration of project ':app' has not completed yet".free/paidflavors and a customstagingbuild type: APK names and versionCodes match the deleted code.Pre-launch Checklist
///).