getsentry / sentry-native

Sentry SDK for C, C++ and native applications.
MIT License
404 stars 170 forks source link

feat: support 16kb page size on Android 15 #1028

Closed supervacuus closed 3 months ago

supervacuus commented 3 months ago

This fixes #989.

I tried to reach the most minimal version I could successfully deploy to a 16kb page-size image on the emulator.

Important here:

Open topics:

github-actions[bot] commented 3 months ago
Fails
:no_entry_sign: Please consider adding a changelog entry for the next release.
Messages
:book: Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- support 16kb page size on Android 15 ([#1028](https://github.com/getsentry/sentry-native/pull/1028))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by :no_entry_sign: dangerJS against 6ba71e0f238e4be01b5172000c09f8b1207b2f11

markushi commented 3 months ago

@supervacuus thanks for taking a look into this!

  • This should be tested with the NDK sample app by at least another Android developer in the team
  • How (and when) to integrate this in sentry-android since it will also have to be added there for version < 8.

1) I manually tested the changes on an "Android 15 16KB page size" emulator, looking good, no more obvious crashes 2) I manually tested the changes on non 16KB devices, just to ensure the build flags don't cause a regression, looking good as well! 3) The required changes for sentry-java 7.x are here: https://github.com/getsentry/sentry-java/pull/3620 I did some manual testing here as well, looks promising.

supervacuus commented 3 months ago

I have also tested with a separate app, only initializing sentry-native on a 16kb image but using the local maven AAR instead of the sample APK.

I think it should be enough to specify the NDK version in the sample build script because our packaged .so does not have 16kb-built runtime dependencies.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.77%. Comparing base (131a1ee) to head (6ba71e0). Report is 22 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1028 +/- ## ========================================== + Coverage 83.75% 83.77% +0.01% ========================================== Files 53 53 Lines 5510 5510 Branches 1197 1197 ========================================== + Hits 4615 4616 +1 Misses 783 783 + Partials 112 111 -1 ```
supervacuus commented 3 months ago

Manual check if there are any remaining hardcoded 4096 page-size assumptions

Context: https://developer.android.com/guide/practices/page-sizes#check-code

So, from my side, we're good if everything works on the sentry-android side.