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

Bumping iOS deployment target to 11.0 (10.0 after discussion) #1318

Merged
merged 1 commit into from
Jun 19, 2023

Conversation

crsantos
Copy link
Contributor

What changes

  • Bumping iOS deployment target to 11.0
  • Also osx to 10.13, tvos to 11.0 and watchos to 4.0
  • Fixed some warnings due to the bump
  • Bumped version to 6.23.0 (confirm me if I should do it or not, I can revert if needed)

Why do we need this PR

This PR fixes the issue reported on #1317, while using Xcode 14.3 to validate the podspec.

Every pod that depends on PromiseKit/CorePromise will fail to lint due to what is being explained here #1308 (comment).

TL;DR
Apple has stopped shipping libarclite_iphoneos.a separately 1

As #1311 only fixes the xcodeproj, this PR fixes the podspec.

Implications

With this change it means that consumers need a minimum deployment target of 11.0, but this will enable future Xcode versions since 14.3.

@RomanPodymov RomanPodymov requested review from mxcl and RomanPodymov May 31, 2023 18:06
@mxcl mxcl force-pushed the feature/bumping_ios11-deployment-target branch 3 times, most recently from 7aa9dfb to 8bcaf68 Compare June 5, 2023 22:26
@mxcl
Copy link
Owner

mxcl commented Jun 5, 2023

@crsantos
Copy link
Contributor Author

crsantos commented Jun 6, 2023

Seems we're missing some deployment targets? https://github.com/mxcl/PromiseKit/actions/runs/5182487581/jobs/9339327669?pr=1318#step:3:22

Oh,

Here's the issue as I'm seeing it, as I didn't know about these subspecs being submodules.

We have a lot of submodules from different libs/repos (e.g.: PromiseKit/Alamofire), but they're exposed as subspecs of PromiseKit,
and they all depend on a past version of PromiseKit, e.g.: here

github "mxcl/PromiseKit" ~> 6.13.0

Maybe I'm seeing it wrong, but we would need a new version of PromiseKit pointed on every single submodule Cartfile, compiling on every Xcode, including 14.3 (using 6.22.1 would work), but they are 23 extension, AFAIK:

  • AVFoundation
  • Accounts
  • AddressBook
  • Alamofire
  • AssetsLibrary
  • Bolts
  • CloudKit
  • CoreBluetooth
  • CoreLocation
  • EventKit
  • Foundation
  • HealthKit
  • HomeKit
  • MapKit
  • MessagesUI
  • OMGHTTPURLRQ
  • Photos
  • QuartzCore
  • Social
  • StoreKit
  • SystemConfiguration
  • UIKit
  • WatchConnectivity

So we need 23 PRs 😬

Bu please, confirm if I'm getting this right.

@mxcl
Copy link
Owner

mxcl commented Jun 6, 2023

Bu please, confirm if I'm getting this right.

no that only effects Carthage. The CocoaPod is entirely defined by the PromiseKit.podspec file.

We don't have the same problems with Carthage (which ofc backs up my original opinion that this is a CocoaPods issue that we are being forced to work around).

@crsantos
Copy link
Contributor Author

Hi, apologies for the delay.

But please, confirm if I'm getting this right.

no that only effects Carthage. The CocoaPod is entirely defined by the PromiseKit.podspec file.

We don't have the same problems with Carthage (which ofc backs up my original opinion that this is a CocoaPods issue that we are being forced to work around).

You're totally right, the dependencies of submodules/subspecs are being defined using Carthage, and it's a completely different subject.

But I have found the issue of the failed pod lib lint:

