iOS: don't override EXCLUDED_ARCHS when installing Hermes

[jpdriver]

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 (iOS: only exclude i386 while using hermes #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

    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:

[IOS] [FIXED] don't override EXCLUDED_ARCHS when installing Hermes

Test Plan:

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,948,041	-15,919
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	9,541,703	-12,604
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 10a076f
Branch: main

[cipolleschi]

Hi @jpdriver! Thank you so much for this update. I think that is very good.

However, there are a couple of tests that are failing and need to be updated:

• https://github.com/facebook/react-native/blob/main/packages/react-native/scripts/cocoapods/__tests__/utils-test.rb#L223
• https://github.com/facebook/react-native/blob/main/packages/react-native/scripts/cocoapods/__tests__/utils-test.rb#L242

Can you take care of it?

[cipolleschi]

Amazing job, thank you so much for adding some tests on top of these changes.
I left some suggestions to improve maintainability and readibility.

[jpdriver]

@cipolleschi changed as requested 🙇🏻‍♂️

note that with the early returns in place these changes mean we will not set EXCLUDED_ARCHS at all if you are not using Hermes.

existing behaviour is we align all projects to use a blank string. (see changes to tests for more detail)

-            assert_equal(config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"], "")
+            assert_equal(config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"], nil)

so not sure if this now constitutes a "breaking" change or not? 😅

[cipolleschi]

I think it is better this way, honestly.
I don't like that we manipulate the settings when it is not needed and I guess that empty string or nil are treated the same way by xcodebuild

[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 merged this pull request in f2447e6.