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.

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] - 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

grabbou · 2020-10-26T12:39:29Z

Note: Upgrading to CocoaPods 1.10.0 in this PR to resolve #29984

alloy · 2020-10-27T09:41:12Z

@grabbou Looking at the CI output, it doesn't look like the pod install step is using the Gemfile that locks the CP version, it should be bundle exec pod install. So perhaps it’s still using an older version of CP?

hramos · 2020-10-27T19:04:39Z

@grabbou Looking at the CI output, it doesn't look like the pod install step is using the Gemfile that locks the CP version, it should be bundle exec pod install. So perhaps it’s still using an older version of CP?

Correct, we should be using bundle exec there. I have an internal diff (external PR copy here: #30250) which makes this change, but it's currently blocked on Xcode 12.1.0 becoming available on our internal CI. Feel free to make the bundle exec change as part of this PR if necessary.

grabbou · 2020-10-29T15:55:58Z

@hramos I'll do that - thank you @alloy for spotting it.

…ve into feat/ios-hermes-try

grabbou · 2020-10-29T20:42:53Z

FYI @hramos, I bumped Xcode to 12 and it fixed the build time errors I was getting on the CI. I will rebase once your PR is merged and remove my commits.

grabbou · 2020-10-29T20:43:58Z

packages/rn-tester/RNTesterIntegrationTests/RCTLoggingTests.m

@@ -90,7 +90,7 @@ - (void)testLogging

  XCTAssertEqual(_lastLogLevel, RCTLogLevelError);
  XCTAssertEqual(_lastLogSource, RCTLogSourceJavaScript);
-  XCTAssertEqualObjects(_lastLogMessage, @"Invariant Violation: Invariant failed");
+  XCTAssertTrue([_lastLogMessage containsString:@"Invariant Violation: Invariant failed"]);


js engine: Hermes is added to the end of the error message / invariant when Hermes is on. Replaced with containsString to make it more universal.

CC: @alloy

grabbou · 2020-10-29T20:44:42Z

packages/rn-tester/RNTesterIntegrationTests/RCTLoggingTests.m

@@ -114,7 +114,7 @@ - (void)testLogging

  XCTAssertEqual(_lastLogLevel, RCTLogLevelError);
  XCTAssertEqual(_lastLogSource, RCTLogSourceJavaScript);
-  XCTAssertEqualObjects(_lastLogMessage, @"Error: Throwing an error");
+  XCTAssertTrue([_lastLogMessage containsString:@"Error: Throwing an error"]);


On that note, shall we have two targets for integration tests and two targets for unit tests and run them on both Hermes and JSC? To be honest, it sounds to me like an overkill, but would like to discuss this idea.

Right now, Hermes is turned on by default in RNTester on iOS. As a result, all tests that we run are against Hermes.

I wonder if we can achieve that simply by passing something like GCC_PREPROCESSOR_DEFINITIONS ="RCT_USE_HERMES=0" to the xcodebuild command, rather than adding extra targets.

Either way, it might be better to do that in a follow-up PR?

grabbou · 2020-10-29T20:45:58Z

The good news for anyone who is following this PR is - the build failures on CircleCI are finally gone! This week I spent on trying to understand why everything works and is green on my laptop and breaks on the CI.

My plan for tomorrow is to make both test_ios_unit and test_ios_unit_frameworks green and #shipit.

grabbou · 2020-10-30T08:01:25Z

IntegrationTests/GlobalEvalWithSourceUrlTest.js

@@ -42,7 +42,8 @@ class GlobalEvalWithSourceUrlTest extends React.Component<{...}> {
        'Expected globalEvalWithSourceUrl to throw on a syntax error',
      );
    }
-    if (!(syntaxError instanceof SyntaxError)) {
+    // Hermes throws an Error, not a SyntaxError
+    if (syntaxError.jsEngine !== 'hermes' && !(syntaxError instanceof SyntaxError)) {


The exact error is:

Error: Exception in HostFunction: Compiling JS failed: 1:2:'}' expected at end of block

Is this a bug or a known characteristics of Hermes?

CC: @Huxpro

@grabbou It's kinda related to a known problem that is complex and not going to be resolved anytime soon. It's complex since it's not so much of Hermes but the way we are building and linking our apps. I think we can leave the code this way for now and file an issue against the Hermes repo so we can track it down later.

analysis-bot · 2020-10-30T08:24:40Z

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

Base commit: 36b0f7d

analysis-bot · 2020-10-30T09:32:57Z

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

Base commit: 36b0f7d

hramos · 2020-10-30T16:07:18Z

@grabbou no need to wait for my PR to land first, don't let that block you, I can rebase after your changes land. As for the failing Android tests, we landed some fixes to master recently.

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.

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

0 of 3 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 3 commits Oct 26, 2020

Invalidate cache

Loading status checks…

fe7cbef

chore: do not pull CP repo, its using CDN anyway

Loading status checks…

7fee493

fix: upgrade CocoaPods to fix Xcode 12 and other issues

Loading status checks…

5f92caa

invalidate the cache once again

Loading status checks…

ec5f23b

AkalUstat mentioned this pull request Oct 29, 2020

Enable Hermes on Android shabados/mobile#108

Open

osdnk mentioned this pull request Oct 29, 2020

Question: support for Hermes on iOS microsoft/react-native-code-push#1966

Open

grabbou added 6 commits Oct 29, 2020

chore: use CocoaPods from Gemfile

82253cf

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

Loading status checks…

a1da047

…ve into feat/ios-hermes-try

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

Loading status checks…

9aa5815

chore: bump xcode to see if it helps

Loading status checks…

80142fb

chore: bump os

Loading status checks…

579d03f

fix: some unit tests

Loading status checks…

e08d0d0

grabbou reviewed Oct 29, 2020

View changes

fix: unit tests

Loading status checks…

5351d68

grabbou reviewed Oct 30, 2020

View changes

grabbou added 3 commits Oct 30, 2020

fix: build failure of libevent in use_frameworks mode

Loading status checks…

48eeaa5

fix: eslint issue

e131e10

Revert changes to libevent for now

Loading status checks…

7379157

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.

grabbou commented Oct 26, 2020

alloy commented Oct 27, 2020

hramos commented Oct 27, 2020

grabbou commented Oct 29, 2020

grabbou commented Oct 29, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

grabbou commented Oct 29, 2020

This comment has been minimized.

This comment has been minimized.

analysis-bot commented Oct 30, 2020

analysis-bot commented Oct 30, 2020

hramos commented Oct 30, 2020

alloy commented Oct 30, 2020

Huxpro commented Oct 30, 2020 •

edited

facebook / react-native

Join GitHub today

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.

grabbou commented Oct 26, 2020

alloy commented Oct 27, 2020

hramos commented Oct 27, 2020

grabbou commented Oct 29, 2020

grabbou commented Oct 29, 2020

This comment has been minimized.

grabbou Oct 29, 2020 Author Collaborator

This comment has been minimized.

grabbou Oct 29, 2020 • edited Author Collaborator

This comment has been minimized.

alloy Oct 30, 2020 Collaborator

grabbou commented Oct 29, 2020

This comment has been minimized.

grabbou Oct 30, 2020 • edited Author Collaborator

This comment has been minimized.

Huxpro Oct 30, 2020 • edited Contributor

analysis-bot commented Oct 30, 2020

analysis-bot commented Oct 30, 2020

hramos commented Oct 30, 2020

alloy commented Oct 30, 2020

Fastest turnaround time

Medium turnaround time

Longest turnaround time

Huxpro commented Oct 30, 2020 • edited

Essential cookies

Always active

Analytics cookies

grabbou commented Sep 9, 2020 •

edited

analysis-bot commented Sep 9, 2020 •

edited

grabbou Oct 29, 2020
Author Collaborator

grabbou Oct 29, 2020 •

edited

Author Collaborator

alloy Oct 30, 2020
Collaborator

grabbou Oct 30, 2020 •

edited

Author Collaborator

Huxpro Oct 30, 2020 •

edited

Contributor

Huxpro commented Oct 30, 2020 •

edited