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): fix Time.h patch not being applied #32961

Closed
wants to merge 1 commit into from
Closed

Conversation

tido64
Copy link
Collaborator

@tido64 tido64 commented Jan 24, 2022

Summary

The path to Time.h is currently hard-coded and does not take into
consideration the --project-directory flag when running pod install.

Changelog

[iOS] [Fixed] - Fix Time.h patch not being applied when running pod install --project-directory=ios

Test Plan

git clone https://github.com/microsoft/react-native-test-app.git
cd react-native-test-app
npm run set-react-version main
yarn
cd example
pod install --project-directory=ios
../scripts/xcodebuild.sh ios/Example.xcworkspace build

The path to `Time.h` is currently hard-coded and does not take into
consideration the `--project-directory` flag when running `pod install`.
@facebook-github-bot 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. 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 Jan 24, 2022
@tido64
Copy link
Collaborator Author

tido64 commented Jan 24, 2022

@lunaleaps, @kelset: Please also backport this to 0.67 as it is a regression.

@kelset kelset requested a review from cortinico January 24, 2022 19:10
@@ -646,5 +646,6 @@ def __apply_Xcode_12_5_M1_post_install_workaround(installer)
# "Time.h:52:17: error: typedef redefinition with different types"
# We need to make a patch to RCT-Folly - remove the `__IPHONE_OS_VERSION_MIN_REQUIRED` check.
# See https://github.com/facebook/flipper/issues/834 for more details.
`sed -i -e $'s/ && (__IPHONE_OS_VERSION_MIN_REQUIRED < __IPHONE_10_0)//' Pods/RCT-Folly/folly/portability/Time.h`
time_header = "#{Pod::Config.instance.installation_root.to_s}/Pods/RCT-Folly/folly/portability/Time.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha of course! My hack lives on 😆 but I didn't think about making it durable.
Is there an RCT-Folly issue pursuing this for a non-hack fix? Just curious

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the non hack fix not be facebook/folly#1688 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Saadnajmi that looks like it exactly! Hopefully it lands soon-ish and react-native 0.68 can include it

Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the original hacker, this is terrible and I'm saying that about my own hack 😅 , but also this is an obvious improvement. Nice catch + fix

@facebook-github-bot
Copy link
Contributor

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @tido64 in 60cef85.

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 Jan 24, 2022
@tido64 tido64 deleted the tido/fix-pod-install branch January 26, 2022 10:51
kelset pushed a commit that referenced this pull request Jan 31, 2022
Summary:
The path to `Time.h` is currently hard-coded and does not take into
consideration the `--project-directory` flag when running `pod install`.

## Changelog

[iOS] [Fixed] - Fix `Time.h` patch not being applied when running `pod install --project-directory=ios`

Pull Request resolved: #32961

Test Plan:
```
git clone https://github.com/microsoft/react-native-test-app.git
cd react-native-test-app
npm run set-react-version main
yarn
cd example
pod install --project-directory=ios
../scripts/xcodebuild.sh ios/Example.xcworkspace build
```

Reviewed By: christophpurrer

Differential Revision: D33748789

Pulled By: lunaleaps

fbshipit-source-id: b125734eba30e552ae139e7ecd4e634c8fa1bcd7
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. 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.

None yet

6 participants