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

Cherry pick "Fix post_install_workaround downgrading development targets" to 0.66-stable #32715

Merged
merged 1 commit into from
Dec 7, 2021
Merged

Cherry pick "Fix post_install_workaround downgrading development targets" to 0.66-stable #32715

merged 1 commit into from
Dec 7, 2021

Conversation

Yonom
Copy link
Contributor

@Yonom Yonom commented Dec 6, 2021

Summary

ℹ️ This is a cherry-pick of #32633 to 0.66-stable

The __apply_Xcode_12_5_M1_post_install_workaround script changes the IPHONEOS_DEPLOYMENT_TARGET to 11.0 for all pods. This causes problems if the pods were targetting 12.0 or higher. Many expo modules are targetting 12.0.

I fixed this issue by checking the existing version and only bumping the target if it is lower than 11.0.

See also: this discussion post by @mikehardy reactwg/react-native-releases#1 (comment)

Changelog

[iOS] [Fixed] - __apply_Xcode_12_5_M1_post_install_workaround causing pods targetting iOS 12 and above to fail

Test Plan

Test (failing before this patch, passing after this patch)

  1. pick an iOS Pod that has a minimum deployment target of iOS 12 or higher, I chose the Braintree package
  2. npx react-native init myrnapp
  3. Open ios/Podfile and add the pod as a dependency: pod 'Braintree', '~> 5' (and upgrade the Podfile target to 12 (platform :ios, '12.0'))
  4. Compile the app.

Before applying this patch: ❌ Build fails because Braintree uses iOS 12 features and was downgraded to target 11.0
After applying this patch: ✅ Build succeeds

Summary:
The `__apply_Xcode_12_5_M1_post_install_workaround` script changes the `IPHONEOS_DEPLOYMENT_TARGET` to `11.0` for all pods. This causes problems if the pods were targetting `12.0` or higher. Many expo modules are targetting `12.0`.

I fixed this issue by checking the existing version and only bumping the target if it is lower than `11.0`.

See also: this discussion post by mikehardy reactwg/react-native-releases#1 (comment)

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - __apply_Xcode_12_5_M1_post_install_workaround causing pods targetting iOS 12 and above to fail

Pull Request resolved: #32633

Test Plan:

1. pick an iOS Pod that has a minimum deployment target of iOS 12 or higher, I chose the Braintree package
2. `npx react-native init myrnapp`
3. Open `ios/Podfile` and add the pod as a dependency: `pod 'Braintree', '~> 5'` (and upgrade the Podfile target to 12 (`platform :ios, '12.0'`))
4. Compile the app.

Before applying this patch: ❌ Build fails because Braintree uses iOS 12 features and was downgraded to target 11.0
After applying this patch: ✅ Build succeeds

Reviewed By: fkgozali

Differential Revision: D32638171

Pulled By: philIip

fbshipit-source-id: 0487647583057f3cfefcf515820855c7d4b16d31
@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 Dec 6, 2021
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Dec 6, 2021
@pull-bot
Copy link

pull-bot commented Dec 6, 2021

Warnings
⚠️

❔ Base Branch - The base branch for this PR is something other than main. Are you sure you want to merge these changes into a stable release? If you are interested in backporting updates to an older release, the suggested approach is to land those changes on main first and then cherry-pick the commits into the branch for that release. The Releases Guide has more information.

Generated by 🚫 dangerJS against 65f7e8f

@analysis-bot
Copy link

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

Base commit: bba5e6b

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,943,010 +651,081
android hermes armeabi-v7a 8,477,902 +855,280
android hermes x86 9,362,026 +599,785
android hermes x86_64 9,329,256 +627,881
android jsc arm64-v8a 10,626,440 +946,232
android jsc armeabi-v7a 9,548,852 +881,265
android jsc x86 10,639,736 +1,003,716
android jsc x86_64 11,250,658 +1,017,901

Base commit: bba5e6b

@mikehardy
Copy link
Contributor

Not sure of the process but I think the release management folks just command line these @lunaleaps ?
Not to take away from the fix itself being helpful 🙂

@lunaleaps
Copy link
Contributor

@mikehardy I'm looking to change the process so the work is placed on the pick requester -- really the "work" here is figuring out merge conflicts and also triggering our CircleCI tests. Granted, our CircleCI tests don't really provide much coverage yet but I think building the habit won't hurt.

@lunaleaps lunaleaps merged commit 83b9ddd into facebook:0.66-stable Dec 7, 2021
@Yonom Yonom deleted the 0.66-stable-fix-post-install-workaround branch December 7, 2021 13:23
facebook-github-bot pushed a commit to facebook/folly that referenced this pull request Feb 2, 2022
Summary:
When trying to bring react-native-macos up to date with 0.66, I was running into an issue getting RNTester to compile due to an error regarding redefining `clockid_t`. Other people have been seeing similar issues as per [these search results](https://github.com/facebook/folly/issues?q=clockid_t).

The history behind this appears to be as follows:

Several declarations in `<time.h>` were not available on Apple platforms until macOS 10.12 and iOS 10, which is why Folly needs to check the minimum version and set `FOLLY_HAVE_CLOCK_GETTIME` as needed. The problem is, the current logic as it stands right now is to set `FOLLY_HAVE_CLOCK_GETTIME = 1` (which implies that we don't need to declare them ourselves as the OS provides them for us) if...
* ...we're building for macOS, and the minimum required version is less than 10.12, or...
* ...we're building for iOS, and the minimum required version is less than 10.

However, this doesn't make any sense. This is saying that we don't need to declare these missing APIs if we could be compiling Folly for use on an older version (i.e., macOS 10.11/iOS 9 or earlier), which is totally wrong! Instead, we ought to be checking if the versions are *at least* macOS 10.12 or iOS 10.

React Native currently works around this by eliminating the minimum version check entirely with [this PR](facebook/react-native#32715), which is certainly a valid local fix ([the minimum iOS version for React Native apps is currently iOS 11](https://github.com/facebook/react-native/blob/1b31d6bb582768ccbe92d3c1a9e43354a8c531e5/template/ios/Podfile#L4)), but doesn't solve the problem at its core. This PR does solve the problem.

I have not tested building this with a minimum version below the above thresholds for use on a modern version of macOS/iOS, but considering the discussion in #1545, I think we should be safe to ignore these older versions from now on.

Pull Request resolved: #1688

Reviewed By: yfeldblum

Differential Revision: D33483705

Pulled By: Orvid

fbshipit-source-id: 0fe7c556af7e5b79a7679f75d003cf81a8f412ce
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. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants