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

Defining versions in Cartfile for dependencies #212

Merged
merged 4 commits into from
Apr 10, 2019
Merged

Defining versions in Cartfile for dependencies #212

merged 4 commits into from
Apr 10, 2019

Conversation

cowgp
Copy link
Contributor

@cowgp cowgp commented Mar 29, 2019

Description of the pull request

Defining versions in Cartfile for dependencies so dependencies can update to latest xcode/swift and publish breaking versions without breaking current release of this when still run on something other than the latest xcode/swift development environment

Why is the change necessary?

Reachability, Starscream, and CryptoSwift have all published releases compatible with Swift 5 and XCode 10.2. Because the current Cartfile does not define any version requirements for the depencies, the "latest" release is always taken. Running carthage update or carthage bootstrap as our CI build tool does is now failing as we are not yet running XCode 10.2 in CI (or locally).

Going forward, as releases of this library come out supporting different swift/xcode versions, the versions of the dependencies should remain defined (and updated) in the Cartfile so that they build with the correct known working versions.

…e to latest xcode/swift and publish breaking versions without breaking current release of this when still run on something other than the latestt xcode/swift development enviornment
@TomKemp
Copy link
Contributor

TomKemp commented Apr 4, 2019

Thanks for the PR. That seems sensible. What was the reason for lowering the version of TaskQueue?

Also, the dependencies in the Podspec should match the Cartfile, so I'll make some changes to the Podspec to match and push them to this branch.

@cowgp
Copy link
Contributor Author

cowgp commented Apr 6, 2019

I made the podspec and carthage consistent and switched all dependencies to == rather than ~> as CryptoSwift was somehow evaluating to 1.0.0 when it was set to ~> 0.15 which shouldn't be possible, but it was. I did put TaskQueue back to 1.1.1 - when I went through and put the values in for the dependencies I was looking at the releases on each repo, and TaskQueue hasn't been publishing releases even though they are tagging each one - so I put an old version in as a result. But it's now at listed as 1.1.1

@TomKemp
Copy link
Contributor

TomKemp commented Apr 8, 2019

Sorry, I didn't get round to pushing my changes. It looks like we ended up with roughly the same Podspec though. The only difference is that I don't think it's necessary to pin CryptoSwift and TaskQueue to precise versions. It's probably desirable to allow flexibility.

I've tested my version using both Carthage and CocoaPods and I get the correct versions installed. I noticed that ~>to means something different in Carthage vs Cocoapods, which could be why you were having problems, but I'm not sure how that would end up returning 1.0.0.

I've pushed my proposed changes. What do you think?

@TomKemp
Copy link
Contributor

TomKemp commented Apr 8, 2019

I appreciate that the versions don't match exactly between the Cartfile and Podspec but my reasoning is that:

  • Starscream: updated to Swift 5 on a minor version. In Carthage ~> will update minor versions so we have to pin to an exact version. In CocoaPods however, we can use ~> to only update patch versions. It makes sense for us to allow flexibility where we can, whilst still holding the version back before that Swift 5 release.

  • CryptoSwift: the Swift 5 release was 1.0.0, but the behaviour of ~> in Carthage is different in versions < 1.0.0 and won't update minor versions, so we can't express the same restrictions in Carthage and CocoaPods.

  • Reachability: updated to Swift 5 on a patch version therefore we have to pin to an exact version in both files.

  • TaskQueue: no recent changes.

@TomKemp
Copy link
Contributor

TomKemp commented Apr 10, 2019

@lukabratos and I reviewed this together in person and decided it's ready to merge.

@TomKemp TomKemp merged commit 33d994e into pusher:master Apr 10, 2019
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.

2 participants