Fix native assets failing to build with AGP 8.4 and multiple flavors #155039
Conversation
The nativeAssetDir is not flavor specific and it should only be added to the jniLibs.scrDir once.
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). 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. 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. |
|
What would be a reasonable way to test this? |
Maybe take an existing test* that builds native assets and unzip the apk and verify that one folder is present.
The other option would be to write a groovy unit test. @gmackall who might be able to point out where one of those is already defined. |
I actually don't think we have any unit tests of groovy code, at least that I'm aware of |
There was a problem hiding this comment.
🙏 Thanks for the report and the fix @knopp!
Maybe the integration test can be modified to try another AGP version:
- test/integration.shard/isolated/native_assets_test.dart
Also, we have some AGP version testing in
- test/integration.shard/android_plugin_example_app_build_test.dart
But I don't believe that that covers package_ffi yet. Maybe you could grok from that and a test/integration.shard/isolated/native_assets_agp_version_test.dart that combines the concepts of the two tests.
This is the example I was thinking of https://github.com/flutter/flutter/pull/125998/files#diff-5ecbf5765f9a06d24990a5b5c3cbcfd590b92e2e88f1463f8c4feaf74bea916a That said building with agp 8.4 also works for me. |
|
@reidbaker @gmackall is there any update on this? |
I believe it still needs either a test or a test exemption. I don't fully understand what conditions are needed to reproduce the issue that the PR fixes, is it specific to AGP 8.4? Could you build off of a test in |
|
Just added a test for this that works for me locally. Looks like I'll need to fix some formatting and figure out why it's failing on CI! |
packages/flutter_tools/test/integration.shard/isolated/native_assets_agp_version_test.dart
Show resolved
Hide resolved
…hub.com:knopp/flutter into android_fix_native_assets_with_multiple_flavors
| ]; | ||
|
|
||
| void main() { | ||
| if (!platform.isMacOS && !platform.isLinux && !platform.isWindows) { |
There was a problem hiding this comment.
@camsim99 @andrewkolos is this the right way to exclude platforms? I seem to recall a parameter that can be set on pure dart tests but am not sure if we have a documented way in integration tests?
There was a problem hiding this comment.
This is what was used in the other native assets integration test. Will defer to @dcharkes and @andrewkolos.
There was a problem hiding this comment.
That is good enough for me. If we hear back from the tooling team then we can file a bug and fix it later.
The nativeAssetDir is not flavor specific and it should only be added to the jniLibs.scrDir once.
Fixes #155038
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.