-
Notifications
You must be signed in to change notification settings - Fork 38
Add x86_64 support #19
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have some questions, it's not clear to me just looking at the code what the answers are.
# version to be 1 so that StdLibExtra.h doesn't try to declare | ||
# is_trivially_destructible | ||
# Except from this single place JSC code doesn't check for 4.8.1 anywhere else | ||
WTF_EXPORTED_PREPROCESSOR_FLAGS.extend([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this OK to remove now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gcc 4.9 is ok, gcc 4.8 need that patch.
ndk_version = r10e | ||
cpu_abis = armv7, arm64, x86, x86_64 | ||
gcc_version = 4.9 | ||
app_platform = android-21 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be 16 to support Android 4.1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried different platforms, it looks like only android-21
can work, but i can figure out the reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK thanks, the JSC AAR file needs to work with API 16 since React Native supports API 16.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably because android-21
was the first platform that contained the x86_64
and arm64
ndk ABI.
@@ -165,6 +152,7 @@ cxx_library( | |||
supported_platforms_regex = SUPPORTED_PLATFORMS, | |||
header_namespace = '', | |||
headers = subdir_glob([ | |||
('WTF', '*.h'), | |||
('WTF/wtf', '*.h'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
encounter an error, missing "config.h"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsc/WTF/wtf/unicode/CollatorDefault.cpp:29:20: fatal error: config.h: No such file or directory
#include "config.h"
^
compilation terminated.
jsc/WTF/wtf/unicode/CollatorDefault.cpp:29:20: fatal error: config.h: No such file or directory
#include "config.h"
^
compilation terminated.
jsc/WTF/wtf/unicode/CollatorDefault.cpp:29:20: fatal error: config.h: No such file or directory
#include "config.h"
^
compilation terminated.
jsc/WTF/wtf/unicode/CollatorDefault.cpp:29:20: fatal error: config.h: No such file or directory
#include "config.h"
^
compilation terminated.
BUILD FAILED: //jsc:wtfcollation#android-x86,compile-pic-CollatorDefault.cpp.oe467649f failed with exit code 1:
c++ piped_preprocess_and_compile
This is the build output, but this PR depends on facebook/buck#921
|
Is this ever going to get cleaned up and landed? |
(buck support for 64bit on Android has been in for months) |
This looks good to me. I believe it's fine to bump the target Android version, which is separate from the minimum Android version, so we'll still maintain compatibility with 4.x. @mkonicek Could you merge this? |
I'm trying to understand why React Native doesn't support arm64, and from reading the discussion on the RN issues post it seems this project is one of the dependencies, so until android-jsc officially supports arm64 support I guess RN won't be able to. What else is blocking this PR from being merged? |
It'd be great to see this PR merged. The build scripts are broken on master. |
If there is any appetite to get this merged, I can raise a PR. The URL to download jsc sources is now broken, but we can still checkout the svn repo and get the same revision. If there are other problems, it'd help us. We are seeing massive drop in performance for some use cases. If we are unable to support 64-bit sooner, it might make react-native a no-go for us. |
@govi Have you tried the JavaScriptCore from this project? https://github.com/SoftwareMansion/jsc-android-buildscripts |
@newyankeecodeshop well, expo seemed to have rolled it back in their last release, due to some crashes in date functions. Prolly irrelevant to this ticket, but what is your experience with the new-jsc? |
@govi We've been using it for several months on a new app running on Android 4.2-5.1 and haven't had any issues. We don't use the date |
@ide anything we can do to push this foward ? |
@gengjiawen The work was happening in this repo: https://github.com/SoftwareMansion/jsc-android-buildscripts. The one thing that comes to mind is testing the new JSC in a lot of conditions (different types of phones, different JS bundles). |
But currently android-jsc still use nightly build webkit before 2016-08-26 and have no test. My main concern is that keep javascripcore up to date will give us better performance and better support to modern js features. And also support more cpu will get react native widely adopted. |
Work in this repo appears to have stopped -- the most recent work on JSC for RN Android was on this repo: https://github.com/SoftwareMansion/jsc-android-buildscripts. That's where work for new versions of JSC (ES2015, 64-bit) is happening. That said, it seems fine to me to merge this PR and I'll talk to someone who works on the FB Open Source team to see if they can help. |
As far as I know, react native is still use this repo (https://github.com/facebook/react-native/blob/999851a3891b4c6ceffd37fa3a6bdb27c5b6159e/ReactAndroid/build.gradle#L291). And thanks for reaching out. |
When is this going to be added? Am I missing something? |
See #5 & facebook/react-native#2814
cc @ide @astreet @kmagiera