Mac Catalyst patches #34026

Arkkeeper · 2022-06-18T18:26:20Z

Summary

This PR adds a new method called __apply_mac_catalyst_patches to scripts/react_native_pods.rb. If it is enabled in the Podfile, it will apply three patches necessary for successful building not only for iOS and tvOS targets, but also for macOS using Apple's Mac Catalyst technology.

These 3 patches are:

Fixing bundle signing issues by altering CODE_SIGN_IDENTITY
Explicitly setting dead code stripping flag in project.pbxproj
Modifying library search paths

The details were discussed here reactwg/react-native-releases#21 (comment)

Changelog

[iOS] [Added] - Add Mac Catalyst compatibility (can be enabled in Podfile)

Test Plan

Go to project settings in Xcode, to General tab. Enable "iPad" and "Mac Catalyst" checkboxes
Go to "Signing & Capabilities" tab, ensure that a correct bundle id and development team are set
Edit Podfile, uncomment __apply_mac_catalyst_patches(installer) line
Run pod install in ios directory
Get back to Xcode, select "My Mac (Mac Catalyst)" as a target device
Build & run

analysis-bot · 2022-06-18T18:56:39Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,830,116	-232
android	hermes	armeabi-v7a	7,216,498	-208
android	hermes	x86	8,139,019	-258
android	hermes	x86_64	8,120,749	-269
android	jsc	arm64-v8a	9,695,700	-244
android	jsc	armeabi-v7a	8,451,163	-196
android	jsc	x86	9,646,317	-255
android	jsc	x86_64	10,244,130	-274

Base commit: d6c08bd
Branch: main

mikehardy

These look like exactly the compromise discussed in the road to 0.69 discussion - make it easy for people to do, but not enabled by default since side effects may break non-catalyst folks. Looks great to me from that perspective and is technically what I've been doing to get my catalyst builds to work. Nice!

analysis-bot · 2022-06-18T20:55:39Z

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

Base commit: d6c08bd
Branch: main

mikehardy · 2022-06-20T17:59:31Z

Hey @Arkkeeper - I followed some curiosity in a related thread and there may be a smaller / more minimal intervention for the DEAD_CODE_STRIPPING portion of this fix. You might try it, it appears to work based on success reports

#32016 (comment)

With apologies, I dug it up but have not tried it myself yet for the macCatalyst case...

cortinico · 2022-06-20T18:35:18Z

Looks great to me from that perspective and is technically what I've been doing to get my catalyst builds to work. Nice!

Code looks good to me. Thanks @mikehardy for cross-checking it.
I'd like @cipolleschi to pick this up and drive this forward.

What I can say on the code itself, is that we've been moving a lot of our ruby code from scripts/react_native_pods.rb to scripts/cocoapods (like this PR: #33982).
So ideally having this code added directly there + tests would be preferrable. You can find other tests there that can be used as inspiration.

Arkkeeper · 2022-06-22T09:49:35Z

@cortinico thanks for your comment, I've modified this PR according to it

mikehardy

Now looks even better to me :-)

facebook-github-bot · 2022-06-22T22:25:10Z

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

cipolleschi

Hi @Arkkeeper, thanks a lot for your PR. Great work and thanks to the effort of supporting also Catalyst!

I apologise for the delay, but I was on PTO last week.

I left some comments and I suggested some changes to improve the code structure and the testing. Let me know what do you think about them!

cipolleschi · 2022-06-27T08:43:22Z

scripts/cocoapods/__tests__/utils-test.rb

+        firstTarget = prepare_target("FirstTarget")
+        secondTarget = prepare_target("SecondTarget")
+        thirdTarget = prepare_target("ThirdTarget")


Suggested change

firstTarget = prepare_target("FirstTarget")

secondTarget = prepare_target("SecondTarget")

thirdTarget = prepare_target("ThirdTarget")

first_target = prepare_target("FirstTarget")

second_target = prepare_target("SecondTarget")

third_target = prepare_target("ThirdTarget")

[nit] let's stick to the snake_case for everything in Ruby. It is a personal preference, but mixing the cases makes it harder to collaborate.

That's also regards test_fixLibrarySearchPaths_correctlySetsTheSearchPathsForAllProjects. I've renamed both of them.

cipolleschi · 2022-06-27T08:48:27Z

scripts/cocoapods/__tests__/utils-test.rb

+            if target.respond_to?(:product_type) and target.product_type == "com.apple.product-type.bundle"
+                target.build_configurations.each do |config|
+                    assert_equal(config.build_settings["CODE_SIGN_IDENTITY[sdk=macosx*]"], "-")


These lines are replicating the logic of the fix. It is not really testing that the code executes correctly. For example, this test will pass also when "CODE_SIGN_IDENTITY[sdk=macosx*]" is set to "-" for all the target.

Can I suggest a different approach?

We already have the references to all the the targets (firstTarget, secondTarget, thirdTarget). Why don't we assert the targets directly?

Another thing that I don't think is correct: TargetMock does not respond to :product_type.
The TargetMock class should be enhanced to have this property and the property must be set accordingly.

I'm pretty sure that this assert is never really executed. Could you double check that, please?

We are still executing the same logic in the tests.
How do you feel about asserting something like this:

first_target.build_configurations.each do |config| assert_nil(config.build_settings["CODE_SIGN_IDENTITY[sdk=macosx*]"]) end second_target.build_configurations.each do |config| assert_nil(config.build_settings["CODE_SIGN_IDENTITY[sdk=macosx*]"]) end third_target.build_configurations.each do |config| assert_equal(assert_equal(config.build_settings["CODE_SIGN_IDENTITY[sdk=macosx*]"], "-")) end

?

In this way we don't replicate the logic but we assert on the expected outcome of the cod execution.

cipolleschi · 2022-06-27T08:53:49Z

scripts/cocoapods/utils.rb

+                    config.build_settings['LIBRARY_SEARCH_PATHS'] = ['$(SDKROOT)/usr/lib/swift', '$(SDKROOT)/System/iOSSupport/usr/lib/swift', '$(inherited)']
+                end  
+            end
+            aggregate_target.user_project.save  


[nit] Perhaps it is not idiomatic, but could we use save()? We use the () everywhere else and it is better to be consistent.

Also, our internal linter says that there are trailing whitespaces at this line. Can you remove them?

cipolleschi · 2022-06-27T09:00:23Z

template/ios/Podfile

@@ -33,5 +33,7 @@ target 'HelloWorld' do
  post_install do |installer|
    react_native_post_install(installer)
    __apply_Xcode_12_5_M1_post_install_workaround(installer)
+    # Enable the next line in order to apply patches necessary for Mac Catalyst builds
+    #__apply_mac_catalyst_patches(installer)


I'll move the invocation of __apply_mac_catalyst_patches(installer) inside the react_native_post_install, actually removing the __apply_mac_catalyst_patches. Given that not all the projects need these, we can update the react_native_post_install function to accept a boolean apply_catalyst_patch with a default value of false.

The rationale here is that:

it helps discoverability. At the end of the refactoring effort, the react_native_pods script will have only two function: use_react_native and react_native_post_install. These will be documented and people will know about the existence of this flag. Otherwise, they have to read the whole script file to learn bout its existence.

it makes it easier for us to maintain the script. When, in the future, we will have to remove it, we could just delete all the code, and deprecate the flag. No other change will be required by the apps for a while, until we actually remove the flag.

Note that this change may foce you to also update the RNTester app.

How do you feel about this? Does it make sense?

cipolleschi · 2022-06-27T09:02:42Z

scripts/cocoapods/__tests__/utils-test.rb

+                end
+            end
+        end
+


Our internal linter says that there are trailing whitespaces at this line. Can you remove them?

cipolleschi · 2022-06-27T09:03:10Z

scripts/cocoapods/__tests__/utils-test.rb

+        end
+
+        assert_equal(user_project_mock.save_invocation_count, 1)
+    end    


Our internal linter says that there are trailing whitespaces at this line. Can you remove them?

cipolleschi · 2022-06-27T09:03:22Z

scripts/cocoapods/utils.rb

+                    config.build_settings['PRESERVE_DEAD_CODE_INITS_AND_TERMS'] = 'YES'
+                    # Modify library search paths
+                    config.build_settings['LIBRARY_SEARCH_PATHS'] = ['$(SDKROOT)/usr/lib/swift', '$(SDKROOT)/System/iOSSupport/usr/lib/swift', '$(inherited)']
+                end  


Our internal linter says that there are trailing whitespaces at this line. Can you remove them?

Arkkeeper · 2022-06-29T15:34:37Z

@cipolleschi thank you for your proposals, I've tried to implement them all.

cortinico · 2022-06-29T15:59:32Z

Could you resolve the conflict on packages/rn-tester/Podfile @Arkkeeper ?

cortinico · 2022-06-29T20:39:32Z

There seems to be an issue with the CI:

undefined local variable or method `mac_catalyst_enabled' for #Pod::Podfile:0x00007fe38f92e990

facebook-github-bot · 2022-06-30T09:00:51Z

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

cipolleschi · 2022-06-30T09:07:21Z

packages/rn-tester/Podfile

@@ -21,6 +21,8 @@ def pods(options = {}, use_flipper: false)
  project 'RNTesterPods.xcodeproj'

  fabric_enabled = true
+  mac_catalyst_enabled = false


This line is not required! ;)

cipolleschi · 2022-06-30T09:10:09Z

packages/rn-tester/Podfile

@@ -66,6 +68,7 @@ target 'RNTesterIntegrationTests' do
 end

 post_install do |installer|
-  react_native_post_install(installer, @prefix_path)
+  mac_catalyst_enabled = false
+  react_native_post_install(installer, mac_catalyst_enabled, @prefix_path)


Instead of defining a variable, which I think is done just to have the name of the parameter for a code-as-documentation purpose, why don't we use named parameters from Ruby?

We can define a function with this syntax:

def foo(named_param: false) end

and then invoke the function with this syntax:

foo(:named_param => true)

with this, we can emove the variable declaration and update this code as it follows:

Suggested change

react_native_post_install(installer, mac_catalyst_enabled, @prefix_path)

react_native_post_install(installer, @prefix_path, :mac_catalyst_enabled => false)

cipolleschi · 2022-06-30T09:15:45Z

scripts/cocoapods/__tests__/utils-test.rb

+            if target.respond_to?(:product_type) and target.product_type == "com.apple.product-type.bundle"
+                target.build_configurations.each do |config|
+                    assert_equal(config.build_settings["CODE_SIGN_IDENTITY[sdk=macosx*]"], "-")


We are still executing the same logic in the tests.
How do you feel about asserting something like this:

first_target.build_configurations.each do |config| assert_nil(config.build_settings["CODE_SIGN_IDENTITY[sdk=macosx*]"]) end second_target.build_configurations.each do |config| assert_nil(config.build_settings["CODE_SIGN_IDENTITY[sdk=macosx*]"]) end third_target.build_configurations.each do |config| assert_equal(assert_equal(config.build_settings["CODE_SIGN_IDENTITY[sdk=macosx*]"], "-")) end

?

In this way we don't replicate the logic but we assert on the expected outcome of the cod execution.

cipolleschi · 2022-06-30T09:18:20Z

scripts/react_native_pods.rb

+  if mac_catalyst_enabled
+    ReactNativePodsUtils.apply_mac_catalyst_patches(installer)
+  end


[nit] We can use a conditional modifier here:

Suggested change

if mac_catalyst_enabled

ReactNativePodsUtils.apply_mac_catalyst_patches(installer)

end

ReactNativePodsUtils.apply_mac_catalyst_patches(installer) if mac_catalyst_enabled

I assume, the double assert_equal in third_target.build_configurations.each block is redundant, right?

Yeah, sorry... bad copy-and-paste... 🤦

cipolleschi · 2022-06-30T09:19:25Z

scripts/react_native_pods.rb

@@ -141,7 +141,11 @@ def use_flipper!(versions = {}, configurations: ['Debug'])
  use_flipper_pods(versions, :configurations => configurations)
 end

-def react_native_post_install(installer, react_native_path = "../node_modules/react-native")
+def react_native_post_install(installer, mac_catalyst_enabled, react_native_path = "../node_modules/react-native")


I would use a named parameter here, to have self-documenting code. What do you think?

Suggested change

def react_native_post_install(installer, mac_catalyst_enabled, react_native_path = "../node_modules/react-native")

def react_native_post_install(installer, react_native_path = "../node_modules/react-native", mac_catalyst_enabled: false)

cipolleschi · 2022-06-30T09:19:49Z

template/ios/Podfile

+    mac_catalyst_enabled = false
+
+    react_native_post_install(installer, mac_catalyst_enabled)


If we use the named parameter approach, we can avoid declaring the variable! ;)

cipolleschi · 2022-06-30T09:21:00Z

Great job! 👏 I did another round of review, leaving also a couple of nitpicks, if you want to use them. I think that what we actually need to fix are the tests, because at the moment we are testing the code by knowing how it executes instead of trying to assert the final state of the system. What do you think?

Arkkeeper · 2022-06-30T09:56:03Z

@cipolleschi thank you again for your comments and patience, I've commited the proposed edits.

cipolleschi · 2022-06-30T10:00:58Z

Thasnks a lot for the effort! 👏 Amazing job! :D

facebook-github-bot · 2022-06-30T10:45:54Z

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

react-native-bot · 2022-06-30T13:00:03Z

This pull request was successfully merged by @Arkkeeper in 2fb6a33.

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

mikehardy · 2022-08-19T16:48:42Z

A bit of a comment thread resurrection here but I just want to mention that I just got around to integrating this mac_catalyst_enabled flag in my test script, and it works great, I was able to eliminate most of the podfile workarounds I had - quite nice

Only thing I can think of now is that it may make sense to also integrate one more build setting, but it's also an obvious one for developers. I do it like so in my script

pbxproj flag ios/rnfbdemo.xcodeproj --target rnfbdemo SUPPORTS_MACCATALYST YES

Mac Catalyst patches

c6949ee

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jun 18, 2022

react-native-bot added Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Jun 18, 2022

mikehardy approved these changes Jun 18, 2022

View reviewed changes

kelset requested review from cortinico and cipolleschi June 20, 2022 09:21

Mac Catalyst Patches moved to cocoapods/utils.rb

e04f353

mikehardy approved these changes Jun 22, 2022

View reviewed changes

cipolleschi requested changes Jun 27, 2022

View reviewed changes

cipolleschi reviewed Jun 27, 2022

View reviewed changes

Arkkeeper added 2 commits June 29, 2022 18:28

mac_catalyst_enabled flag

6b261b8

Mac Catalyst tests fixed

2e163cd

Arkkeeper requested a review from cipolleschi June 29, 2022 15:35

cortinico closed this Jun 29, 2022

cortinico reopened this Jun 29, 2022

cortinico closed this Jun 29, 2022

cortinico reopened this Jun 29, 2022

Merge branch 'main' into main

5298910

Arkkeeper added 2 commits June 30, 2022 10:19

Fix for rn-tester Podfile

e56e5c6

Merge branch 'main' of https://github.com/Arkkeeper/react-native

81fa1ce

cipolleschi requested changes Jun 30, 2022

View reviewed changes

Improvements for Mac Catalyst patches & tests

6b6ed3f

cipolleschi approved these changes Jun 30, 2022

View reviewed changes

facebook-github-bot closed this in 2fb6a33 Jun 30, 2022

react-native-bot added the Merged This PR has been merged. label Jun 30, 2022

mikehardy mentioned this pull request Sep 2, 2022

React Native macCatalyst mikehardy/rnfbdemo#14

Open

	react_native_post_install(installer, mac_catalyst_enabled, @prefix_path)
	react_native_post_install(installer, @prefix_path, :mac_catalyst_enabled => false)

	def react_native_post_install(installer, mac_catalyst_enabled, react_native_path = "../node_modules/react-native")
	def react_native_post_install(installer, react_native_path = "../node_modules/react-native", mac_catalyst_enabled: false)

		mac_catalyst_enabled = false

		react_native_post_install(installer, mac_catalyst_enabled)

Mac Catalyst patches #34026

Mac Catalyst patches #34026

Conversation

Arkkeeper commented Jun 18, 2022

Summary

Changelog

Test Plan

analysis-bot commented Jun 18, 2022 • edited Loading

mikehardy left a comment

Choose a reason for hiding this comment

analysis-bot commented Jun 18, 2022 • edited Loading

mikehardy commented Jun 20, 2022

cortinico commented Jun 20, 2022

Arkkeeper commented Jun 22, 2022 • edited Loading

mikehardy left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 22, 2022

cipolleschi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipolleschi Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arkkeeper commented Jun 29, 2022

cortinico commented Jun 29, 2022

cortinico commented Jun 29, 2022

facebook-github-bot commented Jun 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipolleschi commented Jun 30, 2022 • edited Loading

Arkkeeper commented Jun 30, 2022

cipolleschi commented Jun 30, 2022

facebook-github-bot commented Jun 30, 2022

react-native-bot commented Jun 30, 2022

mikehardy commented Aug 19, 2022

analysis-bot commented Jun 18, 2022 •

edited

Loading

analysis-bot commented Jun 18, 2022 •

edited

Loading

Arkkeeper commented Jun 22, 2022 •

edited

Loading

cipolleschi Jun 27, 2022 •

edited

Loading

cipolleschi commented Jun 30, 2022 •

edited

Loading