-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Update Package.swift to support Swift 5 #1913
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should support 4.2 and 5 - I think you need to add both for Swift language versions
@freak4pc, thanks for review. I've added |
Hm, don't see it. |
@kzaher, I forgot to force push the branch 😅 |
@@ -42,6 +42,9 @@ extension Target { | |||
|
|||
let package = Package( | |||
name: "RxSwift", | |||
platforms: [ | |||
.macOS(.v10_10), .iOS(.v8), .tvOS(.v9), .watchOS(.v3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how this works but shouldn't we have Linux as well? Or is that by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so but I couldn't find the linux platform definition: https://github.com/apple/swift-package-manager/blob/b060e8e46ea1d8ae15c909405bb6ff7e7251a8cb/Sources/PackageDescription4/SupportedPlatforms.swift#L28-L90
There is init()
constructor that accepts Platform
but it is unavailable since it's internal:
platforms: [
SupportedPlatform(platform: .linux) // 'init' is inaccessible due to 'internal' protection level
]
a6d6d8c
to
bb6160a
Compare
Hmm I'm not sure how to configure |
Yeah, that's the tricky one. We would need to make our tests more generic. @freak4pc I can do that if you want, just don't modify travis for your PR. |
Sure, no problems. Feel free to make the changes and I’ll amend my PR. |
Hi @devxoul , I'm not sure how this would work. We need to maintain backwards compatibility. Do we need to use 5.0 in the definition? Isn't |
@devxoul We have changed package a bit. Could you please provide some command that is failing for you? |
4.2 should be compatible with the Swift 5 tooling and compiler. I'm not sure 4.0 would be 🤔 |
I think we need to release 5.0 just for Xcode >= 10.2. This is hopeless. |
@kzaher I've made some test on |
It's like SDK version and minimum deployment target. As you can support iOS 9 with the latest iOS SDK 12.2, you can still use Swift 4.2 with Xcode 10.2 (Swift toolchain 5). I don't think it works with Xcode < 10.2 (Swift toolchain 4). |
How did apps worked without this definition up until now? I'm just wondering if it's worth making such a drastic change for SPM alone. |
@devxoul we'll release 5.0 and clean up everything. Otherwise we'll go insane :) At least I will. |
This was done in #1920 |
As upgrading to Swift 5, without having
platforms
the default deployment target is set to the latest version of each OS. It causes a compile-time minimum deployment target error.platforms
(SE-0236).v5
toswiftLanguageVersions