Add Hermes support to React Native on Android by cpojer · Pull Request #25613 · facebook/react-native

cpojer · 2019-07-12T12:05:55Z

Summary

Yesterday we shipped hermesengine.dev as part of the current 0.60 release. This PR brings those changes to master.

Changelog

[General] [Added] - Added support for Hermes

Test Plan

CI is green both on GitHub and at FB
Creating a new app from source can use Hermes on Android

facebook-github-bot

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

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

pull-bot · 2019-07-12T12:08:24Z

	Warnings
⚠️	🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against 50bbfca

benoitdion · 2019-07-12T12:31:21Z

should this be wrapped in an if (enableHermes)? We don't need both hermes and jsc.

benoitdion · 2019-07-12T12:35:09Z

what do you think about moving this to react.gradle so all apps don't have to manually do this? we'd need to be able to infer if a variant requires the release or debug aar. There's already hermesFlagsRelease/hermesFlagsDebug so we could build on that concept.

This change is really large, I prefer to land it and then hope the community will help improve it. Right now this is following what JSC is doing already.

hramos · 2019-07-12T18:11:07Z

test_js failure appears to be a problem handling eslint's response (no errors found, but it returned a non-zero error code). We need to figure out what may have changed between master and the Hermes branch in terms of CI. I don't think this particular failure is merge-blocking, but I'd appreciate help fixing it should it continue appearing on master.
test_js_lts failure matches test_js's, same argument applies.
test_android failure needs to be looked at. Looks like the Hermes branch is missing some of the newer AndroidX changes.
test_docker failure is related to the same issue in test_android.

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

analysis-bot · 2019-07-15T08:24:34Z

no-unused-vars: 'Property' is defined but never used.

analysis-bot · 2019-07-15T08:24:34Z

no-unused-vars: 'Writable' is defined but never used.

facebook-github-bot

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

janicduplessis · 2019-07-15T21:45:57Z

@cpojer Looks like there is a missing BUCK config for hermes.reactexecutor, which then needs to be imported in ReactAndroid/src/main/java/com/facebook/react/BUCK.

cpojer · 2019-07-15T21:51:04Z

Thanks @janicduplessis. I didn’t have time to work on it yet. Any chance you could create the buck file so I can amend it into this PR? I’d love to merge this PR this week but not sure I’ll have the time.

janicduplessis · 2019-07-15T22:14:22Z

@cpojer Is the hermes executor used internally at fb? I'd be surprised if the buck file doesn't already exist. Anyway I'll look into adding it and see if I can get this PR building with BUCK later today.

cpojer · 2019-07-16T14:02:58Z

Buck problem is fixed, now we are actually down to the interesting issues.

cpojer · 2019-07-17T12:20:20Z

Currently blocked on an issue that @mhorowitz is looking into.

Minishlink · 2019-07-18T12:49:03Z

Hi @cpojer, would you mind releasing a 0.60.4 please? The last two commits enables to use Sentry and also reduce the APK size (since it doesn't include sourcemaps anymore)

cpojer · 2019-07-18T12:51:07Z

@Minishlink we'll be making a new release soon.

facebook-github-bot

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

Minishlink · 2019-07-18T13:50:22Z

Thanks @cpojer 😃

janicduplessis · 2019-07-19T02:23:29Z

Got another patch to fix proguard support janicduplessis@aa4dd9e. I guess if this is about to land we can wait and I'll open a new PR.

cpojer · 2019-07-19T10:55:16Z

@janicduplessis currently working on getting CI working at FB. I'll manually pick your change into the internal version of this PR. Thanks for linking it!

janicduplessis · 2019-07-19T17:05:24Z

@cpojer Oups I just noticed I missed adding @DoNotStrip on the first method. janicduplessis@aa4dd9e is the fixed version.

Kudo · 2019-07-25T11:35:23Z

+        pickFirst '**/arm64-v8a/libc++_shared.so'
+        pickFirst '**/x86_64/libc++_shared.so'
+        pickFirst '**/x86/libjsc.so'
+        pickFirst '**/armeabi-v7a/libjsc.so'


Just curious if the pickFirst for libjsc.so is needed?

IMO, even the pickFirst for libc++_shared.so getting some risk.
I knew that Hermes AAR includes libc++_shared.so, so we need the pickFirst here.
Hermes libc++_shared.so should be from NDK r13b and RN's is from NDK r17c or r19c.
This works because NDK maintains good ABI compatibility.
However, if user have other 3rd party gradle dependencies, gradle may pick to the 3rd party libc++_shared.so which is not what we expected.
This kind of problem is hard to troubleshoot because gradle pickFirst is really undeterministic.

Recommend if Hermes could remove libc++_shared.so from AAR and we could remove the pickFirst from template.

react-native-bot · 2019-07-26T06:10:05Z

This pull request was successfully merged by @cpojer in d7f5153.

^{When will my fix make it into a release? | Upcoming Releases}

cpojer · 2019-07-26T08:01:01Z

It landed thanks to @willholen who helped patch everything up at FB. I hope this isn't going to cause internal issues and has to be reverted – we should know more about that at the end of the day.

If you are having suggestions on how to improve Hermes integration, please send PRs to make it incrementally better :)

cpojer requested a review from hramos as a code owner July 12, 2019 12:05

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. p: Facebook Partner: Facebook Partner RN Team labels Jul 12, 2019

facebook-github-bot reviewed Jul 12, 2019

View reviewed changes

analysis-bot suggested changes Jul 12, 2019

View reviewed changes

react-native-bot added Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Jul 12, 2019

benoitdion reviewed Jul 12, 2019

View reviewed changes

cpojer force-pushed the master branch from a6a8e38 to 51e0e97 Compare July 15, 2019 08:11

analysis-bot suggested changes Jul 15, 2019

View reviewed changes

cpojer force-pushed the master branch from b7a373f to 8410e3f Compare July 15, 2019 08:25

facebook-github-bot reviewed Jul 15, 2019

View reviewed changes

cpojer force-pushed the master branch from 8410e3f to 881a0d4 Compare July 15, 2019 12:37

cpojer force-pushed the master branch from 881a0d4 to ccc6b97 Compare July 16, 2019 13:28

motiz88 mentioned this pull request Jul 17, 2019

[0.60.3] --sourcemap-output | enableHermes: false | gradle file | wrong output path #25693

Closed

cpojer force-pushed the master branch 4 times, most recently from 34e9993 to 13ad3aa Compare July 18, 2019 12:26

cpojer and others added 8 commits July 18, 2019 13:50

Add Hermes support to React Native

e3ffae4

Fix hermes path when building from source in ReactAndroid/build.gradle

cd6b247

Fix hermes header include paths when building from source

bea7fdb

Lint

a30750b

Add BUCK files.

4a6a537

Generate correct source map if hermes not enabled (facebook#25700)

e45df9a

[Android] Generate source maps outside of assets/ (facebook#25710)

50bbfca

Possible fix for the CI issue.

ac2624b

cpojer force-pushed the master branch from 13ad3aa to ac2624b Compare July 18, 2019 12:50

facebook-github-bot reviewed Jul 18, 2019

View reviewed changes

Kudo reviewed Jul 25, 2019

View reviewed changes

facebook-github-bot closed this in d7f5153 Jul 26, 2019

react-native-bot added the Merged This PR has been merged. label Jul 26, 2019

fbartho mentioned this pull request Mar 5, 2020

Respect/Merge Input Source Maps With Generated Source Map Data facebook/metro#104

Open

Conversation

cpojer commented Jul 12, 2019

Summary

Changelog

Test Plan

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

analysis-bot left a comment

Choose a reason for hiding this comment

Uh oh!

pull-bot commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benoitdion Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

benoitdion Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

cpojer Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

hramos commented Jul 12, 2019

Uh oh!

analysis-bot left a comment

Choose a reason for hiding this comment

Uh oh!

analysis-bot left a comment

Choose a reason for hiding this comment

Uh oh!

analysis-bot Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

analysis-bot Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

janicduplessis commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpojer commented Jul 15, 2019

Uh oh!

janicduplessis commented Jul 15, 2019

Uh oh!

cpojer commented Jul 16, 2019

Uh oh!

cpojer commented Jul 17, 2019

Uh oh!

Minishlink commented Jul 18, 2019

Uh oh!

cpojer commented Jul 18, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Minishlink commented Jul 18, 2019

Uh oh!

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

Uh oh!

Uh oh!

cpojer commented Jul 19, 2019

Uh oh!

janicduplessis commented Jul 19, 2019

Uh oh!

Kudo Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

react-native-bot commented Jul 26, 2019

Uh oh!

cpojer commented Jul 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

pull-bot commented Jul 12, 2019 •

edited

Loading

janicduplessis commented Jul 15, 2019 •

edited

Loading

janicduplessis commented Jul 19, 2019 •

edited

Loading