-
Notifications
You must be signed in to change notification settings - Fork 63
Bump Android SDK versions + compatibility fixes #457
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
Conversation
- Bump versions (minSdkVersion 23 and ndkVersion 23.2.8568313) - Use libc++_shared for ANDROID_STL - Make Playground release builds debuggable
@@ -62,7 +62,7 @@ android { | |||
externalNativeBuild { | |||
cmake { | |||
abiFilters 'arm64-v8a', 'armeabi-v7a', 'x86', 'x86_64' | |||
arguments '-DANDROID_STL=c++_static', | |||
arguments "-DANDROID_STL=c++_shared", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using shared instead of static may lead to issues if consumers of the package are on a different ndk version. The general guidance is static unless you can control the version across all native libs. Might be worth keeping this static until there is a way to pick and choose which prebuilt .so to take since I think you ship a prebuilt .so in the package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last note here: https://developer.android.com/ndk/guides/cpp-support#one_stl_per_app calls out the challenges of ensuring compatibility of the STL across NDK version changes. The way that I think about it (and I think how the NDK docs imply a little though they are written from the perspective of an end app developer not an sdk / library author) is that it really depends on what else your native code is doing.
If your native code is always going back up through JNI / non native code you can build a self contained set of libs that work across a wider cross section of NDK and app versions (because its all baked into the .so).
This approach though suffers from the fact that it bloats the app to include the STL a bunch of time. If the native code is a bunch .so's that the end app developer can control the NDK version of (for instance if that developer is using cmake and pulling in a lot of dependencies that its going to rebuild from source) then it can save on overall footprint by sharing the runtime.
Unfortunately since BRN is an SDK component and doesn't have a lot of say on the final app being built, it seems more perilous to add a dependency to the shared STL since there can only be one and the package itself can't dictate which one (sure would be swell if gradle/maven/npm could know and complain about mismatches though). As I understand it, BRN is not using the STL across any boundaries (thank goodness) so its really just about trying to make the trade off on compatibility (i.e. what happens when building an app using ndk 25 for the shared runtime) and size and I'm leaning towards the latter for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for completeness, I also found this page: https://developer.android.com/ndk/guides/middleware-vendors.
Perhaps we can offer both versions, but I'm not sure it is a good idea.
Unfortunately since BRN is an SDK component and doesn't have a lot of say on the final app being built, it seems more perilous to add a dependency to the shared STL since there can only be one and the package itself can't dictate which one.
I agree with this, but we can make the same argument for the NDK version, except there is no way to "statically link" the NDK. I believe the C++ runtime version is locked for a particular NDK version?
I will discuss with everyone next week and we will figure out what is best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. And yeah the other option would be to build the .so for multiple NDK versions and have a mechanism to let consumers pick which one gets bundled into the final app.
Honestly either approach right now is probably fine and its more of a concern when newer NDKs and STLs come out that may be incompatible. Not trying to block the PR or anything just want to make sure we are considering our options and what will give the best chance for success.
Here are the build results |
Closing in favor of a smaller change #458. |
Describe the change
Documentation
TODO: maybe should add/update requirements
Testing
TODO