fix(hermes): change logic in build scripts for Apple to use the right version by kelset · Pull Request #34710 · react/react-native

kelset · 2022-09-16T14:31:34Z

Summary

Within the hermes-engine.podspec contained in the RN repo (at react-native/main/sdks/hermes-engine/), there's a bit of logic that triggers ./utils/build-ios-framework.sh and ./utils/build-mac-framework.sh .

The issue is that we all thought that that ./utils/build-ios-framework.sh would invoke the React native version of the scripts (since the podspec file lives right next to the utils folder) but, in reality, it doesn't. It just so happens that the Hermes repo has a root level utils folder which is (you guessed it) where the Hermes variation of those build scripts live.
So, when running the pod install command in a react-native project (build from source), it will go and download the hermes source code (since the source[:git] gets set) but then it will use the hermes variation of the build-*.sh scripts.

Changelog

[iOS] [Fixed] - Change hermes logic in build scripts for Apple to use the correct files

Test Plan

Tested by @Kudo in his work, and in my PR locally - see here.

cortinico · 2022-09-16T14:33:17Z

Code looks fine on my end 👍 @cipolleschi has more context than me on this change

facebook-github-bot · 2022-09-20T07:31:32Z

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

cipolleschi · 2022-09-20T09:57:58Z

 }

 function get_mac_deployment_target {
-ruby -rcocoapods-core -rjson -e "puts Pod::Specification.from_file('hermes-engine.podspec').deployment_target('osx')"


Same as above. Do we always have MAC_DEPLOYMENT_TARGET?

cipolleschi · 2022-09-20T10:02:50Z

 JSI_PATH="$REACT_NATIVE_PATH/ReactCommon/jsi"

 function get_release_version {
-ruby -rcocoapods-core -rjson -e "puts Pod::Specification.from_file('hermes-engine.podspec').version"


Are we sure that in CI, where we invoke the build-ios-framework and the build-mac-framework we have the RELEASE_VERSION, IOS_DEPLOYMENT_TARGET and MAC_DEPLOYMENT_TARGET properly set?
In this case we are not going through the hermes-engine.podspec, so it could be that these variables are not set...

good catch, I guess we need to modify the CircleCI config to set them up too, right?

thanks for following up the change. when i first proposed the change, i just want to keep it shorter. ideally we could add some if/else check.
for example,
functionget_release_version{if[[ -n"${RELEASE_VERSION}"]];thenecho"${RELEASE_VERSION}"elseruby -rcocoapods-core -rjson -e"puts Pod::Specification.from_file('hermes-engine.podspec').deployment_target('ios')"fi}
or
functionget_release_version{if[[ -z"${RELEASE_VERSION}"]];thenecho"RELEASE_VERSION is not defined."exit1fiecho"${RELEASE_VERSION}"}
please feel free to change the code that makes sense to you.

facebook-github-bot · 2022-09-22T11:10:29Z

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

cipolleschi · 2022-09-22T13:10:41Z

@kelset @Kudo I updated the script with your suggestions (and a bit of refactoring). Can you give it a shot and see if it works properly for you (hoping that the CI does not break!)

facebook-github-bot · 2022-09-22T13:25:46Z

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

Kudo

nit changes 😅

cipolleschi · 2022-09-22T16:18:37Z

Thanks @Kudo, I should have fixed those! :D

facebook-github-bot · 2022-09-23T10:13:29Z

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

facebook-github-bot · 2022-09-23T10:16:14Z

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

facebook-github-bot · 2022-09-23T10:38:40Z

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

analysis-bot · 2022-09-23T10:44:05Z

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

Base commit: 5d2a400
Branch: main

analysis-bot · 2022-09-23T10:44:41Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,725,527	+0
android	hermes	armeabi-v7a	7,128,411	+0
android	hermes	x86	8,032,083	+0
android	hermes	x86_64	8,005,366	+0
android	jsc	arm64-v8a	9,595,873	+0
android	jsc	armeabi-v7a	8,362,168	+0
android	jsc	x86	9,540,083	+0
android	jsc	x86_64	10,132,493	+0

Base commit: 5d2a400
Branch: main

facebook-github-bot · 2022-09-23T18:29:37Z

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

Kudo · 2022-09-24T06:22:50Z

could you double confirm the generated Pods/Local Podspecs/hermes-engine.podspec.json doesn't contain the absolute path? in my case, it will have /Users/kudo, e.g.
"prepare_command": "export BUILD_TYPE=Debug\nexport RELEASE_VERSION=\"1000.0.0\"\nexport IOS_DEPLOYMENT_TARGET=\"12.4\"\nexport MAC_DEPLOYMENT_TARGET=\"10.13\"\nexport JSI_PATH=\"/Users/kudo/react-native/sdks/hermes/../../ReactCommon/jsi\"\n\n# Set HERMES_OVERRIDE_HERMESC_PATH if pre-built HermesC is available\n\n\n\n# Build iOS framework\n/Users/kudo/react-native/sdks/hermes/../../sdks/hermes-engine/utils/build-ios-framework.sh\n\n# Build Mac framework\n/Users/kudo/react-native/sdks/hermes/../../sdks/hermes-engine/utils/build-mac-framework.sh"
that would make the checksum in Podfile.lock unstable between each environments. that's why previously i proposed environment variables approach.

You are right! I have full path too... I'm updating the PR to use the env variable.

it looks good now, thanks @cipolleschi!

Co-authored-by: Riccardo <cipolleschi@fb.com>

facebook-github-bot · 2022-09-24T10:15:34Z

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

facebook-github-bot · 2022-09-24T10:37:19Z

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

react-native-bot · 2022-09-26T09:10:04Z

This pull request was successfully merged by @kelset in cc13b02.

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

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: Microsoft Partner: Microsoft Partner labels Sep 16, 2022

kelset requested review from cipolleschi and cortinico September 16, 2022 14:31

kelset added Tool: CocoaPods Tech: Hermes Hermes Engine: https://hermesengine.dev/ labels Sep 16, 2022

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 Sep 16, 2022

kelset mentioned this pull request Sep 16, 2022
infra(e2e): rework local E2E script #34513
Closed

react-native-bot added Bug Platform: iOS iOS applications. labels Sep 16, 2022

cipolleschi requested changes Sep 20, 2022
View reviewed changes

Kudo reviewed Sep 22, 2022
View reviewed changes

Comment threadsdks/hermes-engine/utils/build-apple-framework.sh Outdated
Comment threadsdks/hermes-engine/utils/build-apple-framework.sh Outdated

cipolleschi force-pushed the kelset/fix-hermes-build-scripts branch from 1ed907f to 33e19a9Compare September 22, 2022 16:12

cipolleschi force-pushed the kelset/fix-hermes-build-scripts branch 3 times, most recently from db71a89 to d7a4c14Compare September 23, 2022 18:24

cipolleschi force-pushed the kelset/fix-hermes-build-scripts branch from d7a4c14 to 39f4cbaCompare September 23, 2022 19:07

Kudo reviewed Sep 24, 2022
View reviewed changes

cipolleschi force-pushed the kelset/fix-hermes-build-scripts branch from 39f4cba to 72e3ebeCompare September 24, 2022 10:10

kelsetand others added 8 commits September 24, 2022 11:10

new kudo patch
9c642e9

Update sdks/hermes-engine/utils/build-apple-framework.sh
e24f06b
Co-authored-by: Riccardo <cipolleschi@fb.com>

Update sdks/hermes-engine/utils/build-apple-framework.sh
2a3e5a7
Co-authored-by: Riccardo <cipolleschi@fb.com>

Update sdks/hermes-engine/utils/build-apple-framework.sh
2bbbe88
Co-authored-by: Riccardo <cipolleschi@fb.com>

Update sdks/hermes-engine/utils/build-ios-framework.sh
1b4f10e
Co-authored-by: Riccardo <cipolleschi@fb.com>

Update sdks/hermes-engine/utils/build-mac-framework.sh
ad1808b
Co-authored-by: Riccardo <cipolleschi@fb.com>

fix: pick up variables from podspec if they are not define in env vars
aa2934f

fix: remove duplicated vars
2274a9e

cipolleschi force-pushed the kelset/fix-hermes-build-scripts branch 3 times, most recently from 3683fa7 to 37985a2Compare September 24, 2022 10:14

fix: Add support for JSI_PATH env var
7c41d4e

cipolleschi force-pushed the kelset/fix-hermes-build-scripts branch from 37985a2 to 7c41d4eCompare September 24, 2022 10:36

facebook-github-bot closed this in cc13b02Sep 26, 2022

react-native-bot added the Merged This PR has been merged. label Sep 26, 2022

kelset deleted the kelset/fix-hermes-build-scripts branch September 26, 2022 09:23

Uh oh!

Conversation

kelset commented Sep 16, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changelog

Test Plan

Uh oh!

cortinico commented Sep 16, 2022

Uh oh!

facebook-github-bot commented Sep 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cipolleschiSep 20, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cipolleschiSep 20, 2022

Choose a reason for hiding this comment

Uh oh!

kelsetSep 20, 2022

Choose a reason for hiding this comment

Uh oh!

KudoSep 20, 2022

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 22, 2022

Uh oh!

cipolleschi commented Sep 22, 2022

Uh oh!

facebook-github-bot commented Sep 22, 2022

Uh oh!

Kudo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cipolleschi commented Sep 22, 2022

Uh oh!

facebook-github-bot commented Sep 23, 2022

Uh oh!

facebook-github-bot commented Sep 23, 2022

Uh oh!

facebook-github-bot commented Sep 23, 2022

Uh oh!

analysis-bot commented Sep 23, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analysis-bot commented Sep 23, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Sep 23, 2022

Uh oh!

KudoSep 24, 2022

Choose a reason for hiding this comment

Uh oh!

cipolleschiSep 24, 2022

Choose a reason for hiding this comment

Uh oh!

KudoSep 25, 2022

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 24, 2022

Uh oh!

facebook-github-bot commented Sep 24, 2022

Uh oh!

react-native-bot commented Sep 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kelset commented Sep 16, 2022•
edited
Loading

analysis-bot commented Sep 23, 2022•
edited
Loading

analysis-bot commented Sep 23, 2022•
edited
Loading