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
Add Multiple Swift Versions Support #174
Conversation
Looks like CI is using "Bundler version 1.15.4" and the bundle file is using Bundler version 2. Checking into it. |
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.
Thanks for doing this! I'll let @dnkoutso do the final approval as he understands CP v1.7 better than I do. I just have one question.
Valet.podspec
Outdated
@@ -1,12 +1,13 @@ | |||
Pod::Spec.new do |s| | |||
s.name = 'Valet' | |||
s.version = '3.2.3' | |||
s.version = '3.2.4' | |||
s.cocoapods_version = '>= 1.7.0' |
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.
What does this line entail? Is this change backwards compatible?
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 is. I was just about to comment to that. It should be backwards compatible all the way back to CocoaPods 1.4.0 when the swift_version
(singular) DSL was first introduced.
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.
Awesome!
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.
You can remove this line btw. Once this PR lands prior versions all the way down to 1.4.0 will pick Swift 5.0 as their version.
Its probably because https://github.com/square/Valet/pull/174/files#diff-e79a60dc6b85309ae70a6ea8261eaf95R76 in this PR |
@jakeholland re Bundler, you should be able to run Edit: looks like |
Hm saw the build failure. I will debug. |
Oh its because of Xcode 9 |
Need to bump that. |
@dnkoutso - Would you rather I specify the Xcode version, or bump the Xcode version in the |
If we say 5.0 is the latest supported version our default should Additionally we can setup multiple jobs one per Swift version we support and use |
@dnkoutso - I am cool with either option, just bumping is obviously the easiest. But, if we update the jobs to include all supported Swift versions that would be the safest option. 🤔 EDIT: One thing to keep in mind is that we will still have to bump the Xcode version in general if we want to test against 5.0. |
You might need to be on Xcode 10.2 which was the first Xcode to support Swift 5. |
Good call! |
The downside of bumping: we no longer have a CI check on our lowest-supported Xcode version, which can lead to potential compiler issues. See how CI caught an Xcode 9 build failure here: #173 Linting on the swift version supported by CI seems fine. Alternatively, we could try running CI over multiple machines, but that seems out of scope for this PR. I'd prefer for us to continue to use the minimum supported Xcode version in CI if at all possible. |
If we get #175 working (it's looking promising), we can rebase this PR on top of |
If #175 lands then we can just update this PR to pass |
It’s merged! Please rebase. I made it easy for you to add the Swift version flag in CI too! Just change this line. |
@@ -1738,6 +1738,7 @@ | |||
SWIFT_OPTIMIZATION_LEVEL = "-Onone"; | |||
TARGETED_DEVICE_FAMILY = 3; | |||
TEST_HOST = "$(BUILT_PRODUCTS_DIR)/Valet tvOS Test Host App.app/Valet tvOS Test Host App"; | |||
TVOS_DEPLOYMENT_TARGET = 12.2; |
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 think we can remove the changes to this file.
Closing in favor of #178 Since this has a bunch of unneeded commits |
With Cocoapods 1.7.0 we are now able to specify support for multiple swift versions at once. This PR bumps the minimum Cocoapods version to 1.7.0 and adds explicit support for Swift versions 4.0, 4.1, 4.2, and 5.0. Valet's version has also been bumped to 3.2.4.