[Android] Make sure jni libs are considered input to the merge task.#187688
[Android] Make sure jni libs are considered input to the merge task.#187688mboetger wants to merge 17 commits into
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. |
There was a problem hiding this comment.
Code Review
This pull request configures the merge JNI libraries task in FlutterPlugin.kt to include jniLibsDir as an input directory. Feedback suggests marking this input directory as optional using .optional() to prevent build failures in projects that do not contain JNI libraries, where the directory may not exist.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
reidbaker
left a comment
There was a problem hiding this comment.
Non blocking but if we want to not regress here we need a test.
|
autosubmit label was removed for flutter/flutter/187688, because - The status or check suite Windows module_custom_host_app_name_test has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/187688, because - The status or check suite Linux module_host_with_custom_build_test has failed. Please fix the issues identified (or deflake) before re-applying this label.
|
|
@reidbaker & @gmackall PTAL. I verified using 'files' also fixes the issue - and fixes the test failures in CI. |
What did you decide to do about this. Follow up PR? Too difficult to maintain? |
Added a test |
| // Verify APK contains probe-BBBB and NOT probe-AAAA. | ||
| apkBytes = apkFile.readAsBytesSync(); | ||
| archive = ZipDecoder().decodeBytes(apkBytes); | ||
| libappFile = |
There was a problem hiding this comment.
Why here and above do you accept any of the 3 arch .so files?
Can we instead build an arch or build universal and know that our chosen arch will be there?
If for some reason we can't then please extract this logic into a helper method.
There was a problem hiding this comment.
They are only broken in the sense that if you try to reduce the abi support and dont pass the flag then you will not get reduced abi support. That is because the agp 9 apis only allow appending.
gmackall
left a comment
There was a problem hiding this comment.
We currently enforce no trailing commas I believe
this was to support older kotlin versions. We can probably remove this (would check first!) but I expect the commas here will make ci fail. Interested to see if not though I suppose
Yeah, I think my KTlint version is out of whack. |
|
@gmackall is taking this over closing the RP out. |
Ensures jniLibs are considered inputs
Fixes: #187553
Pre-launch Checklist
///).