Page MenuHomePhabricator

[native] [5/40] RN 0.70: Update Folly on Android
ClosedPublic

Authored by ashoat on Dec 17 2022, 7:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 6:38 AM
Unknown Object (File)
Fri, Nov 22, 5:48 AM
Unknown Object (File)
Fri, Nov 22, 12:44 AM
Unknown Object (File)
Thu, Nov 14, 10:01 AM
Unknown Object (File)
Thu, Nov 14, 9:51 AM
Unknown Object (File)
Wed, Nov 13, 11:06 PM
Unknown Object (File)
Sun, Nov 10, 5:59 PM
Unknown Object (File)
Sun, Nov 10, 5:55 PM
Subscribers

Details

Summary

Inspired by expo-modules-core's approach – see details here. Not only does this allow us to update Folly, but it also lets us use C++17 finally!! Also critical for react-native-codegen update, and potentially unblocks NDK update (so we can use one that is M1-optimized).

Depends on D5898

Test Plan

Tested along with whole stack: test plan

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

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

Adding CMake people. Note that the Gradle stuff is heavily inspired by Expo stuff (see diff description)

native/android/app/CMakeLists.txt
5 ↗(On Diff #19462)

React Native 0.70 supports C++17

79–104 ↗(On Diff #19462)

This used to be lower, but I moved it up. I think it makes more sense up at the top: it's defining the library we're compiling

130–132 ↗(On Diff #19462)

We no longer need to compile double conversion, we just need the headers

162–167 ↗(On Diff #19462)

We link against the Folly we extract from the pre-packaged React Native AAR, instead of compiling Folly ourselves

169 ↗(On Diff #19462)

Looking into CMake docs indicates target_compile_options is a better, more modern option than add_definitions

178 ↗(On Diff #19462)

Still doing add_definitions here because I didn't have time to test these other libraries and how the change might affect them

Please ignore CI until the end of the stack

native/android/app/CMakeLists.txt
5 ↗(On Diff #19578)

React Native 0.70 supports C++17

79–104 ↗(On Diff #19578)

This used to be lower, but I moved it up. I think it makes more sense up at the top: it's defining the library we're compiling

130–132 ↗(On Diff #19578)

We no longer need to compile double conversion, we just need the headers

162–167 ↗(On Diff #19578)

We link against the Folly we extract from the pre-packaged React Native AAR, instead of compiling Folly ourselves

169 ↗(On Diff #19578)

Looking into CMake docs indicates target_compile_options is a better, more modern option than add_definitions

178 ↗(On Diff #19578)

Still doing add_definitions here because I didn't have time to test these other libraries and how the change might affect them

max added inline comments.
native/android/app/CMakeLists.txt
5 ↗(On Diff #19578)

Exciting 🎉

133–135 ↗(On Diff #19578)

Do we need quotes here? We don't have them at lines 118, 121, etc. maybe we should not use them at all or use them anywhere.

(so we can use one that is M1-optimized)

Is this M1-specific or does it apply to Apple Silicon broadly?

Is this M1-specific or does it apply to Apple Silicon broadly?

As far as I understand, there's broadly no such thing as M1-specific. Whenever you read "M1" you can probably assume it's a legacy stand-in for "Apple Silicon". I'll try to use the proper term in the future

native/android/app/CMakeLists.txt
133–135 ↗(On Diff #19578)

I'm not sure... CMake is very complicated. I referenced what Expo is doing here, and they have the quotes. I'd rather not blow up the scope here to involve adding quotes everywhere, as I'm not sure what other thing that might affect, or whether I'd have to update some syntax somewhere or something

jon added inline comments.
native/android/app/CMakeLists.txt
92–96 ↗(On Diff #19578)

Should probably do another library for this, but I can do that as part of the GLOB RECURSE work.

Created https://linear.app/comm/issue/ENG-2502 in the mean time.

133–135 ↗(On Diff #19578)

Unless you need to perserve whitespace, it shouldn't matter much. From my experience, CMake seems to stringify most things by default.

This revision is now accepted and ready to land.Dec 20 2022, 9:44 AM
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.