Skip to content

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

Closed
wants to merge 3 commits into from

Conversation

bghgary
Copy link
Contributor

@bghgary bghgary commented Sep 9, 2022

Describe the change

  • Bump versions (minSdkVersion 23 and ndkVersion 23.2.8568313)
  • Use libc++_shared for ANDROID_STL
  • Make Playground release builds debuggable

Documentation

TODO: maybe should add/update requirements

Testing

TODO

- 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",
Copy link

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where do you see this guidance? I see the opposite guidance here and here.

Copy link

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.

Copy link
Contributor Author

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.

Copy link

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.

@github-actions
Copy link

github-actions bot commented Sep 9, 2022

Here are the build results
Assembled
Assembled-Windows0.65
Assembled-Windows0.69
Artifacts will only be retained for 90 days.

@ryantrem ryantrem requested a review from CoPrez September 9, 2022 21:52
@bghgary
Copy link
Contributor Author

bghgary commented Sep 10, 2022

Closing in favor of a smaller change #458.

@bghgary bghgary closed this Sep 10, 2022
@bghgary bghgary deleted the bump-api-version branch December 7, 2022 23:57
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.

2 participants