feat: Enable Hermes to work on iOS #29914

grabbou · 2020-09-09T17:12:58Z

Summary

This PR makes it possible to build iOS applications with Hermes. Note that it doesn't work with use_frameworks! just yet.

Fixes #27845 (by downgrading iOS deployment target for RCT-Folly to 9.0)
Fixes #28810 (as above)

Checklist:

Adjust release scripts to create Hermes bytecode bundle
Release new Hermes npm package that includes iOS files (unreleased right now, if you want to try locally, you have to clone Hermes and yarn link its master to this project)
Test on a new React Native application in both Debug and Release (Device)
Test on an RNTester application in both Debug and Release (Device)
Add missing i386 to Hermes framework and enable Bitcode
Inspect CI failures for possible regressions
Resolve Folly issue as reported #27845 and #28810
Release new Hermes and test against it that everything works

Changelog

[IOS] [FEATURE] - Enable Hermes on iOS
[INTERNAL] - Upgrade to CocoaPods 1.10.0 to resolve Xcode 12.0 issues
[INTERNAL] - Upgrade to Xcode 12.0 on the CircleCI
[INTERNAL] - Fix building RNTester in Release mode
[INTERNAL] - Fix build-time errors of libevent with use_frameworks!
[INTERNAL] - Introduce USE_HERMES variable and test all RNTester configurations on the CI
[INTERNAL] - Do not fetch CocoaPods repository since we're using CDN anyway

Test Plan

Turn on hermes_enabled to true in your Podfile, install pods, and run the iOS application. Your app should be running Hermes now.

Preview: (note "Engine: Hermes")

React-Core.podspec

package.json

third-party-podspecs/RCT-Folly.podspec

package.json

packages/react-native-codegen/android/gradlew.bat

alloy

This is amazing! Great stuff as always, @grabbou 👏

Thus far the changes are looking good to me, but will review again when it's no longer marked as WIP.

analysis-bot · 2020-09-09T17:45:46Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,211,837	-182,156
android	hermes	armeabi-v7a	6,861,013	-156,200
android	hermes	x86	7,646,477	-185,632
android	hermes	x86_64	7,537,421	-188,270
android	jsc	arm64-v8a	9,370,806	-165,392
android	jsc	armeabi-v7a	9,012,102	-139,124
android	jsc	x86	9,233,530	-167,344
android	jsc	x86_64	9,810,678	-171,906

Base commit: 36b0f7d

packages/rn-tester/RNTesterPods.xcodeproj/project.pbxproj

scripts/hermes-binary.sh

scripts/react-native-xcode.sh

Summary: The nested `.gitattributes` file in `packages/react-native-codegen/android/` caused some confusion on Linux and macOS, causing Git to show `packages/react-native-codegen/android/gradlew.bat` as modified (CRLF removed, LF added). Instead of relying on repo-local `.gitattributes` files to convert endings in the working directory, the files should be committed to source control with the correct line endings in the first place. There is no reason to convert LF endings in .sh and many other file to CRLF on Windows (maybe this was an issue a long time ago, but unless Notepad is used this won't be a problem for practically all modern editors). Also fixed the line endings of `scripts/launchPackager.bat` which was incorrectly committed as LF. ## Changelog [Internal] [Fixed] - Line endings and .gitattributes Pull Request resolved: #29792 Test Plan: Clone repo on Linux, macOS, and Windows, and make sure no modified files show up. Reviewed By: fkgozali Differential Revision: D23546135 Pulled By: mdvacca fbshipit-source-id: 1572fcb959212f212b137066f1aa66f0bb6e86c3

alloy · 2020-10-30T16:18:38Z

Current status is:

This branch builds when building static libraries. When building dynamic frameworks it fails building the libevent framework, for this issue I have a PR up here: callstack#26
After fixing that, I stumbled on the issue of RN master’s copy of JSI being out of sync with JSI that comes with hermes v0.7.1. Specifically this recent RN commit.
To at least try to get this PR into a mergeable state, I wanted to then switch to consumer Hermes from master, but ran into the issue of Hermes master failing to build for iOS. I’ve tracked the issue down to this Hermes commit which makes the hermesc binary a dependency, which we don’t build for iOS.

Reverting that commit does fix the iOS build again, but I’m unsure of the ramifications. The proper fix would be to first build the macOS artefacts and then re-use its hermesc artefact during the iOS build, but that might make this PR drag on again.

@Huxpro @TheSavior Do you have any thoughts on the best recourse/prioritization?

Possible options I see are the following:

Fastest turnaround time

This could technically be released in a stable RN release.

Revert the commit mentioned above in #2 in RN master.
~~Revert the commit mentioned above in #3 in hermes master.~~
Finish this PR by using the existing Hermes v0.7.1 release.

Medium turnaround time

This can not be released in a stable RN release.

Revert the commit mentioned above in #3 in hermes master.
Finish this PR by temporarily pointing the hermes dependency to hermes master.
Potentially make the proper fix to first build macOS artefacts and use the resulting hermesc binary.
[Soon-ish] release Hermes v0.7.2 with a JSI version matching RN master’s.
Update RN’s hermes dependency to the prebuilt version of v0.7.2.

Longest turnaround time

This can definitely be released in a stable RN release.

Revert the commit mentioned above in #3 in hermes master or make the proper fix to first build macOS artefacts and use the resulting hermesc binary.
Release Hermes v0.7.2 with a JSI version matching RN master’s.
Finish this PR by pointing RN’s hermes dependency to the prebuilt version of v0.7.2.

Huxpro · 2020-10-30T22:44:14Z

@alloy thanks for the detailed summary! It seems like the real road blocking issue was just the JSI getting out of sync by d8b0e9d and we only need to do 2 and 3 of the "longest turnaround time" option to get this PR rolling again.

We are working on a patch that include facebook/hermes@73594b5 and its dependencies onto the Hermes release-v0.7 branch to hopefully have a v0.7.2 that is JSI compatible with RN master's. By this way we don't need to revert from either RN or Hermes master. I'll keep everyone posted once it's merged.

After fixing that, I stumbled on the issue of RN master’s copy of JSI being out of sync with JSI that comes with hermes v0.7.1.

It would be helpful to know what are the steps to repro so we can verify if the patch resolve the issue. I can still build RN Tester app on Android on RN master with the hermes-engine-v0.7.1 npm package. Starting heap profiler does not crash the app but disabling it does. I'm not sure if that is the crash you encountered or another one.

To at least try to get this PR into a mergeable state, I wanted to then switch to consumer Hermes from master, but ran into the issue of Hermes master failing to build for iOS. I’ve tracked the issue down to this Hermes commit which makes the hermesc binary a dependency, which we don’t build for iOS.

The proper fix would be to first build the macOS artefacts and then re-use its hermesc artefact during the iOS build, but that might make this PR drag on again.

Yeah, that would be the proper fix. At some point we'll need a hermes v0.8.0 with the fix for RN 0.65, but that could be months. For now, we should probably not consider using Hermes from master for RN 0.64 a very feasible option.

Make libevent work for static and framework builds

grabbou · 2020-11-02T08:59:44Z

Here's additional option (on top of what @alloy already pointed out) as this PR is getting older and older and starts to have a lot of changes inside it:

This branch builds when building static libraries. When building dynamic frameworks it fails.

Merge it and release a release candidate with a note that use_hermes on iOS only works with static libraries (which is the default setup, use_frameworks! not enabled) and that support for dynamic frameworks will be fixed in the next release candidate.

CC: @TheSavior

TheSavior · 2020-11-02T09:06:31Z

I'm not sure what the all implications of that would be, but in general it seems totally reasonable to land parts of this PR to make things easier on you instead of batching them altogether. You and @alloy can decide what the right call is here.

analysis-bot · 2020-11-02T09:25:38Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 97d6f2e

alloy · 2020-11-02T10:12:48Z

@alloy thanks for the detailed summary! It seems like the real road blocking issue was just the JSI getting out of sync by d8b0e9d and we only need to do 2 and 3 of the "longest turnaround time" option to get this PR rolling again.

Agreed.

We are working on a patch that include facebook/hermes@73594b5 and its dependencies onto the Hermes release-v0.7 branch to hopefully have a v0.7.2 that is JSI compatible with RN master's. By this way we don't need to revert from either RN or Hermes master. I'll keep everyone posted once it's merged.

Perfect 🙏

After fixing that, I stumbled on the issue of RN master’s copy of JSI being out of sync with JSI that comes with hermes v0.7.1.

It would be helpful to know what are the steps to repro so we can verify if the patch resolve the issue. I can still build RN Tester app on Android on RN master with the hermes-engine-v0.7.1 npm package.

The failure is a build time failure. Try building this PR with dynamic frameworks enabled and then build:

cd packages/rn-tester
env USE_FRAMEWORKS=1 pod install

The proper fix would be to first build the macOS artefacts and then re-use its hermesc artefact during the iOS build, but that might make this PR drag on again.

Yeah, that would be the proper fix. At some point we'll need a hermes v0.8.0 with the fix for RN 0.65, but that could be months. For now, we should probably not consider using Hermes from master for RN 0.64 a very feasible option.

Gotcha 👍

alloy · 2020-11-02T10:14:27Z

Here's additional option […] Merge it and release a release candidate with a note that use_hermes on iOS only works with static libraries (which is the default setup, use_frameworks! not enabled) and that support for dynamic frameworks will be fixed in the next release candidate.

I’m in favour. I’d perhaps even go farther and not promise it to be fixed in a RC, just at a later time.

grabbou · 2020-11-02T10:17:36Z

I’m in favour. I’d perhaps even go farther and not promise it to be fixed in a RC, just at a later time.

Interestingly, Flipper doesn't work with use_frameworks! either right now.

# Enables Flipper.
#
# Note that if you have use_frameworks! enabled, Flipper will not work and
# you should disable these next few lines.

https://github.com/facebook/react-native/blob/master/template/ios/Podfile#L20-L23

Could that be related? Either way, I think we're good.

…ve into feat/ios-hermes-try

grabbou · 2020-11-02T10:40:49Z

Opening #30300 restarted CircleCI. Yay!

grabbou · 2020-11-02T11:40:33Z

All tests are passing except test_ios_unit_frameworks_hermes, which is expected. This is the variation that doesn't work right now, as per previous discussion.

This PR is ready to land. We may want to disable the failing job so that the CI remains green. Hope this can be done while landing to avoid re-running all tests. If not, I will submit another commit.

packages/rn-tester/Podfile

Co-authored-by: Eloy Durán <eloy.de.enige@gmail.com>

alloy

This looks good to me now 👍

Thanks for doing this work, @grabbou 👏🙌

yungsters · 2020-11-02T17:03:52Z

@facebook-github-bot import

facebook-github-bot

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

react-native-bot · 2020-11-03T09:20:05Z

This pull request was successfully merged by @grabbou in c95ee5a.

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

feat: initial commit Hermes on iOS

Loading status checks…

4c21abe

grabbou requested review from cpojer, hramos and mhorowitz as code owners Sep 9, 2020

grabbou reviewed Sep 9, 2020

View changes

React-Core.podspec Outdated Show resolved Hide resolved

grabbou reviewed Sep 9, 2020

View changes

package.json Outdated Show resolved Hide resolved

chore: update version

Loading status checks…

1f3598d

grabbou reviewed Sep 9, 2020

View changes

third-party-podspecs/RCT-Folly.podspec Show resolved Hide resolved

Update gradlew.bat

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

3aa3d95

facebook-github-bot added CLA Signed p: Callstack Partner labels Sep 9, 2020

alloy reviewed Sep 9, 2020

View changes

package.json Outdated Show resolved Hide resolved

TheSavior reviewed Sep 9, 2020

View changes

packages/react-native-codegen/android/gradlew.bat Outdated Show resolved Hide resolved

alloy reviewed Sep 9, 2020

View changes

This comment was marked as off-topic.

Sign in to view

This comment has been hidden.

Sign in to view

TheSavior mentioned this pull request Sep 12, 2020

v0.64.0 Discussion react-native-community/releases#207

Open

3 of 8 tasks complete

This comment was marked as off-topic.

Sign in to view

chore: release script tweaks

Loading status checks…

52bc83a

grabbou reviewed Sep 14, 2020

View changes

packages/rn-tester/RNTesterPods.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved

grabbou reviewed Sep 14, 2020

View changes

scripts/hermes-binary.sh Outdated Show resolved Hide resolved

grabbou reviewed Sep 14, 2020

View changes

scripts/react-native-xcode.sh Show resolved Hide resolved

This comment has been hidden.

Sign in to view

friederbluemle and others added 3 commits Sep 4, 2020

chore: update line endings after cherry-picking fix

Loading status checks…

f7592b0

Merge branch 'master' into feat/ios-hermes-try

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

6921e6b

This comment has been hidden.

Sign in to view

grabbou added 2 commits Nov 2, 2020

fix: flow issue in test

Loading status checks…

59a92a2

chore: point out to Hermes issue

Loading status checks…

637a5a8

feat: test jsc/hermes separate

Loading status checks…

9f924c0

grabbou added 4 commits Nov 2, 2020

Merge branch 'master' into feat/ios-hermes-try

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

2dff0ce

chore: remove unused params

d26d900

Merge branch 'feat/ios-hermes-try' of github.com:callstack/react-nati…

Loading status checks…

ff9d5ec

…ve into feat/ios-hermes-try

chore: small docs on enabling Hermes

Loading status checks…

b400e6c

grabbou mentioned this pull request Nov 2, 2020

[INTERNAL] Test Hermes and RNTester on CircleCI #30300

Closed

grabbou requested a review from alloy Nov 2, 2020

alloy reviewed Nov 2, 2020

View changes

packages/rn-tester/Podfile Outdated Show resolved Hide resolved

Update packages/rn-tester/Podfile

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

9ec3e29

Co-authored-by: Eloy Durán <eloy.de.enige@gmail.com>

alloy approved these changes Nov 2, 2020

View changes

facebook-github-bot reviewed Nov 2, 2020

View changes

grabbou mentioned this pull request Nov 2, 2020

Hermes throws Error instead of SyntaxError #30304

Open

facebook-github-bot closed this in c95ee5a Nov 3, 2020

react-native-bot added the Merged label Nov 3, 2020

facebook / react-native

feat: Enable Hermes to work on iOS #29914

feat: Enable Hermes to work on iOS #29914

grabbou commented Sep 9, 2020 •

edited

alloy left a comment

analysis-bot commented Sep 9, 2020 •

edited

This comment was marked as off-topic.

This comment has been hidden.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment has been hidden.

This comment has been hidden.

alloy commented Oct 30, 2020 •

edited

Huxpro commented Oct 30, 2020 •

edited

grabbou commented Nov 2, 2020 •

edited

TheSavior commented Nov 2, 2020 •

edited

analysis-bot commented Nov 2, 2020

alloy commented Nov 2, 2020

alloy commented Nov 2, 2020

grabbou commented Nov 2, 2020 •

edited

grabbou commented Nov 2, 2020

grabbou commented Nov 2, 2020

alloy left a comment

yungsters commented Nov 2, 2020

facebook-github-bot left a comment

react-native-bot commented Nov 3, 2020

facebook / react-native

Join GitHub today

GitHub is where the world builds software

feat: Enable Hermes to work on iOS #29914

feat: Enable Hermes to work on iOS #29914

Conversation

grabbou commented Sep 9, 2020 • edited

Summary

Changelog

Test Plan

alloy left a comment

analysis-bot commented Sep 9, 2020 • edited

This comment was marked as off-topic.

This comment has been hidden.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment has been hidden.

This comment has been hidden.

alloy commented Oct 30, 2020 • edited

Fastest turnaround time

Medium turnaround time

Longest turnaround time

Huxpro commented Oct 30, 2020 • edited

grabbou commented Nov 2, 2020 • edited

TheSavior commented Nov 2, 2020 • edited

analysis-bot commented Nov 2, 2020

alloy commented Nov 2, 2020

alloy commented Nov 2, 2020

grabbou commented Nov 2, 2020 • edited

grabbou commented Nov 2, 2020

grabbou commented Nov 2, 2020

alloy left a comment

yungsters commented Nov 2, 2020

facebook-github-bot left a comment

react-native-bot commented Nov 3, 2020

Essential cookies

Always active

Analytics cookies

grabbou commented Sep 9, 2020 •

edited

analysis-bot commented Sep 9, 2020 •

edited

alloy commented Oct 30, 2020 •

edited

Huxpro commented Oct 30, 2020 •

edited

grabbou commented Nov 2, 2020 •

edited

TheSavior commented Nov 2, 2020 •

edited

grabbou commented Nov 2, 2020 •

edited