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

Use the right logic to decide when we build Hermes from source #35469

Closed
wants to merge 1 commit into from

Conversation

cipolleschi
Copy link
Contributor

@cipolleschi cipolleschi commented Nov 25, 2022

Summary

This PR backports the changes in 0.71 to apply the same logic to decide when we have to build Hermes from source in both the hermes-engine.podspec
and in the react_native_pods.rb script.

Changelog

[iOS] [Fixed] - Use the right logic to decide when we have to build from source

Test Plan

Working on RC2

@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. p: Facebook Partner: Facebook Partner labels Nov 25, 2022
@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.

1 similar comment
@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 react-native-bot added Bug Platform: iOS iOS applications. labels Nov 25, 2022
@analysis-bot
Copy link

analysis-bot commented Nov 25, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,103,561 +0
android hermes armeabi-v7a 6,471,886 +0
android hermes x86 7,521,203 +0
android hermes x86_64 7,380,081 +0
android jsc arm64-v8a 8,968,399 +0
android jsc armeabi-v7a 7,699,505 +0
android jsc x86 9,030,706 +0
android jsc x86_64 9,508,749 +0

Base commit: c4959b9
Branch: main

@dmytrorykun
Copy link
Contributor

is_building_hermes_from_source in this PR looks different from the on in 49c0267

@cipolleschi cipolleschi force-pushed the cipolleschi/backport_hermes_logic branch from fc5c8e1 to 9fa35ba Compare November 25, 2022 11:24
@cipolleschi
Copy link
Contributor Author

PR updated. The

    if ENV['HERMES_ENGINE_TARBALL_PATH'] != nil
        return false
    end

Is required to avoid to build from source even when the tarball is passed.

I had a merge conflict which has been resolved wrongly, applying the wrong return statement. Luckily, the ruby tests were failing! :D

Thanks for spotting this!

@cipolleschi cipolleschi force-pushed the cipolleschi/backport_hermes_logic branch 2 times, most recently from 8ae8c60 to e6d154b Compare November 25, 2022 11:40
@cipolleschi cipolleschi force-pushed the cipolleschi/backport_hermes_logic branch from e6d154b to ee51c07 Compare November 25, 2022 11:40
@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.

1 similar comment
@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.

@analysis-bot
Copy link

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

Base commit: c4959b9
Branch: main

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @cipolleschi in 67d0264.

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 Nov 25, 2022
@cipolleschi cipolleschi deleted the cipolleschi/backport_hermes_logic branch March 27, 2023 13:26
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…ook#35469)

Summary:
This PR backports the changes in 0.71 to apply the same logic to decide when we have to build Hermes from source in both the `hermes-engine.podspec`
and in the `react_native_pods.rb` script.

## Changelog

[iOS] [Fixed] - Use the right logic to decide when we have to build from source

Pull Request resolved: facebook#35469

Test Plan: Working on RC2

Reviewed By: cortinico

Differential Revision: D41529539

Pulled By: cipolleschi

fbshipit-source-id: 879522c2187df28f40f6bc699057e9ecfb7e25fb
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. Merged This PR has been merged. p: Facebook Partner: Facebook Partner Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants