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

[RFC] CocoaPods Ranged Swift Version support #7134

Closed
dantoml opened this Issue Oct 13, 2017 · 38 comments

Comments

Projects
None yet
10 participants
@dantoml
Copy link
Member

dantoml commented Oct 13, 2017

Core PR: CocoaPods/Core#417
CocoaPods PR: #7253

Background

These issues provide some more context, but I am happy to do a more detailed write up of my goals as a maintainer, if needs be:

Suggested Solution

DSL

Add a swift_version property to a specification, which allows a user to provide
a Requirement a la >= 3.2, <= 4.5 or 4.0.

For example:

Pod::Spec.new do |s|
  s.name = 'BanannaLib'
  s.version = '1.0.0'
  s.swift_version = '>= 3.2, <= 4.0'
  s.source_files = '**/*.swift'
end

Linting

When linting a pod, via pod lib lint or pod spec lint, we will maintain
the current behaviour of allowing a --swift-version or .swift-version file.

A future change for this would be to allow specifying a collection of swift
versions, such as --swift-version=3.2,4.0,4.1 and building and testing the lib
for each of these versions.

Integration

When integrating a Pod, we should always aim to use the most recent version of
Swift that is possible. Because CocoaPods does not have a way to know about
the version of Xcode that is in use, our interpretation of the 'newest' version
should be that of the target you are integrating into.

For example:

- Target A (Swift 5.0)
  - Pod B (>= 3.2) -> Swift 5.0
  - Pod C (>= 3.2, <= 4.0) -> Swift 4.0
    - Pod D (no-range) -> Swift 4.0
    - Pod E (<= 3.2) -> Swift 3.2
      - Pod F (no-range) -> Swift 3.2
      - Pod G (= 4.0) -> Swift 4.0

When we have multiple targets:

- Target A (Swift 5.0)
  - Pod B (>= 3.2) -> Swift 4.0
- Target B (Swift 4.0)
  - Pod B (>= 3.2) -> Swift 4.0

When Compatibility is Broken

The swift_version property represents the library authors intention of the
code to support the version range that they specified, with the information that
they have about past and future versions of Swift.

Due to this being unverifiable on future Swift versions at the time of publishing,
a loose version range, such as >= 3.2 may break in the future.
Authors should be mindful of this, and specify defensive ranges, rather than
being overly broad.

For example, >= 3.2, < 5 may be a reasonable range for a pod authored today.

In the cases where a resolved version of Swift proves to be incompatible, CocoaPods
will not offer a specific language to override this, as doing so for issues in transitive dependencies would require complicating much of the DSL. However, teams may override the setting with a post install hook like below:

post_install do
  # Include example to override swift versions here.
end
@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Oct 13, 2017

@dantoml I'd argue we deprecate or remove --swift-vesion and .swift-version file entirely and let the podspec DSL drive the actual value to use. It seems weird for a podspec to have a Swift version set in its DSL but then to be overridden (and published) using a different version during spec or lib lint (or push).

Thoughts?

@dantoml

This comment has been minimized.

Copy link
Member Author

dantoml commented Oct 13, 2017

@dnkoutso Our issue then, is what do we do when:

  • No version range is provided
  • Only a minimum version range is provided
  • Only a maximum range is provided
@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Oct 13, 2017

Hm good points, should we handle these cases when --swift-version is passed and potentially error out if you are within the range or not?

Pod::Spec.new do |s|
  s.name = 'BanannaLib'
  s.version = '1.0.0'
  s.swift_version = '>= 3.2'
  s.source_files = '**/*.swift'
end

and:

pod lib lint BanannaLib --swift-version=2.3 ---> error?

@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Oct 13, 2017

Btw the RFC LGTM overall!

@dantoml

This comment has been minimized.

Copy link
Member Author

dantoml commented Oct 13, 2017

@dnkoutso I think we’d actually get that for free when it tried to integrate the test project, but yes that is the behaviour that seems most obvious to me

@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Oct 13, 2017

Minor edge case to handle, and not in the core, is a podspec providing a swift_version without having any Swift files in it (uses_swift? returns false)

@dantoml

This comment has been minimized.

Copy link
Member Author

dantoml commented Oct 13, 2017

That should fail linting unless you have a very good reason IMO

@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Oct 13, 2017

Agreed.

@orta

This comment has been minimized.

Copy link
Member

orta commented Oct 14, 2017

IMO we should keep the .swift-version file support if it's easy to keep around - it's a good community standard with SwiftPM also

@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Oct 27, 2017

Just wanted to chime in regarding the test native targets. I believe test native targets generated by test specs of a podspec should inherit the swift version specified. Is that correct or am I missing something @dantoml ?

@1ec5 1ec5 referenced this issue Oct 30, 2017

Merged

Update to swift 4 #50

rebe1one added a commit to rebe1one/alertcontroller that referenced this issue Nov 1, 2017

@gistya

This comment has been minimized.

Copy link

gistya commented Nov 2, 2017

I would prefer simply:

Pod::Spec.new do |s|
  s.name = 'RubyToSwiftConversionAI'
  s.version = '1.0.0'
  s.swift = '>= 3.2'
  s.source = '**/*.swift'
end

... dropping the unnecessary (and inaccurate) _version and _files.

(Inaccurate because we have a set of ranges of versions, not a version; and because we have a path to directories, files, and symlinks, not just files.)

Other than that I really like this direction. As for @dnkoutso's comment, the answer should be yes they should inherit their parent .podspec's s.swift version, unless y'know, they override it with test_spec.swift = '< 4.0' in which case it would simply add to the parent's s.swift array unless it caused a conflict—dependency management 🗡

@dantoml

This comment has been minimized.

Copy link
Member Author

dantoml commented Nov 2, 2017

@gistya it’s not possible to lint a pod without a concrete version to build and test against, so we need to keep that file around.

Subspecs will not be allowed to override or modify the swift version, and should obey the same range as the root spec.

@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Nov 2, 2017

@dantoml is this case correct that's in the initial proposal?

- Target A (Swift 5.0)
  - Pod C (>= 3.2, <= 4.0) -> Swift 4.0

Should we allow this case since Target A supports Swift 5.0 but Pod C has Swift 4.0?

@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Nov 2, 2017

I guess depends if we treat the targets Swift version as <= 5.0 or not and whether Xcode supports this configuration.

@tzm41

This comment has been minimized.

Copy link

tzm41 commented Dec 5, 2017

Thanks. It would be great if the changes in 1.4.0, and proposed changes in 1.5.0 could be clarified.

@jshier

This comment has been minimized.

Copy link

jshier commented Dec 11, 2017

@dnkoutso After your PR, master now requires the swift_version field and crashes without it, at least for podspecs out of git repos.

@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Dec 11, 2017

@jshier thanks for reporting this. Do you publish podspecs in ruby and not JSON? We used to do the same but I think its preferred to publish them in JSON using --use-json parameter during publishing. I highly recommend doing the same.

Having said that, since this is a minor update (1.4.0) of CocoaPods I think we should guard against this. Will probably have a PR this week after I chat a bit with colleagues on handling this.

@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Jan 17, 2018

Updated title to specify this is support for "Ranged" Swift version Support in 1.5.0 (if we make it).

For 1.4.0 swift_version DSL has been added with a single swift version.

@dantoml

This comment has been minimized.

Copy link
Member Author

dantoml commented Jan 18, 2018

@dnkoutso isn't that still ignored?

@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Jan 18, 2018

To what are you referring to?

@thomasvl

This comment has been minimized.

Copy link

thomasvl commented Jul 26, 2018

Any updates on this?

With things like Swift support for conditional conformances or new protocols in 4.2 (CaseIterable), as an author of a library, we can include the support wrapped in #if swift(>=4.2) guards, but CocoaPods support for a single swift_version it seems to force libraries to either:

  1. Have a branch for every swift version with unique version numbers to segment.
  2. Force all our consumers to adopt the newest Xcode to get the tools support.

The overhead 1 would force has the potential to be non trivial, but 2 doesn't seem right either; especially since 2 would likely mean we'd be back to having to do multiple branches if there was a bug fix we needed to provide for folks that couldn't move to a new toolchain (if they are about to do an release release and cannot change the toolchain right then).

fyi - SwiftPM does support indicating multiple versions, so it doesn't run into this.

@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Jul 26, 2018

Not much progress although I do have my eyes set on this for 1.7.0 version of CocoaPods. Right now I am trying to wrap up the first 1.6.0 beta.

Totally agree we should expand this to support multiple versions, perhaps an array of versions in the DSL similar to that of SwiftPM? I wouldn't mind if its the exact same functionality as SwiftPM in terms of rules of choosing Swift version to use during pod install.

These are the current 1.6.0 PRs open (https://github.com/CocoaPods/CocoaPods/pulls?q=is%3Aopen+is%3Apr+milestone%3A1.6.0) but only one of the two needs to land (the test spec one) in order for 1.6.0 beta to begin as its the last feature enhancement.

If anyone would like to take sometime in the meantime to expand this I'd be more than happy to review and maybe even land it in 1.6.0! I'd start here https://github.com/CocoaPods/Core/blob/master/lib/cocoapods-core/specification/dsl.rb#L132-L143 by expanding the DSL to accept an array of strings perhaps which is all supported Swift versions.

@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Jul 26, 2018

We might also need to introduce a DSL for the Podfile to allow consumers to select a Swift version for a pod (as long as it's included in the list of supported Swift versions the pod author has declared).

Like:
pod 'SwiftPod', '1.0', :swift_version => '4.0'

Although today there are ways to do this via a post_install hook the DSL will be cleaner.

@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Aug 4, 2018

Gonna mark this for 1.7.0

@markst

This comment has been minimized.

Copy link

markst commented Oct 15, 2018

We might also need to introduce a DSL for the Podfile to allow consumers to select a Swift version for a pod (as long as it's included in the list of supported Swift versions the pod author has declared).

Like:
pod 'SwiftPod', '1.0', :swift_version => '4.0'

often have a use case for this!

@dnkoutso

This comment has been minimized.

Copy link
Contributor

dnkoutso commented Oct 15, 2018

To everyone following this thread. I am closing it in favor of #8191

I wrote the entire RFC again and have actually implemented the changes.

Please direct comments to #8191.

Aiming to ship this with 1.7.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment