fix(hermes): change logic in build scripts for Apple to use the right version

[kelset]

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.

Read more here.

This PR is taking @Kudo's proposed patch here - props for the fix go to him.

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]

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

[facebook-github-bot]

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

[cipolleschi]

Same as above. Do we always have MAC_DEPLOYMENT_TARGET?

[cipolleschi]

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

[kelset]

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

[Kudo]

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,

function get_release_version {
  if [[ -n "${RELEASE_VERSION}" ]]; then
    echo "${RELEASE_VERSION}"
  else
    ruby -rcocoapods-core -rjson -e "puts Pod::Specification.from_file('hermes-engine.podspec').deployment_target('ios')"
  fi
}

or

function get_release_version {
  if [[ -z "${RELEASE_VERSION}" ]]; then
    echo "RELEASE_VERSION is not defined."
    exit 1
  fi
  echo "${RELEASE_VERSION}"
}

please feel free to change the code that makes sense to you.

[facebook-github-bot]

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

[cipolleschi]

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

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

[Kudo]

nit changes 😅

[cipolleschi]

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

[facebook-github-bot]

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

[facebook-github-bot]

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

[facebook-github-bot]

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

[analysis-bot]

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

Base commit: 5d2a400
Branch: main

[analysis-bot]

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]

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

[Kudo]

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.

[cipolleschi]

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

[Kudo]

it looks good now, thanks @cipolleschi!

[facebook-github-bot]

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

[facebook-github-bot]

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

[react-native-bot]

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

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