Skip to content

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

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.

Sign up for GitHub

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

Closed
kelset wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9c642e9
new kudo patch
kelset Sep 16, 2022
e24f06b
Update sdks/hermes-engine/utils/build-apple-framework.sh
kelset Sep 20, 2022
2a3e5a7
Update sdks/hermes-engine/utils/build-apple-framework.sh
kelset Sep 20, 2022
2bbbe88
Update sdks/hermes-engine/utils/build-apple-framework.sh
kelset Sep 20, 2022
1b4f10e
Update sdks/hermes-engine/utils/build-ios-framework.sh
kelset Sep 20, 2022
ad1808b
Update sdks/hermes-engine/utils/build-mac-framework.sh
kelset Sep 20, 2022
aa2934f
fix: pick up variables from podspec if they are not define in env vars
Sep 22, 2022
2274a9e
fix: remove duplicated vars
Sep 23, 2022
7c41d4e
fix: Add support for JSI_PATH env var
Sep 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions sdks/hermes-engine/hermes-engine.podspec
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -66,18 +66,25 @@ Pod::Spec.new do |spec|
}

if source[:git] then
ENV['REACT_NATIVE_PATH'] = react_native_path
hermes_utils_path = "/sdks/hermes-engine/utils"

spec.prepare_command = <<-EOS
BUILD_TYPE=#{build_type.to_s.capitalize}
export BUILD_TYPE=#{build_type.to_s.capitalize}
export RELEASE_VERSION="#{version}"
export IOS_DEPLOYMENT_TARGET="#{spec.deployment_target('ios')}"
export MAC_DEPLOYMENT_TARGET="#{spec.deployment_target('osx')}"
export JSI_PATH="$REACT_NATIVE_PATH/ReactCommon/jsi"

# Set HERMES_OVERRIDE_HERMESC_PATH if pre-built HermesC is available
#{File.exist?(import_hermesc_file) ? "export HERMES_OVERRIDE_HERMESC_PATH=#{import_hermesc_file}" : ""}
#{File.exist?(import_hermesc_file) ? "echo \"Overriding HermesC path...\"" : ""}

# Build iOS framework
./utils/build-ios-framework.sh
$REACT_NATIVE_PATH#{hermes_utils_path}/build-ios-framework.sh

# Build Mac framework
./utils/build-mac-framework.sh
$REACT_NATIVE_PATH#{hermes_utils_path}/build-mac-framework.sh
EOS
end
end
18 changes: 14 additions & 4 deletions sdks/hermes-engine/utils/build-apple-framework.sh
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,18 +7,28 @@
NUM_CORES=$(sysctl -n hw.ncpu)
IMPORT_HERMESC_PATH=${HERMES_OVERRIDE_HERMESC_PATH:-$PWD/build_host_hermesc/ImportHermesc.cmake}
REACT_NATIVE_PATH=${REACT_NATIVE_PATH:-$PWD/../..}
JSI_PATH="$REACT_NATIVE_PATH/ReactCommon/jsi"
if [[ -z "$JSI_PATH" ]]; then
JSI_PATH="$REACT_NATIVE_PATH/ReactCommon/jsi"
fi

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

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

@cipolleschicipolleschiSep 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kelsetkelsetSep 20, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@KudoKudoSep 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

use_env_var_or_ruby_prop "${RELEASE_VERSION}" "version"
}

function get_ios_deployment_target {
ruby -rcocoapods-core -rjson -e "puts Pod::Specification.from_file('hermes-engine.podspec').deployment_target('ios')"
use_env_var_or_ruby_prop "${IOS_DEPLOYMENT_TARGET}" "deployment_target('ios')"
}

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

@cipolleschicipolleschiSep 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. Do we always have MAC_DEPLOYMENT_TARGET?

use_env_var_or_ruby_prop "${MAC_DEPLOYMENT_TARGET}" "deployment_target('osx')"
}

# Build host hermes compiler for internal bytecode
Expand Down
3 changes: 2 additions & 1 deletion sdks/hermes-engine/utils/build-ios-framework.sh
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -5,7 +5,8 @@
# LICENSE file in the root directory of this source tree.

# shellcheck source=xplat/js/react-native-github/sdks/hermes-engine/utils/build-apple-framework.sh
. ./utils/build-apple-framework.sh
CURR_SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)"
. "${CURR_SCRIPT_DIR}/build-apple-framework.sh"

if [ ! -d destroot/Library/Frameworks/universal/hermes.xcframework ]; then
ios_deployment_target=$(get_ios_deployment_target)
Expand Down
3 changes: 2 additions & 1 deletion sdks/hermes-engine/utils/build-mac-framework.sh
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -5,7 +5,8 @@
# LICENSE file in the root directory of this source tree.

# shellcheck source=xplat/js/react-native-github/sdks/hermes-engine/utils/build-apple-framework.sh
. ./utils/build-apple-framework.sh
CURR_SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)"
. "${CURR_SCRIPT_DIR}/build-apple-framework.sh"

if [ ! -d destroot/Library/Frameworks/macosx/hermes.framework ]; then
mac_deployment_target=$(get_mac_deployment_target)
Expand Down