Mac Catalyst patches by Arkkeeper · Pull Request #34026 · react/react-native

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

+ 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

+ 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.eachdo |config| assert_nil(config.build_settings["CODE_SIGN_IDENTITY[sdk=macosx*]"])endsecond_target.build_configurations.eachdo |config| assert_nil(config.build_settings["CODE_SIGN_IDENTITY[sdk=macosx*]"])endthird_target.build_configurations.eachdo |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

+ 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

 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

+ 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

+ 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

+ 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

 project 'RNTesterPods.xcodeproj'

 fabric_enabled = true
+ mac_catalyst_enabled = false


This line is not required! ;)

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

 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:
deffoo(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

+ 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.eachdo |config| assert_nil(config.build_settings["CODE_SIGN_IDENTITY[sdk=macosx*]"])endsecond_target.build_configurations.eachdo |config| assert_nil(config.build_settings["CODE_SIGN_IDENTITY[sdk=macosx*]"])endthird_target.build_configurations.eachdo |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

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


[nit] We can use a conditional modifier here:
Suggested change
ifmac_catalyst_enabled
ReactNativePodsUtils.apply_mac_catalyst_patches(installer)
end
ReactNativePodsUtils.apply_mac_catalyst_patches(installer)ifmac_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

 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
defreact_native_post_install(installer,mac_catalyst_enabled,react_native_path="../node_modules/react-native")
defreact_native_post_install(installer,react_native_path="../node_modules/react-native",mac_catalyst_enabled: false)

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

+ 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 cipolleschi and cortinico 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' ofhttps://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 2fb6a33Jun 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
Closed

-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")

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

-ifmac_catalyst_enabled
-ReactNativePodsUtils.apply_mac_catalyst_patches(installer)
-end
+ReactNativePodsUtils.apply_mac_catalyst_patches(installer)ifmac_catalyst_enabled

	defreact_native_post_install(installer,mac_catalyst_enabled,react_native_path="../node_modules/react-native")
	defreact_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)

Uh oh!

Conversation

Arkkeeper commented Jun 18, 2022

Summary

Changelog

Test Plan

Uh oh!

analysis-bot commented Jun 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikehardy left a comment

Choose a reason for hiding this comment

Uh oh!

analysis-bot commented Jun 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikehardy commented Jun 20, 2022

Uh oh!

cortinico commented Jun 20, 2022

Uh oh!

Arkkeeper commented Jun 22, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikehardy left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 22, 2022

Uh oh!

cipolleschi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cipolleschiJun 27, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Arkkeeper commented Jun 29, 2022

Uh oh!

cortinico commented Jun 29, 2022

Uh oh!

cortinico commented Jun 29, 2022

Uh oh!

facebook-github-bot commented Jun 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cipolleschi commented Jun 30, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

analysis-bot commented Jun 18, 2022•
edited
Loading

analysis-bot commented Jun 18, 2022•
edited
Loading

Arkkeeper commented Jun 22, 2022•
edited
Loading

cipolleschiJun 27, 2022•
edited
Loading

cipolleschi commented Jun 30, 2022•
edited
Loading