Build 64bit binaries for Android by marafat · Pull Request #19536 · react/react-native

marafat · 2018-06-01T21:23:39Z

Motivation

Add 64bit support to Android. Fixes#2814

How is this different from the previous effort done in #17640 and #18754

This PR uses the JSC from the SoftwareMansion's fork mentioned under Support third-party 64-bit libraries on Android #2814
It builds with NDK 16/clang
it updates the APP_STL to c++_shared

Test Plan

Make the RNTester app build a separate APK per abi
assemble debug and release APK

$ cd react-native
react-native$ rm -rf ./node_modules && yarn install
react-native$ ./gradlew :ReactAndroid:cleanReactNdkLib :RNTester:android:app:clean :RNTester:android:app:assembleRelease

I found these to be beneficial against some build errors:

$ cd react-native
react-native$ rm -rf ./.buckd/
react-native$ rm -rf ./buck-out/

use adb install to install the arm64-v8a APK on a PIXEL 2XL

$ cd react-native
react-native$ adb install ./RNTester/android/app/build/outputs/apk/app-arm64-v8a-release.apk

the app runs fine in both cases.

Related PRs

[X] ~~Todo: change instructions on building from source.~~react/react-native-website#394

Release Notes

[ANDROID] [ENHANCEMENT] [ReactAndroid] - add x86_64 arm64-v8a support.

The same flag is later overwritten on the following line. It is later appended to LOCAL_CFLAGS

# Conflicts: # ContainerShip/Dockerfile.android-base

hramos · 2018-06-01T22:57:03Z

Sweet. Let's make sure test_android is passing before merging. At this time, the test_android failure is unrelated to your change as master is failing with the same reason (#19309).

* master: Bump metro@0.38.1 Remove duplicated attachWebsocketServer module Switch to ES6 Class Slider to ES6 Class Slider move prop comments to flow types Slider remove $FlowFixMe #take2 Revert D8234803: [RN] Slider remove $FlowFixMe normalizeColor to compute regexps lazily Implement release of FabricUIManager resources Add backward compatible support for onLayout event in Fabric refactor JSI module initialization Slider remove $FlowFixMe better place for dismiss all button Low the priority for logging events in fabric react-native-xcode.sh: Support Homebrew-installed nodenv make logMarker visible for consistency with logTaggedMarker

hey99xx · 2018-06-04T18:37:51Z

@hramos in your other comment in #2814 (comment) you said you prefer to stick with existing JSC that FB uses. So are you ok with this PR integrating with the new JSC from SoftwareMansion? Aren't those conflicting statements, or did something happen to convince FB that new JSC is finally usable/stable?

