Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitizer: Set useLegacyPackaging for asan #970

Merged

Conversation

robotchaoX
Copy link
Contributor

@robotchaoX robotchaoX commented Dec 14, 2023

[Why]
Property useLegacyPackaging determin whether to use the legacy convention of compressing all .so files in the APK. If null, .so files will be uncompressed and page-aligned when minSdk >= 23. Otherwise, it failed with "INSTALL_FAILED_INVALID_APK: Failed to extract native libraries, res=-2" when minSdk >= 23.
And wrap.sh is only available for API level 27 and above.

[How]
Set useLegacyPackaging to true.
Set compileSdk to 27.

[Reference]
https://developer.android.com/ndk/guides/asan

@robotchaoX robotchaoX force-pushed the dev-Sanitizers-Set-useLegacyPackaging branch from 959797b to 3ced853 Compare December 18, 2023 14:36
@robotchaoX robotchaoX changed the title Sanitizer: Set useLegacyPackaging to true Sanitizer: Set useLegacyPackaging for asan Dec 18, 2023
@robotchaoX robotchaoX force-pushed the dev-Sanitizers-Set-useLegacyPackaging branch from 3ced853 to 876ed5c Compare December 18, 2023 14:43
sanitizers/app/build.gradle Outdated Show resolved Hide resolved
@robotchaoX robotchaoX requested a review from fmayer January 5, 2024 14:16
@fmayer
Copy link
Contributor

fmayer commented Jan 5, 2024

@DanAlbert @enh-google PTAL

Copy link
Contributor

@enh-google enh-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this one i think i do understand, and it does match what i believe to be true. but you might still want to wait for danalbert next week if you want more than a "seems legit?" from someone who doesn't really know what they're talking about :-) )

@DanAlbert DanAlbert force-pushed the dev-Sanitizers-Set-useLegacyPackaging branch from 55f6d50 to b355695 Compare January 9, 2024 22:54
@DanAlbert DanAlbert enabled auto-merge (squash) January 9, 2024 22:55
@DanAlbert
Copy link
Member

I added explanatory comments but otherwise LGTM.

@DanAlbert
Copy link
Member

... but our CI's broken. I'll go look into that.

@DanAlbert
Copy link
Member

GitHub is currently in the middle of rolling out changes to the GHA images that alter how the Android tools are installed: actions/runner-images#8952. That bug says that sdkmanager is supposed to still be there, but maybe that isn't happening. I see Sentry updated their CI to use https://github.com/android-actions/setup-android, which I think tells us the problem:

Adding platform-tools (contains adb) and cmdline-tools/11.0/bin (contains sdkmanager) to $PATH.

I guess sdkmanager moved to a non-predictable location? Awesome. I'll send a PR to switch us over to setup-android.

@DanAlbert
Copy link
Member

#975 appears to fix it. Once that merges I'll rebase this PR so the checks can re-run, then it should be good to merge.

robotchaoX and others added 3 commits January 9, 2024 16:06
[Why]
Property useLegacyPackaging determin whether to use the legacy convention of compressing all .so files in the APK.
If null, .so files will be uncompressed and page-aligned when minSdk >= 23.
Otherwise, it failed with "INSTALL_FAILED_INVALID_APK: Failed to extract native libraries, res=-2" when minSdk >= 23.
And wrap.sh is only available for API level 27 and above.

[How]
Set useLegacyPackaging to true.
Set compileSdk to 27.
Reference: https://developer.android.com/ndk/guides/asan
@DanAlbert DanAlbert force-pushed the dev-Sanitizers-Set-useLegacyPackaging branch from b355695 to 7316939 Compare January 10, 2024 00:06
@DanAlbert DanAlbert merged commit be6b6b0 into android:main Jan 10, 2024
2 checks passed
@robotchaoX robotchaoX deleted the dev-Sanitizers-Set-useLegacyPackaging branch January 10, 2024 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants