Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ios): add xcode 14 workaround (turn off signing resource bundles) for pods #34826

Closed
wants to merge 8 commits into from

Conversation

kelset
Copy link
Contributor

@kelset kelset commented Sep 30, 2022

Summary

This is inspired by the Expo workaround expo/expo@d970a9e to address an issue that cocoapods has with Xcode 14: CocoaPods/CocoaPods#11402

This wants to address this #34673 in a way that we can also cherry-pick on the 0.70 branch.

Changelog

[iOS] [Fixed] - add xcode 14 workaround (turn off signing resource bundles) for React-Core

Test Plan

Tested locally by opening RNTester via Xcode 14.0.1, and targetting my iPhone as device. After applying the patch, the error for React Core AccessibilityResources disappears.

Also, added ruby test for new patch.

@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 Sep 30, 2022
@kelset kelset requested a review from cipolleschi September 30, 2022 11:50
@facebook-github-bot facebook-github-bot added Contributor A React Native contributor. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Sep 30, 2022
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Sep 30, 2022
@analysis-bot
Copy link

analysis-bot commented Sep 30, 2022

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

Base commit: 36c9716
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.

We should refactor this in the ReactNativePodsUtils file and add some tests.

@danilobuerger
Copy link
Contributor

I don't like how this modifies all pods, not only the ones included by rn. We only need this for one resource bundle:

s.resource_bundle = { "AccessibilityResources" => ["React/AccessibilityResources/*.lproj"]}

@Kudo
Copy link
Contributor

Kudo commented Oct 3, 2022

I don't like how this modifies all pods, not only the ones included by rn. We only need this for one resource bundle:

we use this strategy because we also have other resource bundles at expo 😅 please do whatever best fit for react-native templates.

@kelset
Copy link
Contributor Author

kelset commented Oct 4, 2022

yeah I agree that this workaround is not ideal - my current concern is that if we do it for just that specific resource bundle, developers might still hit the issue. I feel that maybe we should just add this to the template so that developers will have to "opt in" into copying this pod postinstall over in their setups understanding what this is doing.

@kelset
Copy link
Contributor Author

kelset commented Oct 4, 2022

also, it seems that by going for this route (of not doing the signing) you might incurr in Apple rejection along the lines of:

The binary with bundle identifier 'SOME_IDENTIFIER' at path [SOME_APP.app] contains an invalid signature. Make sure you have signed your application with a distribution certificate, not an ad hoc certificate or a development
certificate. Verify that the code signing settings in Xcode are correct at the target level (which override
any values at the project level). If you are certain your code signing settings are correct, choose 'Clean
All' in Xcode, delete the 'build' directory in the Finder, and rebuild your release target.

see more details here.

So maybe we should go another route and tell folks to go for

 installer.generated_projects.each do |project|
    project.targets.each do |target|
        target.build_configurations.each do |config|
            config.build_settings["DEVELOPMENT_TEAM"] = "TEAM_ID"
        end
    end
  end

instead, as suggested here? (meaning, configuring the team account to sign it all)

@danilobuerger
Copy link
Contributor

I think the rejects are "unrelated" and not caused by unsigned resource bundles. We have gone through review a couple of times now with unsigned resource bundles and xcode 14. 🤷‍♂️

We could just add a new func like we did for the m1 so people can opt out more easily without removing the whole post install func call.

@cipolleschi
Copy link
Contributor

What if we modify this to act only on the React-Core pod, like we did here?

Also, it would be nice to wrap this code in a function that lives in the ReactNativePodsUtils class with some tests.

@kelset, if you are busy, I can take it. Let me know what do you prefer

@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.

@kelset kelset force-pushed the kelset/add-workaround-for-xcode14 branch from 19b6f48 to 4860792 Compare October 5, 2022 09:25
@analysis-bot
Copy link

analysis-bot commented Oct 5, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,741,312 +0
android hermes armeabi-v7a 7,143,663 +0
android hermes x86 8,051,980 +0
android hermes x86_64 8,023,548 +0
android jsc arm64-v8a 9,608,592 +0
android jsc armeabi-v7a 8,373,913 +0
android jsc x86 9,556,067 +0
android jsc x86_64 10,148,413 +0

Base commit: 36c9716
Branch: main

@kelset kelset requested a review from cipolleschi October 5, 2022 10:54
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.

Hi @kelset! Thank you for doing this! I left some more improvements for the tests. Let me know what do you think!

scripts/cocoapods/__tests__/utils-test.rb Outdated Show resolved Hide resolved
scripts/cocoapods/__tests__/utils-test.rb Outdated Show resolved Hide resolved
scripts/cocoapods/__tests__/utils-test.rb Outdated Show resolved Hide resolved
@kelset kelset force-pushed the kelset/add-workaround-for-xcode14 branch from 291b9cf to 357df61 Compare October 6, 2022 09:38
@kelset kelset requested a review from cipolleschi October 6, 2022 11:27
@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.

@kelset kelset deleted the kelset/add-workaround-for-xcode14 branch October 7, 2022 09:34
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @kelset in 967de03.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Oct 7, 2022
kelset added a commit that referenced this pull request Oct 11, 2022
… for pods (#34826)

Summary:
This is inspired by the Expo workaround expo/expo@d970a9e to address an issue that cocoapods has with Xcode 14: CocoaPods/CocoaPods#11402

This wants to address this #34673 in a way that we can also cherry-pick on the 0.70 branch.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[iOS] [Fixed] - add xcode 14 workaround (turn off signing resource bundles) for `React-Core`

Pull Request resolved: #34826

Test Plan:
Tested locally by opening RNTester via Xcode 14.0.1, and targetting my iPhone as device. After applying the patch, the error for React Core AccessibilityResources disappears.

Also, added ruby test for new patch.

Reviewed By: hramos

Differential Revision: D40063828

Pulled By: hramos

fbshipit-source-id: e10d5b6a917a6a7cbacd14ecfdac55e60e46c6f8
@taylorkline
Copy link

we should be able to remove this once CocoaPods catches up to it, see more details here CocoaPods/CocoaPods#11402

Will be fixed in CocoaPods 1.12.0: CocoaPods/CocoaPods#11402 (comment)

OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
… for pods (facebook#34826)

Summary:
This is inspired by the Expo workaround expo/expo@d970a9e to address an issue that cocoapods has with Xcode 14: CocoaPods/CocoaPods#11402

This wants to address this facebook#34673 in a way that we can also cherry-pick on the 0.70 branch.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[iOS] [Fixed] - add xcode 14 workaround (turn off signing resource bundles) for `React-Core`

Pull Request resolved: facebook#34826

Test Plan:
Tested locally by opening RNTester via Xcode 14.0.1, and targetting my iPhone as device. After applying the patch, the error for React Core AccessibilityResources disappears.

Also, added ruby test for new patch.

Reviewed By: hramos

Differential Revision: D40063828

Pulled By: hramos

fbshipit-source-id: e10d5b6a917a6a7cbacd14ecfdac55e60e46c6f8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner Platform: iOS iOS applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants