[ANDROID] [ENHANCEMENT] [build] - Use clang to build Android libraries

[Kudo]

Fixes #20342 and support for future NDK r18

TODO:

- [x] Wait folly merged with this fix

• Wait android-jsc with clang built. See [META] Upgrading Folly #20302 (comment) for details.
  Workaround to test without clang android-jsc: put libgnustl_shared.so directly into apk, i.e. will have both libstd++ (for JSC) and libc++ (for libreactnativejni.so) runtime.
  Or maybe try to use community maintained android-jsc which is built by clang.
  JSC should also built by clang. Wait for Kudo/android-jsc@0437216 landed.

- [x] Upgrade iOS folly version as well

Test Plan:

Test by RNTester

Release Notes:

[ANDROID] [ENHANCEMENT] [build] - Use clang to build Android libraries

[dulmandakh]

@Kudo it seems that folly is merged your PR, so that you can proceed with upgrading folly 👍

[Kudo]

@dulmandakh Updated to folly 2018.10.22 that included the fix.
Now the only remaining task for Android is about the JSC, not sure if there are any plans to migrate JSC with clang from upstream ?

[dulmandakh]

Cool, did you tried to upgrade folly for iOS?

[Kudo]

@dulmandakh 61f8b05 and cdc70e7 upgrade folly for iOS and checked CocoaPods not broken. Please also take a look. Thank you.

[hey99xx]

not sure if there are any plans to migrate JSC with clang from upstream ?

You should continue with making android-jsc build with libc++, with the assumption that jsc-android-buildscripts will not be ready for a while. See #19536 (comment)

[dulmandakh]

I think that it might be better to stick to gnustl when we still use gcc

[Kudo]

Well, the main purpose of this PR is to use clang build, so here to drop gnustl_shared and gcc.

[Kudo]

@hey99xx I see @gengjiawen's changes from facebookarchive/android-jsc#30 and the Dockerfile at https://github.com/gengjiawen/android-ndk. Thanks for @gengjiawen's awesome work.

I am trying to build JSC with clang but pretty not sure if the old JSC r174650 could build with clang or not.

[Kudo]

Unfortunately, JSC r174650 does not directly support clang.
This patch WebKit/WebKit@d656310 landed after r174650.

[gengjiawen]

If we don't upgrade jsc, will it cause problem ?

[hey99xx]

@gengjiawen If we use android-jsc as is, we'd need both libgnustl_shared.so and libc++_shared.so in the app as mentioned in Kudo's workaround in the PR. Not sure if that causes any problem in runtime or not (*), but certainly a pointless app size increase. So updating existing android-jsc to use libc++_shared.so is the best looking option for me.

@Kudo Nothing in WebKit/WebKit@d656310 or https://bugs.webkit.org/show_bug.cgi?id=146833 tells me r174650 didn't support clang.
What I understand is previous COMPILER(GCC) was true for both GCC and CLANG due to __GNUC__ defined in both, so writing COMPILER(CLANG) || COMPILER(GCC) was pointless. This fixed that and introduced COMPILER(GCC_OR_CLANG) replacing all existent COMPILER(GCC) usages. In fact if you download r174650, you can see standalone COMPILER(CLANG) usages.

Edit:

(*) I see @dulmandakh also mentioned in #20342 (comment)

Compile JSC and RN using Clang against libc++, because gnustl and libc++ ABI are incompatible

[dulmandakh]

@Kudo thank you for your effort to land latest folly and clang migration.

Just now, I fetched your branch and did some quick tests. I can build RN and RNTester app without any problems, but RNTester app is failing on startup. A while ago I managed to compile RN with clang against libc++, and RNTester app was failing to launch. I think that cause is described at #21863 (comment).

Also I learned that small and specific PRs are reviewed/merged sooner than a PR that touches many things. So I would like to ask you to split this PR into smaller ones that only upgrades folly, migrates to clang and libc++, Gradle cleanup and so on.

[Kudo]

@hey99xx Yes, just as you understood, COMPILER(GCC) will be true even for clang.
After digging more, that is not the root cause of the build error, but it definitely need some patches for r174650 to build with clang.
I will try more later.

@dulmandakh You could check the adb log, if there are errors like libgnustl_shared.so not found, that is the problem.
Agree that small sized changes makes review easier. I will try to split the changes into different PR accordingly.

[Kudo]

@dulmandakh Opened PRs for just upgrade folly. #21976 for iOS and #21977 for Android.
I will revise this PR later for only build by clang once the above two PRs are landed.
Please also take looks for the two PRs.

[dulmandakh]

@Kudo thank you for your hard work, folly is now up to date.

[Kudo]

@dulmandakh Thanks for your coordinating.
I will revise the PR to only include clang build support later.
It seems I could build JSC r174650 with clang soon.
Fighting with Travis CI and will update once I have done.

[dulmandakh]

@Kudo do you have experience with CMake? If yes, will it make Android development and build easier, faster?

[Kudo]

@dulmandakh
As far as I know, CMake is just like a makefile generator and the compilers are still clang or gcc. It will not help for build speed.
Please let me share my thoughts for CMake.

• Reuse third_party libraries' CMakeLists.txt and save our life to maintain Android.mk for each. That is pretty much the @fkgozali mentioned in [META] Upgrading Folly #20302 (comment). However, currently the folly's CMakeLists.txt have all the things and it should have a subset for RN. Otherwise, libfolly_json.so binary size will be mass increased.

• More for above, we can remove the dirty patch for glog. CMake could do the configuration from automake.

• For iOS, I am not sure the CMake Xcode generator works or not. It would be great if we could use CMake to generate xcodeproj. But I don't think CMake could generate Podspec for CocoaPods.

In summary, I don't see much benefits in the short term. Migrate to CMake currently is just to rewrite Android.mk with CMakeLists.txt.
If folly could provide modulized CMakeLists.txt dedicated for RN, it will drive me migrate to CMake.

[Kudo]

I've updated this PR to only for clang build support and remove previous folly changes.
Based on @gengjiawen's awesome work, I could now build JSC r174650 by clang.
Here is the commit: Kudo/android-jsc@0437216

With some enhancements from original work:

• Build by clang
• NDK r17c
• Keep app_platform (minSdkVersion) at API 16 for 32bits, follows
  original JSC & RN's spec. 64bits will be API 21.
• Repack AAR with POM by gradle and publish into npm. Works referenced from
  https://github.com/react-community/jsc-android-buildscripts.
  This supports to use local maven repository directly from node_modules.
• Docker build based on pure openjdk:8-jdk image. The docker builder
  referenced from https://github.com/gengjiawen/android-ndk.
• All build steps are reproducible by Travis CI and the npm deployment are also done in Travis CI.

CI build log: https://travis-ci.org/Kudo/android-jsc/builds/448793745
npm package: https://www.npmjs.com/package/@kudo-ci/android-jsc

Migration Guide:

1. yarn add @kudo-ci/android-jsc
2. Use @kudo-ci/android-jsc local maven repository before jcenter()

