Skip to content

iOS: only exclude i386 while using hermes - #38132

Closed
jpdriver wants to merge 1 commit into
react:mainfrom
jpdriver:skip-excluded-archs
Closed

iOS: only exclude i386 while using hermes#38132
jpdriver wants to merge 1 commit into
react:mainfrom
jpdriver:skip-excluded-archs

Conversation

@jpdriver

Copy link
Copy Markdown
Contributor

Summary:

Existing Behaviour

 def self.exclude_i386_architecture_while_using_hermes(installer)
projects = self.extract_projects(installer)
# Hermes does not support `i386` architecture
excluded_archs_default = self.has_pod(installer, 'hermes-engine') ? "i386" : ""
projects.each do |project|
project.build_configurations.each do |config|
config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"] = excluded_archs_default
end
project.save()
end
end

🐛 However 🐛

  • this function is currently called regardless of whether or not you are actually using Hermes
  • this means existing projects that have EXCLUDED_ARCHS set, the existing value will be changed either to "i386" or a blank string

Changed Behaviour

  • moved the function call inside an if ReactNativePodsUtils.has_pod(installer, "React-hermes") block
  • EXCLUDED_ARCHS should only be modified if needed now

Changelog:

[IOS] [FIXED] Only modify EXCLUDED_ARCHS when needed for Hermes

Test Plan:

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 29, 2023
@analysis-bot

analysis-bot commented Jun 29, 2023

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a9,060,768-2
androidhermesarmeabi-v7a8,310,099+2
androidhermesx869,576,980+0
androidhermesx86_649,419,403-1
androidjscarm64-v8a9,613,486+1
androidjscarmeabi-v7a8,740,120+2
androidjscx869,700,430-2
androidjscx86_649,946,999+0

Base commit: ebc1fe6
Branch: main

@cipolleschicipolleschi left a comment

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.

Thank you so much for doing this. I think it slipped away while refactoring the cocoapods scripts. 🤦

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@cipolleschi

Copy link
Copy Markdown
Contributor

/rebase

@github-actions
github-actionsBotforce-pushed the skip-excluded-archs branch from d3f09cd to 9e397afCompareJuly 3, 2023 12:22
@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@cipolleschi

Copy link
Copy Markdown
Contributor

/rebase

@github-actions
github-actionsBotforce-pushed the skip-excluded-archs branch from 9e397af to b436111CompareJuly 4, 2023 08:35
@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@github-actions

Copy link
Copy Markdown

This pull request was successfully merged by @jpdriver in ee1cd13.

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

@github-actionsgithub-actionsBot added the Merged This PR has been merged. label Jul 4, 2023
facebook-github-bot pushed a commit that referenced this pull request Aug 29, 2023
Summary:
- if an existing iOS project specifies `EXCLUDED_ARCHS`, these settings are overwritten by the `react_native_post_install` step as part of the Cocoapods install
- this can break the build of existing iOS apps that want to include React Native
- a previous PR (#38132) updated the Cocoapods utils to that we only update `EXCLUDED_ARCHS` when using Hermes
- this worked around the issue for apps that opted to use JSC
- however if you _do_ want to use Hermes (the default) this problem persists
### Existing Behaviour
- one of the functions called as part of the `react_native_post_install` step is `exclude_i386_architecture_while_using_hermes`
- see [/packages/react-native/scripts/cocoapods/utils.rb](https://github.com/facebook/react-native/blob/v0.72.1/packages/react-native/scripts/cocoapods/utils.rb#L56-L69)
```
def self.exclude_i386_architecture_while_using_hermes(installer)
projects = self.extract_projects(installer)
# Hermes does not support `i386` architecture
excluded_archs_default = self.has_pod(installer, 'hermes-engine') ? "i386" : ""
projects.each do |project|
project.build_configurations.each do |config|
config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"] = excluded_archs_default
end
project.save()
end
end
```
🐛 **However** 🐛
- this means existing projects that have `EXCLUDED_ARCHS` set, the existing value will be overwritten either to `"i386"` or a blank string
### Changed Behaviour
- appends `"i386"` to existing string if set, or just sets the value to `"i386"` if there is no existing value
## Changelog:
<!-- Help reviewers and the release process by writing your own changelog entry.
Pick one each for the category and type tags:
[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message
For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->
[IOS] [FIXED] don't override `EXCLUDED_ARCHS` when installing Hermes
Pull Request resolved: #39060
Reviewed By: dmytrorykun
Differential Revision: D48515441
Pulled By: cipolleschi
fbshipit-source-id: 8cb3c8b680d92272da0b106553179af051d0f84e
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

react_native_post_install set main project EXCLUDED_ARCHS[sdk=iphonesimulator*] to empty string in iOS

4 participants

@jpdriver@analysis-bot@facebook-github-bot@cipolleschi