* master: skip dismiss all if all rows are hidden (react#19564) Trigger nested VirtualizedLists to re-measure if their containing cell's onLayout fires

Minishlink · 2018-06-08T13:58:58Z

@marafat Very nice!
test_android is now passing on master, can you rebase your branch please?

* master: Fix NullPointerException when emiting event using UIManagerModule Fixed concurrency issue in remote debugger Fix ReactImagePropertyTest SoLoader failures (react#19607) Remove unused include. (react#19548) Update Danger token (react#19606) Modified deepFreezeAndThrowOnMutationInDev to use Object.prototype.ha… (react#19598) Change error message on interpolation (react#19571) Bump Prettier to 1.13.4 on xplat Fix/security issues (react#19373) Add open source Flow declaration for Metro module Use correct library reference for libfishhook.a in RCTWebSocket (react#19579) Enable proguard for Fabric in release builds Fix events not working after closing and navigating back to Fabric screen in FB4A

marafat · 2018-06-08T14:55:28Z

@hramos looking at the logs of test_android it seems that we need to increase the storage specified on the container running the emulator. Not sure if you agree, but if you do, can you point me where I should do it and maybe suggest a new value.

Also, not sure how to fix test_ios_e2e but I will look into it.

Thanks!

bestander · 2018-06-14T17:09:38Z

Hey everyone, I am not actively involved with RN recently, working on a product that uses RN now.
I'll point to this PR in our internal group.
Just FYI those things are usually hard to merge, dependencies management is complicated

marafat · 2018-06-14T17:51:01Z

Thank you @bestander for chiming in and for the internal push. I understand your point, and I tried as much as I could to make this PR as slim as possible.

For my personal notes, I made a document that represents the timeline of issues I faced trying to get this done, and the solution I chose for each. If you think this would be helpful in speeding up the review process I will be happy to share.

marafat · 2018-06-14T17:54:33Z

Thanks @hey99xx for the detailed help. I will incorporate asap...

Quoting the message from the commit that changed jscBaseUrl from svn to github:

It would be more in line with other dependencies to depend only on github for thirdparty bridge dependencies.

Although I find the svn link easier to update in the future, I will respect the internal strategy and use the github mirror link.

kmagiera · 2018-06-18T06:22:16Z

Thanks for working on this @marafat – we've been waiting for updated JSC on Android for way too long.

As @kelset mentioned in #19737 we discussed that in some other thread and I suggested we split this into a few intermediate steps. Splitting this work will make it easier to merge as there are going to be a fewer moving parts. Also if it turns out we need to back out some of the parts we can still benefit from others that are ok.

My suggestion was to proceed as follows:

PR that upgrades build tools (NDK, stl lib), we can also upgrade less strategic libs like: double-conv, glog etc
Upgrade JSC version – we are actively working on JSC buildscripts right now and trying different versions. We are not yet convinced that JSC builds are 100% stable as we have seen some random crashes we haven't manage to reproduce or fix. We need some more data points from people who use jsc-android-buildscripts in production if they have any reports on that (please report on the project's github if you have seen any). We expect to move forward with that really soon.
Enable 64bit architecture. For that we also need to figure out strategy of how this should be communicated. This changes files in app template and to make sure people start using it we need to add detailed description to the release notes (this comes as a fragment of PR description). We should verify that this works fine with Android App Bundle feature of Google Play, otherwise even simple RN apps will be very large (like 40MB). We also should add a chapter to documentation explaining why debug builds are huge and that it is not what the users who install the app would get.

gengjiawen · 2018-06-18T08:17:18Z

I think stay in one pr is okay, since the main motivation update current toolchain is getting arm64 support.

gengjiawen · 2018-06-18T10:39:00Z

 cd $NDK_HOME
 echo "Downloading NDK..."
- curl -o ndk.zip https://dl.google.com/android/repository/android-ndk-r10e-linux-x86_64.zip
+ curl -o ndk.zip https://dl.google.com/android/repository/android-ndk-r16-linux-x86_64.zip


Remove android.useDeprecatedNdk=true in gradle.properties

This flag is not needed anymore the following reasons: 1. It is important when the gradle plugin is the one invoking the ndk command. 2. For ReactAndroid, the ndk commnad (ndk-build in this case) is invoked manually. See gradle task: buildReactNdkLib 3. If the gradle plugin is invoking the ndk command, these lines will cause the plugin to invoke the ndkCompile (the deprecated ndk) over ndk-build. This is undesired.

facebook-github-bot · 2018-07-18T17:05:30Z

@marafat I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

bestander · 2018-07-18T17:27:11Z

A quick update, the teams inside FB know about the issue and are working on a solution to resolve it.
I don't have any ETA as the work is complicated but hopefully soon RN will be less coupled to a specific JSC.

patrickkempff · 2018-07-18T19:23:56Z

@bestander thanks for the update. I am very curious, can you share a bit more information about this? 👍

hramos · 2018-08-18T00:54:03Z

Lots of conflicts here, but they're likely due to the progress that has been made to update several of these dependencies in other PRs.

Like Konstantin said, we're working on making it easier to use a different JSC soon.

sospedra · 2018-10-09T08:44:56Z

Hello dear mantainers <3
This is marked for the milestone 0.58
In order to organize ourselves, is this gonna happen finally? 😱 😍

hey99xx · 2018-10-09T21:15:38Z

@sospedra I think they're just taking this one #18754. So 64bit binary support will be there, but no update to the JSC engine (like this PR was doing) and will keep using the current android-jsc.

hramos · 2018-10-09T21:17:54Z

Yes, it will happen, though I do not think it will definitely be in place for 0.58. There's some work we need to do to determine what actually needs to be done to support this, and it is being discussed at #2814. As @hey99xx and @bestander said, we need to do some work related to the JSC used in React Native on Android.

hramos · 2019-01-15T21:29:32Z

The new JSC with 64-bit architectures is on master now. Any interest in resolving the conflicts here and moving the PR forward?

hey99xx · 2019-01-18T10:38:06Z

@hramos Practically everything that this PR was doing is already done by other PRs.

From f3e5cce "Use new JavaScriptCore from npm" commit message:

- Use clang instead of the deprecated gcc
- Use libc++ instead of the deprecated gnustl

64-bit work was done earlier without the JSC upgrade in 0a2825f with a custom 64-bit build of the old android-jsc.aar checked in into the repo.
NDK bump from 10e to 17b was done in 6117a6c.
glog and double-conversion updates were done in b5fca80 and f32032e.

Maybe give @marafat chance to look again if there's still anything needed, but I'd think this PR can be closed without being merged in favor of all the other work completed.

marafat · 2019-01-18T17:27:27Z

Closing - Thanks @hey99xx for providing links to all the pieces. I did go over all of them, and I confirm that they all collectively achieve everything this PR is trying to.

@hramos - for those getting here from google, I wonder if you can share a stable release number that will include 64bit support?

hramos · 2019-01-19T02:01:05Z

The new JSC (the last piece of the puzzle) is scheduled for React Native 0.59.

marafat added 11 commits June 1, 2018 12:57

[CP] Allow RN to build for 64bit architecture
480f7c5

revert updating NDK
1cfb4b6

progress 1
c85b48d

progress 2
35e321a

Progress 3 - Build successfully
fdad0fd

Add support for new architectures when bundling JS assets.
44e8a1c

fix reference to jsc-android-manager
4b9aa17

Remove overwritten flag definition
4787623
The same flag is later overwritten on the following line. It is later appended to LOCAL_CFLAGS

Fix an issue when bundling JS Assets in RNTester
1ca7d67

Add downloadJSC to bash script that downloads deps
67ac60d

remove dupe
e2533e7

marafat requested a review from hramos as a code owner June 1, 2018 21:23

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 1, 2018

update NDK version to use for CI tests
6e03a82

react-native-bot added ✅Test Plan Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Jun 1, 2018

marafat added 2 commits June 1, 2018 18:09

Merge branch 'master' into muhammad/arch-64-fb-submit-master
5d39edd
# Conflicts: # ContainerShip/Dockerfile.android-base

update NDK version to use for CI tests 2
bab84fc

hramos added the Core Team label Jun 1, 2018

This was referenced Jun 4, 2018
Run RN tests post 64bit changes plexinc/fb-react-native#2
Closed
Update Docs - Android build from source instructions react/react-native-website#394
Closed

Merge branch 'master' into muhammad/arch-64-fb-submit-master
b0851f7
* master: skip dismiss all if all rows are hidden (react#19564) Trigger nested VirtualizedLists to re-measure if their containing cell's onLayout fires

Sync jscAPIBaseUrl with webkit SVN rev 224110
71a9215

kelset mentioned this pull request Jun 15, 2018
[META] Upgrading JSC #19737
Closed

gengjiawen mentioned this pull request Jun 16, 2018
add x86_64 arm64-v8a support. Fixes #2814 #18754
Closed

gengjiawen reviewed Jun 18, 2018
View reviewed changes

This comment has been minimized.
Sign in to view

gengjiawen mentioned this pull request Jun 26, 2018
bump Android NDK to r14b #19893
Closed

hramos added this to the 0.58 RC milestone Sep 25, 2018

hramos modified the milestone: 0.58 RCOct 9, 2018

hey99xx mentioned this pull request Oct 23, 2018
[ANDROID] [ENHANCEMENT] [build] - Use clang to build Android libraries #21863
Closed
1 task

aeddi mentioned this pull request Nov 26, 2018
Build: Android doesn't build properly for armeabi-v8a target berty/berty#627
Closed

hramos removed this from the 0.58 RC milestone Jan 15, 2019

marafat closed this Jan 18, 2019

Uh oh!

Conversation

marafat commented Jun 1, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Test Plan

Related PRs

Release Notes

Uh oh!

hramos commented Jun 1, 2018

Uh oh!

hey99xx commented Jun 4, 2018

Uh oh!

Minishlink commented Jun 8, 2018

Uh oh!

marafat commented Jun 8, 2018

Uh oh!

bestander commented Jun 14, 2018

Uh oh!

marafat commented Jun 14, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marafat commented Jun 14, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kmagiera commented Jun 18, 2018

Uh oh!

gengjiawen commented Jun 18, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gengjiawenJun 18, 2018

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

facebook-github-bot commented Jul 18, 2018

Uh oh!

bestander commented Jul 18, 2018

Uh oh!

patrickkempff commented Jul 18, 2018

Uh oh!

hramos commented Aug 18, 2018

Uh oh!

sospedra commented Oct 9, 2018

Uh oh!

hey99xx commented Oct 9, 2018

Uh oh!

hramos commented Oct 9, 2018

Uh oh!

hramos commented Jan 15, 2019

Uh oh!

hey99xx commented Jan 18, 2019

Uh oh!

marafat commented Jan 18, 2019

Uh oh!

hramos commented Jan 19, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

marafat commented Jun 1, 2018•
edited
Loading

marafat commented Jun 14, 2018•
edited
Loading

marafat commented Jun 14, 2018•
edited
Loading

gengjiawen commented Jun 18, 2018•
edited
Loading

hramos commented Jan 19, 2019•
edited
Loading