Skip to content

Conversation

@jpdriver
Copy link
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-bot facebook-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
Copy link

analysis-bot commented Jun 29, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,060,768 -2
android hermes armeabi-v7a 8,310,099 +2
android hermes x86 9,576,980 +0
android hermes x86_64 9,419,403 -1
android jsc arm64-v8a 9,613,486 +1
android jsc armeabi-v7a 8,740,120 +2
android jsc x86 9,700,430 -2
android jsc x86_64 9,946,999 +0

Base commit: ebc1fe6
Branch: main

Copy link
Contributor

@cipolleschi cipolleschi left a comment

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
Contributor

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

@cipolleschi
Copy link
Contributor

/rebase

@github-actions github-actions bot force-pushed the skip-excluded-archs branch from d3f09cd to 9e397af Compare July 3, 2023 12:22
@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.

@cipolleschi
Copy link
Contributor

/rebase

@github-actions github-actions bot force-pushed the skip-excluded-archs branch from 9e397af to b436111 Compare July 4, 2023 08:35
@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.

@github-actions
Copy link

github-actions bot commented Jul 4, 2023

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

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

@github-actions github-actions bot 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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

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