Migrate abi build paths to use new abi filtering api #AGP9#181828
Migrate abi build paths to use new abi filtering api #AGP9#181828auto-submit[bot] merged 15 commits intoflutter:masterfrom
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Android ABI filtering logic for non-split builds. The changes migrate from programmatically setting abiFilters on each buildType to configuring them on the defaultConfig using the newer Android Gradle Plugin APIs. This is achieved by introducing a new configureAbiWithoutSplits function in FlutterPlugin.kt. The list of supported ABIs is now encapsulated in a new constant PLATFORM_ABI_LIST in FlutterPluginConstants.kt. Additionally, a helper function getAndroidApplicationExtension has been added to FlutterPluginUtils.kt to centralize access to the ApplicationExtension.
packages/flutter_tools/gradle/src/main/kotlin/FlutterPluginConstants.kt
Outdated
Show resolved
Hide resolved
| internal fun getAndroidApplicationExtension(project: Project): ApplicationExtension { | ||
| return project.extensions.getByType(ApplicationExtension::class.java) | ||
| } |
There was a problem hiding this comment.
For consistency and clarity, it would be beneficial to add KDoc to this new function, similar to getAndroidExtension. Additionally, it can be converted to a more concise single-expression function, which is idiomatic in Kotlin.
| internal fun getAndroidApplicationExtension(project: Project): ApplicationExtension { | |
| return project.extensions.getByType(ApplicationExtension::class.java) | |
| } | |
| /** | |
| * Returns ApplicationExtension for the project. | |
| * | |
| * This is the entry point for configuring the android application. | |
| */ | |
| internal fun getAndroidApplicationExtension(project: Project): ApplicationExtension = | |
| project.extensions.getByType(ApplicationExtension::class.java) |
References
- The style guide recommends optimizing for readability and following the Android Kotlin Style Guide, which encourages writing concise, idiomatic code. Using a single-expression function improves conciseness. While not explicitly for
internalmembers, the guide's emphasis on documentation for public members and even private members suggests that documenting this helper function is good practice. (link)
…overriding in buildTypes
github-actions
bot
added
platform-android
…stants.kt Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the ABI filtering logic for Android builds, moving it from direct manipulation of buildTypes to configuring defaultConfig. This is a good change that allows users to more easily override the default ABI settings. The introduction of helper functions and constants in FlutterPluginUtils and FlutterPluginConstants improves code organization. The accompanying integration tests correctly validate the new behavior, including the use of the disable-abi-filtering flag.
I have one suggestion to make the new ABI configuration logic more robust and less dependent on Gradle's script evaluation order, ensuring that user configurations are always respected as intended.
reidbaker
changed the title
reidbaker
left a comment
reidbaker
left a comment
There was a problem hiding this comment.
adding comments
packages/flutter_tools/test/integration.shard/gradle_jni_packaging_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/integration.shard/gradle_jni_packaging_test.dart
Outdated
Show resolved
Hide resolved
jesswrd
left a comment
jesswrd
left a comment
There was a problem hiding this comment.
Thanks for including the testing command! Just had a few suggestions but lgtm to me.
Is this true? It looks to me like previously, because we set the abiFilters on all types, that flag would be required by a user to set their own abiFilters on a build type. But now that we are only mucking with the defaultConfig, wouldn't they not need that flag now, when setting abiFilters on their own build type? |
| // Developers can set this value by passing `-P disable-abi-filtering=true` | ||
| // to flutter build. Where "disable-abi-filtering" comes from | ||
| // com.flutter.gradle.FlutterPluginUtils.PROP_DISABLE_ABI_FILTERING. |
There was a problem hiding this comment.
nit: I think we should delete this comment. I don't think there is value in explaining the internals of the helper function inline with its use. I think we are overly verbose in comments in this area of the code in general.
There was a problem hiding this comment.
Fair feedback but I disagree. I, when working in this code and with an above average understanding of agp and gradle, and with a way above average understanding of this package structure had to spend time to track down how to set this value. I think if you are reading this code then the comment can be really helpful. I think that when searching for "ABI" in the flutter/flutter repo because you do not understand why abi filtering does not work like you expect you are likely to find this code and less likely to find the flag. I also think that the cost of comments is low.
Given these are opinions I checked the style guide to see if that would give us some guidance at what to include or exclude and it does not cover this topic.
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo/3ec25f128e35aaf0d4862e0f80a1ab38d573e5e0#comments
@jesswrd what do you think?
There was a problem hiding this comment.
I think it could be helpful, as long as the internals of the method don't change. But if they do it is now actively harmful, and it is very unlikely that the person changing the internals of the method will find a comment about its use. I think the internals of the method are the methods business, not comments elsewhere.
There was a problem hiding this comment.
ok then how about I move this comment to the method. Then it is still one jump but it is adjacent to the implementation.
There was a problem hiding this comment.
Sure, I think a comment on the helper method explaining that it makes use of the flutter tool's piping of gradle properties via the -android-project-arg/-P option makes sense, my opposition was only to making a comment at a call site explain the internals of a helper method defined elsewhere
There was a problem hiding this comment.
That sounds good to me. While I do agree this area of FGP is verbose with comments, I appreciate the comments existing. It helps me make sense of what's happening in this area. I don't have strong opinons of where to move the comments—I just care that they exist somewhere.
There was a problem hiding this comment.
Sure, I think a comment on the helper method explaining that it makes use of the flutter tool's piping of gradle properties via the
-android-project-arg/-Poption makes sense, my opposition was only to making a comment at a call site explain the internals of a helper method defined elsewhere
That is good feedback updated.
| 'build', | ||
| 'apk', | ||
| '-P', | ||
| 'disable-abi-filtering=true', |
There was a problem hiding this comment.
Does this test fail without this line? Given that we are only modifying the default config now that would surprise me
There was a problem hiding this comment.
Yes it does. I found this test file because on an early iteration this test failed. I also ran this locally as part of my debugging.
Finally I confirmed by removing the -P disable-abi-filtering=true and running the test locally. Without the flag this test fails, With the flag it passes.
FLUTTER_ROOT=~/flutter-work/ ../../bin/cache/dart-sdk/bin/dart test test/integration.shard/gradle_jni_packaging_test.dart from packages/flutter_tools
There was a problem hiding this comment.
One odd thing, the code passes with disable-abi-filtering=true or disable-abi-filtering=false
There was a problem hiding this comment.
Ok it turns out that the default config gets unioned with, not overridden by, build type specific configuration (see http://issuetracker.google.com/issues/285353854). This explains why the test fails without that line, though it is very surprising behavior by AGP. I left a comment on the issue expressing that.
Co-authored-by: Gray Mackall <[email protected]> Co-authored-by: jesswrd <[email protected]>
…k the value of the property
| ); | ||
|
|
||
| // Verify that libflutter.so is packaged in the APK for Flutter supported architectures. | ||
| expect(_checkLibIsInApk(projectDir, 'lib/arm64-v8a/libflutter.so'), false); |
There was a problem hiding this comment.
should this be true? I noticed the tests are failing on this line, and this is a supported architecture right
There was a problem hiding this comment.
typo I believe, updating locally.
reidbaker
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
…11088) Manual roll Flutter from c023e5b2474f to 91b2d41a66d1 (31 revisions) Manual roll requested by [email protected] flutter/flutter@c023e5b...91b2d41 2026-02-19 [email protected] Reland #179643, only scroll hit-testable primary scroll views on status bar tap (flutter/flutter#182391) 2026-02-19 [email protected] Replace References to `flutter/engine` with `flutter/flutter` (flutter/flutter#182600) 2026-02-19 [email protected] Remove specific iOS extended attributes to fix code signing (flutter/flutter#180710) 2026-02-19 [email protected] Manual roll Skia from 7bbdc51ab0aa to ce5854495a3a (flutter/flutter#182637) 2026-02-19 [email protected] [pv]add integration test for original untappable web view link behind context menu bug (flutter/flutter#182111) 2026-02-19 [email protected] Roll pub packages (flutter/flutter#182579) 2026-02-19 [email protected] Remove Material import from scroll_view_test.dart (flutter/flutter#181281) 2026-02-19 [email protected] Add RawTooltip.ignorePointer (flutter/flutter#182527) 2026-02-19 [email protected] [web] Stop double loading fonts for WebParagraph (flutter/flutter#182026) 2026-02-19 [email protected] Migrate abi build paths to use new abi filtering api #AGP9 (flutter/flutter#181828) 2026-02-19 [email protected] [web] Flutter errors should be reported with console.error() (flutter/flutter#178886) 2026-02-19 [email protected] Manual roll Skia from dfe78d132e24 to 7bbdc51ab0aa (8 revisions) (flutter/flutter#182612) 2026-02-19 [email protected] Refactor autofill_group_test.dart to remove Material dependencies (flutter/flutter#181903) 2026-02-19 [email protected] Roll Packages from 59f905c to 9da22bf (8 revisions) (flutter/flutter#182611) 2026-02-19 [email protected] Roll Fuchsia Linux SDK from Ihau0pUz3u5ajw42u... to KfPgw04T0OEADLJA5... (flutter/flutter#182607) 2026-02-19 [email protected] Marks Mac_arm64_mokey entrypoint_dart_registrant unflaky (flutter/flutter#181648) 2026-02-19 [email protected] Remove material from Modal barrier tests (flutter/flutter#181708) 2026-02-19 [email protected] Remove material from ticker mode test (flutter/flutter#181696) 2026-02-19 [email protected] Remove material imports from Inherited Model, Magnifier, SafeArea, UndoHistory, Navigator and Layers test (flutter/flutter#181709) 2026-02-19 [email protected] docs: fix grammar in animation library documentation (flutter/flutter#182461) 2026-02-19 [email protected] Handle#6537 first grouped tests (flutter/flutter#182077) 2026-02-19 [email protected] Move SelectionArea web test from widgets to material folder (flutter/flutter#181951) 2026-02-19 [email protected] Roll Dart SDK from 44895e617182 to 2642761fca94 (6 revisions) (flutter/flutter#182572) 2026-02-19 [email protected] Update create template to always generate both SwiftPM and CocoaPods support for iOS/macOS plugins (flutter/flutter#181251) 2026-02-18 [email protected] Fix(Material): DateRangePicker ignores DatePickerTheme.dayShape (flutter/flutter#181658) 2026-02-18 [email protected] Fixing ExpansionTile expandedAlignment not Accepts AlignmentGeometry … (flutter/flutter#180814) 2026-02-18 [email protected] Give guided error message when CocoaPod and SwiftPM dependency conflicts (flutter/flutter#182392) 2026-02-18 [email protected] Remove material from interactive_viewer_test.dart (flutter/flutter#181465) 2026-02-18 [email protected] Bring Windows misc coverage out of bringup (flutter/flutter#182332) 2026-02-18 [email protected] Update android symbolication instructions (flutter/flutter#181267) 2026-02-18 [email protected] Unmark stable vulkan platform view tests as bringup (flutter/flutter#182554) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: ...
…81828) Related to flutter#180137 Not equivalent migrations, Before [1]* release/debug `BuildType`'s were modified to clear the set abi values and force set the 3 abi values flutter supports. The proposed code allows overriding of the abi config in defaultConfig without a flag but *requires* `-P disable-abi-filtering=true` to set custom values in either BuildTypes or Flavors. Flavors requiring a flag is consistent behavior with no change. This is because the "newDsl" only allows a union of default and other build types AGP bug tracking this can be found here http://issuetracker.google.com/issues/285353854. [1] if --split-abi flag was not passed and the app did not disable abi filter in their build props `onVariants` is not preferred because [variant](https://android.googlesource.com/platform/tools/base/+/refs/heads/mirror-goog-studio-main/build-system/gradle-api/src/main/java/com/android/build/api/variant/Variant.kt) does not contain the values we wish to set. Specifically any abi/ndk values. The "buildType" on variant is a string which is not the same as `BaseExtension.buildTypes`. `variantBuilder` similarly did not have the values we want to modify. [VariantBuilder Source](https://android.googlesource.com/platform/tools/base/+/refs/heads/mirror-goog-studio-main/build-system/gradle-api/src/main/java/com/android/build/api/variant/VariantBuilder.kt). Command to run a specific jni integration tests from `packages/flutter_tools/` `FLUTTER_ROOT=~/flutter-work/ ../../bin/cache/dart-sdk/bin/dart test test/integration.shard/gradle_jni_packaging_test.dart --plain-name 'abiFilters provided by the user take precedence over the default'` ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Gray Mackall <[email protected]> Co-authored-by: jesswrd <[email protected]>
3 participants
Related to #180137
Not equivalent migrations, Before [1]* release/debug
BuildType's were modified to clear the set abi values and force set the 3 abi values flutter supports. The proposed code allows overriding of the abi config in defaultConfig without a flag but requires-P disable-abi-filtering=trueto set custom values in either BuildTypes or Flavors.Flavors requiring a flag is consistent behavior with no change. This is because the "newDsl" only allows a union of default and other build types AGP bug tracking this can be found here http://issuetracker.google.com/issues/285353854.
[1] if --split-abi flag was not passed and the app did not disable abi filter in their build props
onVariantsis not preferred because variant does not contain the values we wish to set. Specifically any abi/ndk values. The "buildType" on variant is a string which is not the same asBaseExtension.buildTypes.variantBuildersimilarly did not have the values we want to modify. VariantBuilder Source.Command to run a specific jni integration tests from
packages/flutter_tools/FLUTTER_ROOT=~/flutter-work/ ../../bin/cache/dart-sdk/bin/dart test test/integration.shard/gradle_jni_packaging_test.dart --plain-name 'abiFilters provided by the user take precedence over the default'Pre-launch Checklist
///).