diff --git a/ReactAndroid/build.gradle b/ReactAndroid/build.gradle
index a419728d77..cec9b33a7b 100644
--- a/ReactAndroid/build.gradle
+++ b/ReactAndroid/build.gradle
@@ -324,7 +314,7 @@ dependencies {
     api "com.squareup.okhttp3:okhttp:${OKHTTP_VERSION}"
     api "com.squareup.okhttp3:okhttp-urlconnection:${OKHTTP_VERSION}"
     api 'com.squareup.okio:okio:1.14.0'
-    compile project(':android-jsc')
+    compile 'org.webkit:android-jsc:r174650'
diff --git a/build.gradle b/build.gradle
index 08845e85fc..5339ff8981 100644
--- a/build.gradle
+++ b/build.gradle
@@ -20,10 +20,13 @@ buildscript {

 allprojects {
     repositories {
+        maven {
+            url "$rootDir/node_modules/@kudo-ci/android-jsc/dist"
+        }
+
         google()
         jcenter()
         mavenLocal()

         def androidSdk = System.getenv("ANDROID_SDK")
         maven {
             url "$androidSdk/extras/m2repository/"

[hey99xx]

WOW this is absolutely fantastic, truly well done.

I don't know if anyone will land Kudo/android-jsc@0437216 in main facebook/android-jsc repo. You're building on top of facebookarchive/android-jsc#30 but they're not taking that PR either, that's why @gengjiawen had checked in the pre-built AAR in #18754.

So in this case it may make sense for you to do the same thing. The only thing is file hashes for all pre-built libjsc.so files would be changing this time, and I don't know if Facebook would trust on .so files built on someone else's computer and put them in their OSS builds.

[hey99xx]

Also quick question: The new JSC in NPM ships with libjsc and libc++_shared (.so files). If I want to use that project, I understand I'd need to use their upgraded libjsc. But will their libc++_shared conflict with the one that will be used by RN after this PR, or will it be the same or compatible file?

[dulmandakh]

personally I cannot use it because it's minSDK is 21, but I want support for Android 4.x

[Kudo]

@hey99xx
My proposal is not going to land my JSC changes into the react-native repo but instead into somewhere like https://github.com/react-community/jsc-android-buildscripts directly. We could apply a community Travis CI, docker hub registry, and npm package.
Just to add a npm dependency to include JSC.
The proposal also follows the RN repo slimming plan.
And I don't like the private binary distribution either, so the build steps & npm deployment are all happened in Travis CI transparently.

Nice finding for your libc++_shared.so. Yes, after this PR, there should have a libc++_shared.so conflict if going to apply The new JSC in NPM .
Both original JSC builder and my current work remove C++ runtime during AAR packaging.

@dulmandakh
Where did you see that the minSDK is 21?
32bits platform minSDK is 16, and 64bits platform minSDK is 21 from my design.
Android provides 64bits support after API 21.

[dulmandakh]

@Kudo it's about android-jsc package on NPM, which requires to set minSdkVersion to 21.

[Kudo]

@dulmandakh Oops, I see and sorry for my misunderstood.

[dulmandakh]

@Kudo is it possible to have CMake configuration for Folly in RN repo, so we don't have wait for folly developers if we need change?

[fkgozali]

However, currently the folly's CMakeLists.txt have all the things and it should have a subset for RN

Yeah, agreed. This is probably something we can ask the folly repo for? E.g. have a subset specifically for mobile?

[dulmandakh]

I think it might be better to maintain CMake configuration for Folly in RN repo if possible, because we don't have to wait for a new folly release if we need some change. Maintaining configuration in folly repo might be normal for FB RN, but sometimes it breaks open source CI due to fact that some changes might not be released to open source.

FYI, case with Buck b40e23d.

[fkgozali]

I think it might be better to maintain CMake configuration for Folly in RN repo if possible, because we don't have to wait for a new folly release if we need some change

This is a fair point, and I'm okay with having RN-specific config (that can be shared across all platforms as needed - ios/anroid/others)

[Kudo]

Well, I could try to investigate the possibility to CMake migration for both iOS/Android later.
Let's focus at the clang here. Any thoughts for the JSC with clang?

[dulmandakh]

@Kudo I personally prefer more incremental approach to a technical solution. I was thinking if it's possible to build current JSC with clang, then update JSC version.

[Kudo]

@dulmandakh Yes, my commit is current JSC r174650 build with clang.

[akshetpandey]

It seems it would be easier to have a fb employee bundle a prebuilt jsc as is done on master right now...

[akshetpandey]

#22231
I don't see how the other PR addresses android-jsc's dependency on gnustl either :(

[hey99xx]

@akshetpandey This PR plans to handle android-jsc's dependency on gnustl. See the checklist at the top:

JSC should also built by clang. Wait for Kudo/android-jsc@0437216 landed.

The other PR seems to have the same goal as this, but doesn't address android-jsc's dependency on gnustl at all, I don't know if that's correct thing to do. @Kudo @gengjiawen what do you think?

[akshetpandey]

Doesn't look like the jsc PR is going to land. I am using this PR with a locally compiled android-jsc in my fork.

[Kudo]

Close this in favor of #22263