Fix post_install_workaround downgrading development targets

[Yonom]

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)

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

[facebook-github-bot]

Hi @Yonom!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[analysis-bot]

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

Base commit: fab4752
Branch: main

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,519,686	+0
android	hermes	armeabi-v7a	7,823,983	+0
android	hermes	x86	8,989,994	+0
android	hermes	x86_64	8,936,113	+0
android	jsc	arm64-v8a	9,833,101	+0
android	jsc	armeabi-v7a	8,798,904	+0
android	jsc	x86	9,783,739	+0
android	jsc	x86_64	10,386,645	+0

Base commit: a981528
Branch: main

[mikehardy]

This is close to perfect,

My only hesitation is that if someone targets deployment version 13 in their podfile (for whatever reason...) then both this change and the time symbol redefinition would fail

Ideally - not sure if it is possible! - the workaround would detect the Podfile's top-level ios deployment version and use that dynamically here for the conditional and upgrade version, and use it dynamically in the sed for the time redefinition below

This is incremental improvement though, I think this is a positive step by only altering the target to upgrade / avoiding a possible downgrade

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[Yonom]

@mikehardy Why would targetting v13 fail? I might be missing a slice of the bigger picture...

---

I updated my Podfile to say:

platform :ios, '13.0'

and the build still compiles just fine... (dependencies are still being bumped to minimum target 11 of course)

[mikehardy]

and the build still compiles just fine... (dependencies are still being bumped to minimum target 11 of course)

Very interesting result - I was expecting the time symbol redefinition hack to fail because of the way that test works.

See facebook/flipper#834 (comment)

If target is 13 and that test has been modified to 12 (via our hack), I expect the symbol to be defined in that code, resulting in it being redefined in the build as a whole and breaking the build.

I'm sort of shocked that built. I assume that object was not rebuilt when you did the build, also testing that part of the hack is difficult as it edits the file in place based on a regex from the original file, that is to say, after you run it once the sed will never happen again until you delete pods (maybe also) node_modules) and reinstall. I used npx react-native-clean-project a lot while testing these changes, and manually inspected the files before/after each run in order to verify. A bit tedious

[Yonom]

The time symbol redefinition errors only happen if I bump the minimum version in the workaround script to 12 / 13. In that case, the time symbols get duplicated definitions and that causes the build to fail.

Example of a change that causes the build to fail in the way you predict:

-config.build_settings['IPHONEOS_DEPLOYMENT_TARGET'] = '11.0'
+config.build_settings['IPHONEOS_DEPLOYMENT_TARGET'] = '12.0'

But if I understand correctly, there is no reason to bump the minimum targets to anything above 11.0, right? The main Podfile version seems to have no influence on the target versions of the dependencies

[mikehardy]

If original file is:

 __IPHONE_OS_VERSION_MIN_REQUIRED < __IPHONE_10_0)

and Podfile defines __IPHONE_OS_VERSION_MIN_REQUIRED as 11 (react-native template currently does)
then we fail unless we move the conditional to __IPHONE_12_0 (min target + 1)

That is the original workaround. Next case:

and Podfile defines __IPHONE_OS_VERSION_MIN_REQUIRED as 12 (expo template currently does I think)
then we fail unless we move the conditional to __IPHONE_13_0 (min target + 1 again)

General case:

and Podfile defines __IPHONE_OS_VERSION_MIN_REQUIRED as NN (as any person could do if they wanted)
then we fail unless we move the conditional to __IPHONE_NN+1_0 (min target + 1 again)

That's my understanding.

But if I understand correctly, there is no reason to bump the minimum targets to anything above 11.0, right? The main Podfile version seems to have no influence on the target versions of the dependencies

It was my understanding that __IPHONE_OS_VERSION_MIN_REQUIRED === platform :ios, NN (for whatever NN) from the Podfile.

Perhaps that's my error - is __IPHONE_OS_VERSION_MIN_REQUIRED not related to Podfile::platform version ?

If it is ===, then I don't understand how the time symbol redefinition will work as Podfile version is moved to 12 or NN unless that workaround is also moved to match.

Perhaps it's best at this point to stop being so fiddly about it and say the intention of the hack is to always have that conditional clause return true, so we should just edit to always return true. ?

[Yonom]

It was my understanding that __IPHONE_OS_VERSION_MIN_REQUIRED === platform :ios, NN (for whatever NN) from the Podfile.

Unfortunately I can't find any documentation on __IPHONE_OS_VERSION_MIN_REQUIRED. My test results suggest that __IPHONE_OS_VERSION_MIN_REQUIRED === IPHONEOS_DEPLOYMENT_TARGET (and thus 11.0 regardless of the end user's Podfile target)

Edit:

and Podfile defines __IPHONE_OS_VERSION_MIN_REQUIRED as 12 (expo template currently does I think)
then we fail unless we move the conditional to __IPHONE_13_0 (min target + 1 again)

If and only if you edit __apply_Xcode_12_5_M1_post_install_workaround to use '12.0' instead of '11.0' you will also need to update the sed command.

Changing __apply_Xcode_12_5_M1_post_install_workaround to use '12.0' is one way of getting 12.0 libraries to work in your react native project (because then no library will be downgraded).

This PR implements another way which does not require continously bumping the target version and thus the sed command needs no change.

[mikehardy]

This PR implements another way which does not require continously bumping the target version and thus the sed command needs no change.

I don't believe that is true based on reports of build failures I have seen

(and thus 11.0 regardless of the end user's Podfile target)

Ah, so __IPHONE_OS_VERSION_MIN_REQUIRED is not Podfile, then perhaps it is the Xcode settings which drive a setting in the pbxproj I think?

I still worry. I think people will still be affected by it, I have seen reports of people being affected by a build failure that were fixed by bumping that sed line to use 13, but did not know exactly why. Perhaps it is the pbxproj / Xcode setting moved independent of podfile and that triggers it.

This part may be the real "no need to look at this again" patch to the hack:

Perhaps it's best at this point to stop being so fiddly about it and say the intention of the hack is to always have that conditional clause return true, so we should just edit to always return true. ?

[Yonom]

Looking around, I can see that there are snippets of the fix going around where the minimum version is set to '12.0', for example: #32483 (comment)

This would explain why people are encountering the issue you describe. I agree that removing the check completely will potentially save some people headaches. Thank you for the idea :)

Added a commit.

[mikehardy]

Awesome, for what my approval is worth, I think this is good, and will help more people avoid these subtle compile failures. Fantastic @Yonom

[facebook-github-bot]

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

[ZComwiz]

Hi @Yonom! Can we integrate this fix into your awesome PR: reactwg/react-native-releases#1 (reply in thread)

It is a one-line fix to get us closer to re-supporting MacCatalyst on RN by adding || MacCatalyst to line 31 of time.h

[Yonom]

Hey @ZComwiz,

My PR deals with the temporary function __apply_Xcode_12_5_M1_post_install_workaround. Its purpose is to make builds failing for M1 to work again.

From my understanding, your issue is unrelated to XCode 12.5 or M1 machines, I do not think that it is a good idea to integrate any unrelated fixes to that function... Perhaps its best to apply your changes to RCT-Folly package itself (and in a separate PR)?

[philIip]

thanks for working on this @Yonom!

can you rebase and resubmit? i will go for the land soon.

[Yonom]

@philIip rebased on current main

[facebook-github-bot]

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

[facebook-github-bot]

@philIip merged this pull request in a4a3e67.

[mikehardy]

Thanks both of you @Yonom and @philIip for keeping this alive just a bit longer - it is of course a bit 🤢 to have a hack like this in there, but I also know it is helping a huge number of people because the issue traffic in all my repos regarding broken iOS builds is now literally zero :-). It was off the charts before. Cheers