What needs to be done, AFAIK:

  1. If this is a cocoapods issue, having this fixed on cocoapods
  2. If this is due to Xcode 14.3, (because it works fine with 14.2, and it's not an issue):

Does my explanation make sense?

@mxcl
Copy link
Owner

mxcl commented Jun 14, 2023

We can drop Alamofire and release PMK 7 (this existing PMK7 rewrite branch was never released and should probs be abandoned at this point).

@mxcl
Copy link
Owner

mxcl commented Jun 14, 2023

The fact that Alamofire doesn't support iOS 11 makes me wonder why we cannot do the same? I know they are 10 and we are 8, but if they don't go to 11 why must we?

@crsantos
Copy link
Contributor Author

The fact that Alamofire doesn't support iOS 11 makes me wonder why we cannot do the same? I know they are 10 and we are 8, but if they don't go to 11 why must we?

That may work too, using 10 as minimum, as they're saying here Alamofire/Alamofire#3716 (comment), so it may lint, with just a warning.
I didn't know that was possible otherwise that would be my first try.

Let me validate that it lints with 10 ⌛

That can be one option, but it still require PMKAlamofire to be updated to use the last major of Alamofire 5.7.0.

But if you think we can drop PMKAlamofire subspec, we can go with the 7.0 branch. I'm just not sure what's in there, I'll have to check.

@crsantos
Copy link
Contributor Author

The fact that Alamofire doesn't support iOS 11 makes me wonder why we cannot do the same? I know they are 10 and we are 8, but if they don't go to 11 why must we?

That may work too, using 10 as minimum, as they're saying here Alamofire/Alamofire#3716 (comment), so it may lint, with just a warning. I didn't know that was possible otherwise that would be my first try.

Let me validate that it lints with 10 ⌛

That can be one option, but it still require PMKAlamofire to be updated to use the last major of Alamofire 5.7.0.

But if you think we can drop PMKAlamofire subspec, we can go with the 7.0 branch. I'm just not sure what's in there, I'll have to check.

Confirmed. iOS 10 lints with just warning.

One little detail 😅

along with PromiseKit/Alamofire also PromiseKit/Bolts has the same issue and PromiseKit/OMGHTTPURLRQ too.

They depend on an old version that only supports iOS 8 and 5.

Are they also candidates for deprecation with a major?

@mxcl
Copy link
Owner

mxcl commented Jun 16, 2023

eek, not sure what else to do.

Basically, remove them and see if this passes (remove the PMK version change, our automation will handle that). If it passes I will merge and release a v7.

@crsantos
Copy link
Contributor Author

crsantos commented Jun 18, 2023

eek, not sure what else to do.

Basically, remove them and see if this passes (remove the PMK version change, our automation will handle that). If it passes I will merge and release a v7.

Thanks for the feedback, Max 🙇

So, bumping iOS deployment target to 10.0 and removing the 3 specs, it works on the lint, but of course, with --allow-warnings enabled.

pod lib lint --allow-warnings --fail-fast

I'll update the MR to conform with that and let's see what CI tells us 😃

I'll just ask you and/or @RomanPodymov if the changes I did on the lint script are ok or not.

If this goes forward, we can clean up the old 3 submodules later (I can handle that)

@crsantos crsantos force-pushed the feature/bumping_ios11-deployment-target branch from 8bcaf68 to f66b0e4 Compare June 18, 2023 13:10
…nd OMGHTTPURLRQ subspecs

Also osx to 10.13, tvos to 10.0 and watchos to 4.0
@crsantos crsantos force-pushed the feature/bumping_ios11-deployment-target branch from f66b0e4 to 8b564f7 Compare June 18, 2023 13:12
@crsantos crsantos changed the title Bumping iOS deployment target to 11.0 Bumping iOS deployment target to ~11.0~ 10.0 Jun 18, 2023
@crsantos crsantos changed the title Bumping iOS deployment target to ~11.0~ 10.0 Bumping iOS deployment target to 11.0 (10.0 after discussion) Jun 18, 2023
@mxcl mxcl merged commit 707652b into mxcl:v6 Jun 19, 2023
@mxcl
Copy link
Owner

mxcl commented Jun 19, 2023

v8 https://github.com/mxcl/PromiseKit/actions/runs/5313482075

@mxcl
Copy link
Owner

mxcl commented Jun 19, 2023

failed due to old images, running again.

v8 because v7 was published and some people might depend on it.

@crsantos crsantos deleted the feature/bumping_ios11-deployment-target branch June 20, 2023 07:14
@crsantos
Copy link
Contributor Author

crsantos commented Jun 20, 2023

failed due to old images, running again.

If you need any action from my side, please shout 🙏

I'm seeing that it's trying to use Xcode 11, maybe travis doesn't have that version anymore?

Error: Could not find Xcode version that satisfied version spec: '^11'

I can open a PR adapting the yml files if you see fit.

@mxcl
Copy link
Owner

mxcl commented Jun 20, 2023

https://github.com/mxcl/PromiseKit/actions/runs/5322542957

@crsantos
Copy link
Contributor Author

Thanks for the help @mxcl and @RomanPodymov 🙏

@mxcl
Copy link
Owner

mxcl commented Jun 20, 2023

Apologies for this being long overdue

@oklimberg
Copy link

Now that subspec for Alamofire has been removed with version 8.0.0, can I still use PromiseKit and Alamofire with CocoaPods, or do I need to switch to SPM?
What was the argument against updating the Alamofire subspec (using Alamofire 5.x)? SPM Package PMKAlamofire version 4.0.0 does support Alamofire 5.x.
I also didn't quite understand why the decision was still on iOS 10 for the deployment target, when Apple requires Xcode 14 for updates/submissions and this version only supports iOS 11 (https://developer.apple.com/news/?id=jd9wcyov)
So to get rid of warnings I still have to use a post install hook for CocoaPods.

@crsantos
Copy link
Contributor Author

Hi @oklimberg

Now that subspec for Alamofire has been removed with version 8.0.0, can I still use PromiseKit and Alamofire with CocoaPods, or do I need to switch to SPM?

As of v8, as the podspec doesn't have that subspec, no, you can't use Cocoapods if you need the Alamofire subspec.
But you can target v6 or v7 for that.

What was the argument against updating the Alamofire subspec (using Alamofire 5.x)?

I think we didn't discuss in detail the why, maybe we should, but I'll explain our reasoning below.

SPM Package PMKAlamofire version 4.0.0 does support Alamofire 5.x.

I was not aware of that 🤔 and you're right. But bear in mind that it's not 5.5.2 that fixes the issue with iOS 10/11 deployment target, it's just 5.7.1

But, my 2 cents on why I didn't do this:

PMKAlamofire was locked to Alamofire 4.0, as you can see on the discussion above, and it needs the adoption of that major version 5.7.2. Why do I say that? That it needs adoption of major:

  • Did you test with PMKAlamofire with Xcode 14.3?
    • because PMAlamofire wasn't compiling when bumping that major version of AF, changes were required.

I also didn't quite understand why the decision was still on iOS 10 for the deployment target, when Apple requires Xcode 14 for updates/submissions and this version only supports iOS 11 (https://developer.apple.com/news/?id=jd9wcyov)

The explanation is on this comment.

So to get rid of warnings I still have to use a post install hook for CocoaPods.

IMHO, post install hook solves the issue for a Podfile, and it works.
But a spec not linting because a podspec dependency targets older deployment targets is something, is a real problem.

Nevertheless, the submodule is still there.
If everyone agrees that it should stil be considered and maintained, it's just a question of contributing with a PR to update PMKAlamofire work with AF 5.7.1 and then re-introduce the spec with proper minimum deployment targets.

@oklimberg
Copy link

Thank you very much for your time explaining the reasoning.
I will have a look, if I am able to provide a PR for PromiseKit 8 and Alamofire 5 in general and if the linting issue will require
Alamofire 5.7.1

@crsantos
Copy link
Contributor Author

crsantos commented Jun 22, 2023 via email

@mxcl
Copy link
Owner

mxcl commented Jun 22, 2023

Bringing back the Alamofire subspec is welcome. PR should be pretty simple.

We could not keep Alamofire and change the deployment targets so it had to be removed or upgraded. We wanted to give people a working PromiseKit release ASAP so we removed it.

@oklimberg I can assist with the work if you need it.

@crsantos
Copy link
Contributor Author

@oklimberg I can also assist on this if you need

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants