Move hermes to a separate podspec

[janicduplessis]

Summary

Hermes being a subspec of ReactCore causes some build issues when RN is included in 2 different targets. It also causes it to include a lot of additional dependencies that it doesn't need. This moves it to a separate podspec loosely based on other specs in ReactCommon.

Changelog

[iOS] [Fixed] - Move hermes to a separate podspec

Test Plan

Test that it builds and run properly in an app

[janicduplessis]

Looks like the frameworks build is failing because it cannot find the hermes executor headers, I'm not familiar with using frameworks, is there an additional config needed to expose the headers?

[alloy]

The change seems fine to me, I just have a few questions first:

Hermes being a subspec of ReactCore causes some build issues when RN is included in 2 different targets.

For completeness sake, could you include some details on what these issues are?

It also causes it to include a lot of additional dependencies that it doesn't need.

That shouldn't happen when the subspec isn't enabled or is an issue when you include RN in 2 different targets, as mentioned above?

[alloy]

Looks like the frameworks build is failing because it cannot find the hermes executor headers, I'm not familiar with using frameworks, is there an additional config needed to expose the headers?

Those CI tasks are expected to fail at the moment, afaik /cc @grabbou

[janicduplessis]

@alloy Thanks for having a look!

For completeness sake, could you include some details on what these issues are?

I don't have the exact error handy but it was a Multiple commands produce x error because of React/AccessibilityResources/en.lproj which is included in the base spec config (https://github.com/facebook/react-native/blob/master/React-Core.podspec#L46). From what I understand all subspecs inherit this config and for some reason xcode seems to think it will need to copy this ressource twice if a target includes hermes and the other doesn't. In any case the hermes spec should not depend on this resource and a lot of the base deps specified in the Core subspec as it is just a simple c++ module.

That shouldn't happen when the subspec isn't enabled or is an issue when you include RN in 2 different targets, as mentioned above?

The issue happens only if one subspec includes hermes and the other doesn't. Kind of seems like a bug in cocoapods or xcode, but I think this is a proper fix anyway.

[janicduplessis]

Those CI tasks are expected to fail at the moment, afaik /cc @grabbou

Looks like the error is actually relevant. Not sure why it works without frameworks, but doesn't with.

Testing failed:
	'React/HermesExecutorFactory.h' file not found
	Testing cancelled because the build failed.

https://app.circleci.com/pipelines/github/facebook/react-native/7274/workflows/e6ec6468-ba89-43b7-bb00-d6e39ace3869/jobs/178891

From what I understand this https://github.com/facebook/react-native/pull/30478/files#diff-b34b33f2ba6fa8d5c29a589f91e5b8101289f634bb7bb0a54cdde77eb82fdecaR41 should allow headers to be imported using <React/X> but it doesn't seem to work for frameworks.

[alloy]

Thanks for the elaboration 🙏

As for the build issue, it makes sense that you can no longer refer to the header in the React namespace, as it's now part of the React-hermes module, so references to it should probably be updated to import from there instead. It's weird that it works with static libraries, which probably means there's some header access leaking somewhere between targets and/or an explicit header search path is being inserted somewhere that only resolves when using static libraries 🤷‍♂️

[janicduplessis]

Ok I managed to get everything working, the use_frameworks build is currently crashing at runtime because of duplicate Folly singleton. This is caused by Flipper, from what I understand it is not compatible with use_frameworks so I will open a separate PR to fix the use_frameworks build.

This moves the hermes executor to a new module reacthermes and use public_headers which seemed the less hacky solution to expose the module headers.

[alloy]

Nice 👌

[janicduplessis]

CI is failing for an unreleated reason now so it seems to work!

[facebook-github-bot]

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

[hramos]

Thanks for the changes! I think we can merge them. As for the name of the pod, it could be named plainly hermes.podspec, no React.

[janicduplessis]

@hramos I called it reacthermes to avoid conflicting with hermes, but I can try see if causes any issues. Actually the hermes podspec is called hermes-engine so that part should be fine. Just wondering about headers which are namespaced under <hermes/*> for hermes-engine already.

[alloy]

It would probably be good to namespace it with React to avoid any hassle, but also these are the RN and Hermès integration bits, so it makes sense to me to call it like that.

[janicduplessis]

Headers were initially namespaced under React since it was in ReactCore, I tried keeping that by using header_dir = 'React' but noticed it would generate 2 frameworks named React when using use_frameworks, which might work, but looked wrong. There's probably a way to configure the framework name, but I was thinking it would be better to use its own namespec a bit like we do for jsi (https://github.com/facebook/react-native/blob/master/ReactCommon/jsiexecutor/React-jsiexecutor.podspec#L35). It does mean I needed to update the header path in RNTester and one other RCTBridge file. Not sure if that could cause issues internally.

[hramos]

Let's leave it at React-hermes then. The update to the bridge file is fine.

[react-native-bot]

This pull request was successfully merged by @janicduplessis in 0959ff3.

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

[alloy]

@janicduplessis Do you think this should make its way into v0.64.0?

[janicduplessis]

Probably not critical, but it would be nice to

[alloy]

In that case let’s defer it till v0.64.1

[alloy]

I’m going to wait with creating a v0.64.1 issue for now to not derail ongoing work, but please remember to bring this up for cherry-picking if I don't immediately remember.