Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hermes executor not available when use_frameworks is enabled #34222

Closed
wants to merge 1 commit into from

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Jul 19, 2022

Summary

When in use_frameworks! + hermes_enabled mode, the React podspecs have not dependency to the React-hermes podspec. the HermesExecutorFactory.h is not available here and here. The app will fallback to JSCRuntime and crash in release build because the JSCRuntime cannot evaluate Hermes bundles.

The pr tries to add React-hermes to Framework Search Paths and make the HermesExecutorFactory.h reachable when in use_frameworks mode.

This pr should fix the problem mentioned in 0.70 release discussion.

Changelog

[iOS] [Fixed] - Fix Hermes executor not available when use_frameworks is enabled

Test Plan

use patch-package to apply the change to a RN 0.69.1 project with the following test cases:

  • old architecture + jsc
  • new architecture + hermes
  • old architecture + use_frameworks! :linkage => :static + hermes
  • old architecture +use_frameworks! :linkage => :static + hermes + release build

@facebook-github-bot 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. Contributor A React Native contributor. p: Expo Partner: Expo Partner labels Jul 19, 2022
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Jul 19, 2022
@analysis-bot
Copy link

analysis-bot commented Jul 19, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,825,302 +0
android hermes armeabi-v7a 7,216,532 +0
android hermes x86 8,138,262 +0
android hermes x86_64 8,115,811 +0
android jsc arm64-v8a 9,703,479 +0
android jsc armeabi-v7a 8,457,929 +0
android jsc x86 9,654,618 +0
android jsc x86_64 10,252,016 +0

Base commit: ccdf9ac
Branch: main

@analysis-bot
Copy link

analysis-bot commented Jul 19, 2022

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

Base commit: ccdf9ac
Branch: main

@Kudo Kudo marked this pull request as ready for review July 19, 2022 16:51
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jul 19, 2022
@cipolleschi
Copy link
Contributor

Hi @Kudo, thanks for the PR. However, I don't understand why, with use_framework! we lose the dependency from hermes.

From this file we should install hermes when it is enabled.

I would rather try to understand why with use_framework! this Pod is not installed and fix that, instead of adding a workaround, falling back to something else.

@Kudo
Copy link
Contributor Author

Kudo commented Jul 20, 2022

@cipolleschi in use_frameworks mode, each podspec would has a dedicated xcode target. the header search is mostly based on the Framework Search Paths settings. CocoaPods generates the values based on podspec dependencies.

Screen Shot 2022-07-20 at 11 26 43 PM
this is an example of Framework Search Paths from React-Core target, where contain both RCTAppSetupUtils and RCTCxxBridge files. there's no React-hermes in the Framework Search Paths and no hermes headers available to RCTAppSetupUtils/RCTCxxBridge.

the reason hermes works in non_use_frameworks mode is that the React-Core adds an header search entry to hermes. also in non_use_frameworks mode, the compiler don't have to generate *.a or *.so files. if headers are reachable, the compile can link *.o all together for final executable. however for use_frameworks mode, we should add correct framework search paths for xcode to link correct React-hermes libs.

that's my understands. hopefully that would help you to have some insight for further investigation.

@cipolleschi
Copy link
Contributor

Thanks for the clarification. I just have another question, following this:

the reason hermes works in non_use_frameworks mode is that the React-Core adds an header search entry to hermes. also in non_use_frameworks mode, the compiler don't have to generate *.a or *.so files. if headers are reachable, the compile can link *.o all together for final executable. however for use_frameworks mode, we should add correct framework search paths for xcode to link correct React-hermes libs.

I don't see any check that says to skip that line when use_frameworks! is used. So Why that change to the HEADER_SEARCH_PATHS is not applied in that case? Should we replicate that line in another podspec setting, perhaps?

@Kudo
Copy link
Contributor Author

Kudo commented Jul 20, 2022

I don't see any check that says to skip that line when use_frameworks! is used. So Why that change to the HEADER_SEARCH_PATHS is not applied in that case? Should we replicate that line in another podspec setting, perhaps?

because the path in use_frameworks mode is different. it's ${PODS_CONFIGURATION_BUILD_DIR}/React-hermes/reacthermes.framework/Headers. and we should also change #import <reacthermes/HermesExecutorFactory.h> to #import <HermesExecutorFactory.h>

sorry the following description was wrong. i just tested that. once i reached the header in ``${PODS_CONFIGURATION_BUILD_DIR}/React-hermes/reacthermes.framework/Headers`, xcode can build it successfully.

also in non_use_frameworks mode, the compiler don't have to generate *.a or *.so files. if headers are reachable, the compile can link *.o all together for final executable. however for use_frameworks mode, we should add correct framework search paths for xcode to link correct React-hermes libs.

@cipolleschi
Copy link
Contributor

because the path in use_frameworks mode is different. it's ${PODS_CONFIGURATION_BUILD_DIR}/React-hermes/reacthermes.framework/Headers. and we should also change #import <reacthermes/HermesExecutorFactory.h> to #import <HermesExecutorFactory.h>

I understand. Sorry if I'm throwing all these questions to you but:

  1. why is cocoapods adding the header into the search path for React-cxxreact and not for React-hermes?
  2. what happens if we add the here an entry "FRAMEWORK_SEARCH_PATH" with the proper configurations?
  3. Are we sure we need to change this #import <reacthermes/HermesExecutorFactory.h>? Can't we use the modular_headers settings to avoid it, for example?

I would rather try to reduce as much as we can the changes to the code to avoid the introduction of potential bugs.

@Kudo
Copy link
Contributor Author

Kudo commented Jul 20, 2022

  1. why is cocoapods adding the header into the search path for React-cxxreact and not for React-hermes?

React-Core.podspec doesn't have dependencies to React-hermes.

  1. what happens if we add the here an entry "FRAMEWORK_SEARCH_PATH" with the proper configurations?

brilliant idea. that's work and it's an easy fix. i'll update the pr tomorrow based on this solution.

  1. Are we sure we need to change this #import <reacthermes/HermesExecutorFactory.h>? Can't we use the modular_headers settings to avoid it, for example?

i didn't test this. i like approach 2.

I would rather try to reduce as much as we can the changes to the code to avoid the introduction of potential bugs.

np! i like these discussion and brainstorming for better solution. and there we are 👍

@Kudo Kudo force-pushed the fix-use-frameworks-hermes branch from 78ab43b to 6b6f78a Compare July 21, 2022 14:50
@cipolleschi
Copy link
Contributor

Hi Kudo, thanks for updating this. Could you rebase on main? 🙏

@Kudo Kudo force-pushed the fix-use-frameworks-hermes branch from 6b6f78a to 5ad512f Compare July 22, 2022 15:41
@Kudo
Copy link
Contributor Author

Kudo commented Jul 22, 2022

thanks for the update. let's see how it going for ci jobs

@Kudo
Copy link
Contributor Author

Kudo commented Jul 22, 2022

remaining a test js error , other ci jobs are passed 🤔

@cipolleschi
Copy link
Contributor

JS Error seems unrelated...

@facebook-github-bot
Copy link
Contributor

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @Kudo in 88b7b64.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jul 27, 2022
kelset pushed a commit that referenced this pull request Jul 27, 2022
)

Summary:
When in `use_frameworks!` + `hermes_enabled` mode, the React podspecs have not dependency to the `React-hermes` podspec. the `HermesExecutorFactory.h` is not available [here](https://github.com/facebook/react-native/blob/1d997ce6d67242b9f667b01365c5e719c3d3f8d7/React/AppSetup/RCTAppSetupUtils.h#L15) and [here](https://github.com/facebook/react-native/blob/1d997ce6d67242b9f667b01365c5e719c3d3f8d7/React/CxxBridge/RCTCxxBridge.mm#L44). The app will fallback to JSCRuntime and crash in release build because the JSCRuntime cannot evaluate Hermes bundles.

The pr tries to add `React-hermes` to Framework Search Paths and make the `HermesExecutorFactory.h` reachable when in use_frameworks mode.

This pr should fix the problem mentioned in [0.70 release discussion](reactwg/react-native-releases#26 (reply in thread)).

## Changelog

[iOS] [Fixed] - Fix Hermes executor not available when `use_frameworks` is enabled

Pull Request resolved: #34222

Test Plan:
use patch-package to apply the change to a RN 0.69.1 project with the following test cases:

- old architecture + jsc
- new architecture + hermes
- old architecture + `use_frameworks! :linkage => :static` + hermes
- old architecture +`use_frameworks! :linkage => :static` + hermes + release build

Reviewed By: sammy-SC

Differential Revision: D38112717

Pulled By: cipolleschi

fbshipit-source-id: ee78527f4686400856ab9a055cf30b20900afc17
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
…ebook#34222)

Summary:
When in `use_frameworks!` + `hermes_enabled` mode, the React podspecs have not dependency to the `React-hermes` podspec. the `HermesExecutorFactory.h` is not available [here](https://github.com/facebook/react-native/blob/1d997ce6d67242b9f667b01365c5e719c3d3f8d7/React/AppSetup/RCTAppSetupUtils.h#L15) and [here](https://github.com/facebook/react-native/blob/1d997ce6d67242b9f667b01365c5e719c3d3f8d7/React/CxxBridge/RCTCxxBridge.mm#L44). The app will fallback to JSCRuntime and crash in release build because the JSCRuntime cannot evaluate Hermes bundles.

The pr tries to add `React-hermes` to Framework Search Paths and make the `HermesExecutorFactory.h` reachable when in use_frameworks mode.

This pr should fix the problem mentioned in [0.70 release discussion](reactwg/react-native-releases#26 (reply in thread)).

## Changelog

[iOS] [Fixed] - Fix Hermes executor not available when `use_frameworks` is enabled

Pull Request resolved: facebook#34222

Test Plan:
use patch-package to apply the change to a RN 0.69.1 project with the following test cases:

- old architecture + jsc
- new architecture + hermes
- old architecture + `use_frameworks! :linkage => :static` + hermes
- old architecture +`use_frameworks! :linkage => :static` + hermes + release build

Reviewed By: sammy-SC

Differential Revision: D38112717

Pulled By: cipolleschi

fbshipit-source-id: ee78527f4686400856ab9a055cf30b20900afc17
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
…ebook#34222)

Summary:
When in `use_frameworks!` + `hermes_enabled` mode, the React podspecs have not dependency to the `React-hermes` podspec. the `HermesExecutorFactory.h` is not available [here](https://github.com/facebook/react-native/blob/1d997ce6d67242b9f667b01365c5e719c3d3f8d7/React/AppSetup/RCTAppSetupUtils.h#L15) and [here](https://github.com/facebook/react-native/blob/1d997ce6d67242b9f667b01365c5e719c3d3f8d7/React/CxxBridge/RCTCxxBridge.mm#L44). The app will fallback to JSCRuntime and crash in release build because the JSCRuntime cannot evaluate Hermes bundles.

The pr tries to add `React-hermes` to Framework Search Paths and make the `HermesExecutorFactory.h` reachable when in use_frameworks mode.

This pr should fix the problem mentioned in [0.70 release discussion](reactwg/react-native-releases#26 (reply in thread)).

## Changelog

[iOS] [Fixed] - Fix Hermes executor not available when `use_frameworks` is enabled

Pull Request resolved: facebook#34222

Test Plan:
use patch-package to apply the change to a RN 0.69.1 project with the following test cases:

- old architecture + jsc
- new architecture + hermes
- old architecture + `use_frameworks! :linkage => :static` + hermes
- old architecture +`use_frameworks! :linkage => :static` + hermes + release build

Reviewed By: sammy-SC

Differential Revision: D38112717

Pulled By: cipolleschi

fbshipit-source-id: ee78527f4686400856ab9a055cf30b20900afc17
piaskowyk pushed a commit to software-mansion/react-native-reanimated that referenced this pull request Oct 20, 2022
## Description

When there are frameworks used and Hermes enabled in the Podfile of your
project, RN falls back to JSC in debug mode and crashes in production
mode.

This PR makes it possible to use Hermes when there is `use_frameworks!`
enabled in the Podfile.

Thanks @Kudo who originally fixed this issue in ReactNative repository:
facebook/react-native#34222

## Changes

Added path to `Framework Search Paths` in Podspec of
react-native-reanimated.

## Test code and steps to reproduce

1. Create empty ReactNative project
2. Add react-native-reanimated dependency.
3. For ReactNative 0.69.3 apply patch from:
https://github.com/facebook/react-native/pull/34222/files
4. Enable frameworks in Podfile: `use_frameworks! :linkage => :static`
5. Build project in debug mode -> you will not see `engine: hermes` in
top right corner.
6. Build project in release mode -> app will crash at startup

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Added TS types tests
- [ ] Added unit / integration tests
- [ ] Updated documentation
- [ ] Ensured that CI passes
piaskowyk pushed a commit to software-mansion/react-native-reanimated that referenced this pull request Oct 20, 2022
When there are frameworks used and Hermes enabled in the Podfile of your
project, RN falls back to JSC in debug mode and crashes in production
mode.

This PR makes it possible to use Hermes when there is `use_frameworks!`
enabled in the Podfile.

Thanks @Kudo who originally fixed this issue in ReactNative repository:
facebook/react-native#34222

Added path to `Framework Search Paths` in Podspec of
react-native-reanimated.

1. Create empty ReactNative project
2. Add react-native-reanimated dependency.
3. For ReactNative 0.69.3 apply patch from:
https://github.com/facebook/react-native/pull/34222/files
4. Enable frameworks in Podfile: `use_frameworks! :linkage => :static`
5. Build project in debug mode -> you will not see `engine: hermes` in
top right corner.
6. Build project in release mode -> app will crash at startup

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Added TS types tests
- [ ] Added unit / integration tests
- [ ] Updated documentation
- [ ] Ensured that CI passes
fluiddot pushed a commit to wordpress-mobile/react-native-reanimated that referenced this pull request Jun 5, 2023
…-mansion#3491)

## Description

When there are frameworks used and Hermes enabled in the Podfile of your
project, RN falls back to JSC in debug mode and crashes in production
mode.

This PR makes it possible to use Hermes when there is `use_frameworks!`
enabled in the Podfile.

Thanks @Kudo who originally fixed this issue in ReactNative repository:
facebook/react-native#34222

## Changes

Added path to `Framework Search Paths` in Podspec of
react-native-reanimated.

## Test code and steps to reproduce

1. Create empty ReactNative project
2. Add react-native-reanimated dependency.
3. For ReactNative 0.69.3 apply patch from:
https://github.com/facebook/react-native/pull/34222/files
4. Enable frameworks in Podfile: `use_frameworks! :linkage => :static`
5. Build project in debug mode -> you will not see `engine: hermes` in
top right corner.
6. Build project in release mode -> app will crash at startup

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Added TS types tests
- [ ] Added unit / integration tests
- [ ] Updated documentation
- [ ] Ensured that CI passes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged. p: Expo Partner: Expo Partner Platform: iOS iOS applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants