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

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

Merged
merged 3 commits into from
Aug 6, 2024

Conversation

supervacuus
Copy link
Collaborator

@supervacuus supervacuus commented Aug 5, 2024

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:

  • This should be tested with the NDK sample app by at least another Android developer in the team
  • Manual check if there are any remaining hardcoded 4096 page-size assumptions
  • How (and when) to integrate this in sentry-android since it will also have to be added there for version < 8.

Copy link

github-actions bot commented Aug 5, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 6ba71e0

@markushi
Copy link
Member

markushi commented Aug 6, 2024

@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: Add support for 16KB page sizes (Android 15) sentry-java#3620 I did some manual testing here as well, looks promising.

@supervacuus
Copy link
Collaborator Author

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.

Copy link

codecov bot commented Aug 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.77%. Comparing base (131a1ee) to head (6ba71e0).

Additional details and impacted files
@@            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     

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

LGTM!

@supervacuus
Copy link
Collaborator Author

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.

@supervacuus supervacuus merged commit 1cf8a33 into master Aug 6, 2024
27 checks passed
@supervacuus supervacuus deleted the feat/support_16kb_page_size branch August 6, 2024 11:30
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.

[Android] Support dynamic page size for Android 15+ devices
2 participants