Page MenuHomePhabricator

[native] [6/40] RN 0.70: Update Android NDK to 24 for M1 hosts
ClosedPublic

Authored by ashoat on Dec 17 2022, 7:43 PM.
Tags
None
Referenced Files
F3228622: D5900.id19463.diff
Tue, Nov 12, 2:47 AM
F3228345: D5900.id19463.diff
Tue, Nov 12, 1:40 AM
Unknown Object (File)
Sun, Nov 10, 5:54 PM
Unknown Object (File)
Sun, Nov 10, 12:31 AM
Unknown Object (File)
Thu, Nov 7, 2:52 PM
Unknown Object (File)
Sun, Nov 3, 5:25 AM
Unknown Object (File)
Sat, Nov 2, 12:28 AM
Unknown Object (File)
Sat, Nov 2, 12:28 AM
Subscribers

Details

Summary

The React Native update had this change, but I initially had to disable the newer NDK in D5898 because of issues with compiling our old version of Folly. After D5899 updated us to a recent Folly, I tried NDK 24 again and encountered two other bugs.

The first was a glog build error. I found that the file that was failing to be compiled could be avoided based on a condition here, which seemed to be connected with a CMake flag.

The second was a Cargo build failure from an #include <atomic> line in the produced cxx.cc file. This GitHub comment pointed me at the solution, which was to use a different Cargo target.

I'm not really sure how to test the armeabi-v7a target beyond confirming that the build succeeds, but I should make sure to test the arm64-v8a target on the device at the office. The armeabi-v7a target is the only one where the Cargo target change matters, unfortunately...

Depends on D5899

Test Plan

Tested along with whole stack: test plan

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 17 2022, 7:44 PM
Harbormaster failed remote builds in B14229: Diff 19463!

Please ignore CI until the end of the stack

tomek added inline comments.
native/android/app/CMakeLists.txt
78 ↗(On Diff #19579)

Can we make this consistent and use lowercase set, or is it a different operation and has to be distinct?

Update Android NDK to 24 for M1 hosts

People were very upset and frustrated when "M1" was used in a dev environment docs diff instead of "Apple silicon." We should try to be more precise/consistent throughout.

In D5900#177708, @atul wrote:

Update Android NDK to 24 for M1 hosts

People were very upset and frustrated when "M1" was used in a dev environment docs diff instead of "Apple silicon." We should try to be more precise/consistent throughout.

Docs have way higher standards than diffs, but I'll try to avoid "M1" in the future

native/android/app/CMakeLists.txt
78 ↗(On Diff #19579)

Looks like CMake command names are case-insensitive: https://cmake.org/cmake/help/v3.0/manual/cmake-language.7.html#command-invocations

I'll update this

native/android/build.gradle
10 ↗(On Diff #19579)

This is copy-pasted from React Native docs so I'd prefer not to change it so diffs still work correctly in react-native-upgrade-helper

jon added inline comments.
native/android/build.gradle
10 ↗(On Diff #19579)

Do you mind creating a follow up task to just bump the version to ndk24 for all platforms?

I'm not really sure of the implications of doing so, but I would like to avoid combinatorial complexity within our builds.

This revision is now accepted and ready to land.Dec 20 2022, 9:48 AM
native/android/build.gradle
10 ↗(On Diff #19579)

I thought about this, but this code is copy-pasted straight from the React Native template for React Native 0.70. Strangely enough, the template is later updated in React Native 0.71 (currently prerelease) to unify all archs, but on NDK 23 instead of 24.

I'd rather avoid straying from the template, but based on this we should see these two unified in the next React Native upgrade.

native/android/build.gradle
10 ↗(On Diff #19579)

Interesting, well, it doesn't seem super high priority.

I guess just following react native would be best as that ecosystem is going to be the most opinionated about environment.

This revision was landed with ongoing or failed builds.Dec 20 2022, 12:00 PM
This revision was automatically updated to reflect the committed changes.