Use new JavaScriptCore from npm #22263

DanielZlotin · 2018-11-14T02:00:39Z

Summary:
Pull Request resolved: #22231

Use clang instead of the deprecated gcc
Use libc++ instead of the deprecated gnustl
Updated gradle and android plugin version
Fixed missing arch in local-cli template
clean task should now always succeed
clean task deletes build artifacts
No need to specify buildToolsVersion. It's derived.
Elvis operator for more readable code

Differential Revision: D13004499

pull-bot · 2018-11-14T02:01:58Z

	Warnings
⚠️	🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.
⚠️	📋 Test Plan - This PR appears to be missing a Test Plan.
⚠️	📋 Release Notes - This PR appears to be missing Release Notes.

Generated by 🚫 dangerJS

DanielZlotin · 2018-11-14T20:50:02Z

I published the old jsc-android (the current one used by RN) to npm of the new jsc (jsc-android), under tag stock-react-native (version is 174650.0.0).
new jsc is on latest tag
The goal right now is that the user will be able to choose which version of jsc to use
Easiest way I see right now to solve the gnustl <--> libc++ issue is to bundle gnustl_shared.so inside the package of the old jsc (under the stock-react-native tag). The issue right now is that I need to find a way to compile it, gradle screams at me that it is no longer supported -_-"

Android NDK: APP_STL gnustl_shared is no longer supported. Please switch to either c++_static or c++_shared.

DanielZlotin · 2018-11-14T21:23:32Z

Found it! in ANDROID_NDK/sources/cxx-stl/gnu-libstdc++/4.9/libs/...

ReactAndroid/build.gradle

gengjiawen · 2018-11-15T10:51:59Z

@dulmandakh

kelset · 2018-11-15T15:15:49Z

Can you add the Release notes please? 😇

gusgard · 2018-11-16T18:08:25Z

@DanielZlotin
You should update to gradleVersion = 4.10.2 in local-cli/templates/HelloWorld/android/build.gradle

DanielZlotin · 2018-11-25T12:33:38Z

Using npm is just not the right way to manage jsc as it's a native dependency. After fighting with it for a while I abandoned this approach. The correct approach (which will require some work in https://github.com/react-community/jsc-android-buildscripts) is to manage publications of jsc in maven. This way the user will be able to override / use other versions of jsc.

This PR just updates react-native to use the latest jsc from npm, with full x64 support.

gengjiawen · 2018-11-25T12:45:24Z

I think it can, maven maybe too slow.

gengjiawen · 2018-11-25T12:46:34Z

package.json

@@ -223,6 +223,7 @@
 "flow-bin": "^0.86.0",
 "jest": "24.0.0-alpha.6",
 "jest-junit": "5.2.0",
+ "jsc-android": "latest",


maybe keep it simple, not introduce jsc in this pr.

That's the entire point of this PR

If that's the case, you need to clean the old jsc and lock jsc version.

By clearing old JSC is there anything else except 64-bit libicu_common.so and libjsc.so files checked into this package?

This is the thing I can think of.

gengjiawen · 2018-11-25T12:55:45Z

But maven can make developer use different jsc more easily, it's more flexible.

facebook-github-bot

@axe-fb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Pull Request resolved: facebook#22231 - Use clang instead of the deprecated gcc - Use libc++ instead of the deprecated gnustl - Updated gradle and android plugin version - Fixed missing arch in local-cli template - `clean` task should now always succeed - `clean` task deletes build artifacts - No need to specify buildToolsVersion. It's derived. - Elvis operator for more readable code Pull Request resolved: facebook#22263 Differential Revision: D13004499 fbshipit-source-id: b4eca5d76482539c2c91833801b534e90cf153eb

gengjiawen · 2018-11-25T13:43:58Z

package.json

@@ -223,6 +223,7 @@
 "flow-bin": "^0.86.0",
 "jest": "24.0.0-alpha.6",
 "jest-junit": "5.2.0",
+ "jsc-android": "latest",


If that's the case, you need to clean the old jsc and lock jsc version.

gengjiawen · 2018-11-26T01:11:15Z

build.gradle

@@ -30,8 +30,3 @@ allprojects {
 }
 }
 }
-


Don't delete this, it's used to upgrade gradle.

Overriding wrapper scripts is deprecated and will be removed in future gradle versions

Any doc I can check on this deprecation ?

@gengjiawen looks like there it is (hope) https://docs.gradle.org/4.8/release-notes.html#overwriting-gradle's-built-in-tasks but it says just about syntax, not whole overriding

gengjiawen · 2018-11-26T01:11:44Z

local-cli/templates/HelloWorld/android/build.gradle

@@ -31,9 +30,3 @@ allprojects {
 }
 }
 }
-
-
-task wrapper(type: Wrapper) {


Same as above.

Overriding wrapper scripts is deprecated and will be removed in future gradle versions

hey99xx · 2018-11-27T21:37:23Z

ReactAndroid/build.gradle

@@ -84,17 +85,17 @@ task prepareFolly(dependsOn: dependenciesPath ? [] : [downloadFolly], type: Copy
 from dependenciesPath ?: tarTree(downloadFolly.dest)
 from 'src/main/jni/third-party/folly/Android.mk'
 include "folly-${FOLLY_VERSION}/folly/**/*", 'Android.mk'
- eachFile {fname -> fname.path = (fname.path - "folly-${FOLLY_VERSION}/")}
+ eachFile { fname -> fname.path = (fname.path - "folly-${FOLLY_VERSION}/") }
 includeEmptyDirs = false

 // Patch for folly build break on gcc 4.9 and could be removed after build by clang


Should clean the block below "Patch for folly build break on gcc 4.9 and could be removed after build by clang"

akshetpandey · 2018-12-14T05:35:43Z

Any updates on this?

hramos · 2018-12-20T22:40:07Z

It's still being worked on. There's some changes we've had to make internally. We're close to shipping.

Summary: Pull Request resolved: facebook#22231 - Use clang instead of the deprecated gcc - Use libc++ instead of the deprecated gnustl - Updated gradle and android plugin version - Fixed missing arch in local-cli template - `clean` task should now always succeed - `clean` task deletes build artifacts - No need to specify buildToolsVersion. It's derived. - Elvis operator for more readable code Pull Request resolved: facebook#22263 Reviewed By: hramos Differential Revision: D13004499 Pulled By: DanielZlotin fbshipit-source-id: da54bb744cedb4c6f3bda590f8c25d0ad64086ef

ben-manes · 2019-02-04T20:15:25Z

This is great, but unfortunately there is a memory leak in an unresolved JSC bug (#23259). The GC does not support Android, so the default behavior only collects the young generation and OOME when the old is full. You can either compile with flags set to disable generational GC or apply the provided patch. Unfortunately without fixing JSC itself, RN will crash in long running application.

Summary: Pull Request resolved: facebook/react-native#22231 - Use clang instead of the deprecated gcc - Use libc++ instead of the deprecated gnustl - Updated gradle and android plugin version - Fixed missing arch in local-cli template - `clean` task should now always succeed - `clean` task deletes build artifacts - No need to specify buildToolsVersion. It's derived. - Elvis operator for more readable code Pull Request resolved: facebook/react-native#22263 Reviewed By: hramos Differential Revision: D13004499 Pulled By: DanielZlotin fbshipit-source-id: da54bb744cedb4c6f3bda590f8c25d0ad64086ef

Summary: Pull Request resolved: facebook#22231 - Use clang instead of the deprecated gcc - Use libc++ instead of the deprecated gnustl - Updated gradle and android plugin version - Fixed missing arch in local-cli template - `clean` task should now always succeed - `clean` task deletes build artifacts - No need to specify buildToolsVersion. It's derived. - Elvis operator for more readable code Pull Request resolved: facebook#22263 Reviewed By: hramos Differential Revision: D13004499 Pulled By: DanielZlotin fbshipit-source-id: da54bb744cedb4c6f3bda590f8c25d0ad64086ef

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. ✅pr adds tests labels Nov 14, 2018

hramos removed the ✅pr adds tests label Nov 14, 2018

DanielZlotin mentioned this pull request Nov 14, 2018

Update gradle and mk files #22231

Closed

gengjiawen reviewed Nov 14, 2018

View reviewed changes

ReactAndroid/build.gradle Show resolved Hide resolved

gengjiawen approved these changes Nov 14, 2018

View reviewed changes

DanielZlotin changed the title ~~Update gradle and mk files (#22231)~~ Update gradle and mk files Nov 15, 2018

Kudo mentioned this pull request Nov 15, 2018

Raise out android-jsc issues in current master #22274

Closed

3 tasks

gengjiawen requested a review from dulmandakh November 15, 2018 10:52

kelset added the Platform: Android Android applications. label Nov 15, 2018

gengjiawen mentioned this pull request Nov 20, 2018

[Do not land] test new jsc #22334

Closed

DanielZlotin force-pushed the export-D13004499 branch from e3070b4 to ab8148a Compare November 25, 2018 12:27

DanielZlotin requested a review from hramos as a code owner November 25, 2018 12:27

DanielZlotin changed the title ~~Update gradle and mk files~~ [Android] Use new JavaScriptCore from npm Nov 25, 2018

gengjiawen suggested changes Nov 25, 2018

View reviewed changes

facebook-github-bot reviewed Nov 25, 2018

View reviewed changes

DanielZlotin force-pushed the export-D13004499 branch from ab8148a to fcbfde1 Compare November 25, 2018 15:41

gengjiawen suggested changes Nov 26, 2018

View reviewed changes

hey99xx reviewed Nov 27, 2018

View reviewed changes

gengjiawen mentioned this pull request Nov 28, 2018

Release to maven center. react-native-community/jsc-android-buildscripts#71

Open

hramos added Core Team JavaScript labels Dec 14, 2018

hramos changed the title ~~[Android] Use new JavaScriptCore from npm~~ Use new JavaScriptCore from npm Dec 14, 2018

facebook-github-bot closed this in f3e5cce Dec 27, 2018

Kudo mentioned this pull request Dec 28, 2018

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

Closed

1 task

hey99xx mentioned this pull request Apr 4, 2019

Use node package dependency to manage JSC version #24276

Closed

DanielZlotin mentioned this pull request Feb 18, 2020

[RFC][RNTester] Move Flipper init out of app code #28052

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new JavaScriptCore from npm #22263

Use new JavaScriptCore from npm #22263

DanielZlotin commented Nov 14, 2018

pull-bot commented Nov 14, 2018 •

edited

Loading

DanielZlotin commented Nov 14, 2018

DanielZlotin commented Nov 14, 2018

gengjiawen commented Nov 15, 2018

kelset commented Nov 15, 2018

gusgard commented Nov 16, 2018

DanielZlotin commented Nov 25, 2018

gengjiawen commented Nov 25, 2018

gengjiawen Nov 25, 2018

DanielZlotin Nov 25, 2018

gengjiawen Nov 25, 2018

hey99xx Nov 27, 2018

gengjiawen Nov 28, 2018

gengjiawen commented Nov 25, 2018 •

edited

Loading

facebook-github-bot left a comment

gengjiawen Nov 25, 2018

gengjiawen Nov 26, 2018

DanielZlotin Nov 27, 2018

gengjiawen Nov 28, 2018

radeno Dec 20, 2018 •

edited

Loading

gengjiawen Nov 26, 2018

DanielZlotin Nov 27, 2018

hey99xx Nov 27, 2018 •

edited

Loading

akshetpandey commented Dec 14, 2018

hramos commented Dec 20, 2018

ben-manes commented Feb 4, 2019 •

edited

Loading

@@ @@ -30,8 +30,3 @@ allprojects { @@
  }
  }
 }

Use new JavaScriptCore from npm #22263

Use new JavaScriptCore from npm #22263

Conversation

DanielZlotin commented Nov 14, 2018

pull-bot commented Nov 14, 2018 • edited Loading

DanielZlotin commented Nov 14, 2018

DanielZlotin commented Nov 14, 2018

gengjiawen commented Nov 15, 2018

kelset commented Nov 15, 2018

gusgard commented Nov 16, 2018

DanielZlotin commented Nov 25, 2018

gengjiawen commented Nov 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gengjiawen commented Nov 25, 2018 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeno Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hey99xx Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

akshetpandey commented Dec 14, 2018

hramos commented Dec 20, 2018

ben-manes commented Feb 4, 2019 • edited Loading

pull-bot commented Nov 14, 2018 •

edited

Loading

gengjiawen commented Nov 25, 2018 •

edited

Loading

radeno Dec 20, 2018 •

edited

Loading

hey99xx Nov 27, 2018 •

edited

Loading

ben-manes commented Feb 4, 2019 •

edited

Loading