Skip to content

Remove IPHONEOS_DEPLOYMENT_TARGET for all Pods by letting Pod targets inherit deployment target from main app#3062

Merged
AliSoftware merged 7 commits intodevelopfrom
feat/deployment-target-warnings
Nov 2, 2020
Merged

Remove IPHONEOS_DEPLOYMENT_TARGET for all Pods by letting Pod targets inherit deployment target from main app#3062
AliSoftware merged 7 commits intodevelopfrom
feat/deployment-target-warnings

Conversation

@ctarda
Copy link
Copy Markdown
Contributor

@ctarda ctarda commented Oct 26, 2020

Closes #3022, closes #3028, closes #3026, closes #3021, closes #3030, closes #3027, closes #3025, closes #3023, closes #3024, closes #3029

Xcode 12 issues warnings due to the deployment target of the Pods being different from the deployment target declared in the main binary.

Changes

This PR adds a post_install phase to the Podfile to remove the IPHONEOS_DEPLOYMENT_TARGET setting from all the Pod's target configurations.

By doing so, we would remove 29 warnings.

If this feels like a workaround it's because it probably is, but it seems to be the expected behaviour, and the expected solution.

Testing

  • Checkout the branch
  • Run bundle exec pod install
  • Build the project. Filter warnings by IPHONEOS_DEPLOYMENT_TARGET. There shouldn't be any!

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Add comment

Run bundle exec pod install
@shiki
Copy link
Copy Markdown
Contributor

shiki commented Oct 27, 2020

I also created something like this in this branch which hides the warnings temporarily but also spits out the hidden warnings so we'll be aware to tackle them later. But I wasn't sure if hiding the warnings instead of fixing them was something we would be interested in doing. 😅

Related async discussion at CGPNUU63E-p1602782385085300-slack.

Also, I have no idea what should be the “real” fix. 😅 The only thing that worked for me was upgrading the libraries' deployment targets (e.g. wordpress-mobile/WordPressKit-iOS#286). Hoping to learn from you on this! 🙏

@ctarda
Copy link
Copy Markdown
Contributor Author

ctarda commented Oct 27, 2020

Nice, I missed that discussion, sorry.

I like the idea of letting the configuration of the main binary have preference though, the less special cases we have the easier it should be, in theory, to maintain this in the long term. On the other hand, it's not that big of a deal, it's not like we have to do a lot of manual maintenance.

I think there might be some middle ground: one thing we could do is let the main binary settings be inherited for those Pods we don't have control over, and update our own libraries to upgrade their deployment target?

@peril-woocommerce
Copy link
Copy Markdown

peril-woocommerce bot commented Oct 27, 2020

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-woocommerce
Copy link
Copy Markdown

peril-woocommerce bot commented Oct 27, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@ctarda ctarda self-assigned this Oct 29, 2020
@ctarda ctarda added Low hanging fruit category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc. labels Oct 30, 2020
@ctarda ctarda marked this pull request as ready for review October 30, 2020 01:07
@ctarda ctarda requested a review from a team October 30, 2020 01:07
@ctarda
Copy link
Copy Markdown
Contributor Author

ctarda commented Oct 30, 2020

Pinging @AliSoftware as well. This is pretty much the solution you suggested 😊

@ctarda ctarda requested a review from AliSoftware October 30, 2020 01:08
@ctarda ctarda added this to the 5.4 milestone Oct 30, 2020
Podfile Outdated
platform :ios, '12.0'
app_ios_dt = Gem::Version.new('12.0')

platform :ios, app_ios_dt
Copy link
Copy Markdown
Contributor

@AliSoftware AliSoftware Oct 30, 2020

Choose a reason for hiding this comment

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

Not sure the platform directive officially accepts a Gem::Version instead of a String? 🤔
(Maybe it does, but on phone rn so can't test it myself)

Suggested change
platform :ios, app_ios_dt
platform :ios, app_ios_dt.to_s

nit: also I realised that maybe we should pick better names for those variables than the one I chose when I suggested this solution 😅 e.g. app_deployment_target?

Copy link
Copy Markdown
Contributor Author

@ctarda ctarda Nov 2, 2020

Choose a reason for hiding this comment

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

Thanks for taking a look @AliSoftware

Not sure the platform directive officially accepts a Gem::Version instead of a String? 🤔

It seemed to do so, but I updated to pass .version .

maybe we should pick better names

Renamed!

@ctarda ctarda requested a review from AliSoftware November 2, 2020 01:16
@AliSoftware AliSoftware merged commit 30fe457 into develop Nov 2, 2020
@AliSoftware AliSoftware deleted the feat/deployment-target-warnings branch November 2, 2020 14:07
@designsimply designsimply added good first issue The issue is a good candidate for a community contribution or for a newcomer to the team. and removed Low hanging fruit labels May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment