[Android] Fix abiFilters invalid to Flutter Gradle Plugin #174008
Conversation
(cherry picked from commit b8daa09)
|
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. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where the Flutter Gradle Plugin would overwrite user-defined abiFilters. The change correctly uses project.afterEvaluate to ensure the user's configuration is read first, and only applies default filters if none are provided. This is a solid fix. I've added one minor suggestion to remove a redundant line of code for better clarity and to adhere to the style guide's emphasis on readability.
There was a problem hiding this comment.
Thank you for this fix. To make sure we do not regress we will want an integration test. If you are willing to author that I would be appreciative but I do not want to block this pr.
@gmackall review this assuming we will cherry pick into 3.35
There was a problem hiding this comment.
@angcyo you will need to sign the CLA before this PR can proceed.
This will need to pass tests, as well as have its own test, before it can land. Also, can I ask why in the original report you are adding to the set instead of defining/overriding the set as done in the documentation?
https://developer.android.com/reference/tools/gradle-api/7.1/com/android/build/api/dsl/Ndk#abiFilters:kotlin.collections.MutableSet
I believe if you instead set the abiFilters property to your desired set, this should work right?
If setting (instead of adding) as described above does not work then I think a change like this is warranted (but probably only in that case).
|
Note from Android triage: See last comment in #174004. |
Fix Issues #174004
Pre-launch Checklist
///).