Page MenuHomePhabricator
Differential D5900

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

Authored by ashoat on Dec 17 2022, 7:43 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 12, 2:47 AM
Unknown Object (File)
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
View All Files
Subscribers
atul

Details

Reviewers
tomek
bartek
• jon
varun
Commits
rCOMM1cebf0c85915: [native] [6/40] RN 0.70: Update Android NDK to 24 for M1 hosts
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
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat created this revision.Dec 17 2022, 7:43 PM
Herald added a subscriber: atul. · View Herald TranscriptDec 17 2022, 7:43 PM
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!
ashoat added a child revision: D5901: [native] [7/40] RN 0.70: Get rid of Flipper.Dec 17 2022, 7:47 PM
ashoat requested review of this revision.Dec 17 2022, 8:08 PM

Please ignore CI until the end of the stack

ashoat updated this revision to Diff 19517.Dec 18 2022, 8:26 PM

Rebase

Harbormaster failed remote builds in B14278: Diff 19517!Dec 18 2022, 8:41 PM
ashoat updated this revision to Diff 19579.Dec 19 2022, 8:19 AM

Rebase

Harbormaster failed remote builds in B14330: Diff 19579!Dec 19 2022, 8:22 AM
tomek accepted this revision.Dec 19 2022, 8:34 AM
tomek added inline comments.
native/android/app/CMakeLists.txt
78

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

atul added a comment.Dec 19 2022, 8:50 AM

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.

ashoat added a comment.Dec 19 2022, 9:41 AM
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

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

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

ashoat updated this revision to Diff 19653.EditedDec 19 2022, 9:51 AM

Lowercase set

Harbormaster failed remote builds in B14402: Diff 19653!Dec 19 2022, 9:54 AM
varun accepted this revision.Dec 20 2022, 8:45 AM
• jon accepted this revision.Dec 20 2022, 9:48 AM
• jon added inline comments.
native/android/build.gradle
10

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
ashoat added inline comments.Dec 20 2022, 9:51 AM
native/android/build.gradle
10

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.

• jon added inline comments.Dec 20 2022, 10:02 AM
native/android/build.gradle
10

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.

ashoat updated this revision to Diff 19805.Dec 20 2022, 10:59 AM

Rebase

Harbormaster failed remote builds in B14545: Diff 19805!Dec 20 2022, 11:05 AM
This revision was landed with ongoing or failed builds.Dec 20 2022, 12:00 PM
Closed by commit rCOMM1cebf0c85915: [native] [6/40] RN 0.70: Update Android NDK to 24 for M1 hosts (authored by ashoat). · Explain Why
This revision was automatically updated to reflect the committed changes.
ashoat added a commit: rCOMM1cebf0c85915: [native] [6/40] RN 0.70: Update Android NDK to 24 for M1 hosts.
michal mentioned this in D11041: [backup-client] Compile to wasm.Feb 13 2024, 6:44 AM

Revision Contents
Changeset List

PathSize
native/
android/
app/
11 lines
1 line
17 lines

Diff 19579

View Options

native/android/app/CMakeLists.txt

Loading...
View Options

native/android/app/build.gradle

Loading...
View Options

native/android/build.gradle

Loading...