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

add a CocoaPods podspec generator #54

Merged
merged 3 commits into from Apr 30, 2019
Merged

Conversation

weissi
Copy link
Member

@weissi weissi commented Apr 15, 2019

Motivation:

Users ask for a CocoaPod. Unfortunately, the CocodPod SwiftLog is
already taken, so I went for SwiftLogAPI.

Modification:

Add a podspec generator.

Result:


Example output:

Pod::Spec.new do |s|
  s.name = 'SwiftLogAPI'
  s.version = '1.0.0'
  s.license = { :type => 'Apache 2.0', :file => 'LICENSE.txt' }
  s.summary = 'A Logging API for Swift.'
  s.homepage = 'https://github.com/apple/swift-log'
  s.author = 'Apple Inc.'
  s.source = { :git => 'https://github.com/apple/swift-log.git', :tag => s.version.to_s }
  s.documentation_url = 'https://github.com/apple/swift-log'
  s.module_name = 'Logging'

  s.swift_version = '5.0'
  s.cocoapods_version = '>=1.6.0'
  s.ios.deployment_target = '12.0'
  s.osx.deployment_target = '10.14'
  s.tvos.deployment_target = '12.0'

  s.source_files = 'Sources/Logging/**/*.swift'
end

@weissi weissi requested a review from tomerd April 15, 2019 10:30
@weissi weissi added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Apr 15, 2019
@weissi weissi added this to the 1.0.1 milestone Apr 15, 2019
@larryonoff
Copy link

Please see below my feedback:

  1. I don't see watchOS as a deployment target. Is it supported? I don't think that there're limitations to support watchOS.
  2. What about name 'AppleSwiftLog' or 'swift-log' instead of 'SwiftLogAPI'?

@weissi
Copy link
Member Author

weissi commented Apr 15, 2019

Please see below my feedback:

  1. I don't see watchOS as a deployment target. Is it supported? I don't think that there're limitations to support watchOS.

Oh thanks! Would you mind checking if it works? I have no idea how to build a Watch app :)

  1. What about name 'AppleSwiftLog' or 'swift-log' instead of 'SwiftLogAPI'?

Could do that, or maybe SSWGSwiftLog. @tomerd / @ktoso / @ianpartridge / @tanner0101

@ianpartridge
Copy link
Contributor

SwiftLogAPI seems fine to me - it makes it clear that this is an API package, not a backend.

@ktoso
Copy link
Member

ktoso commented Apr 15, 2019

A bit unfortunate with the module name already occupied hm... SwiftLogAPI sounds fine though.
Would we care to have all API packages in both systems adopt same naming style for this?

@weissi
Copy link
Member Author

weissi commented Apr 16, 2019

@tanner0101 you also fine with pod name SwiftLogAPI?

NOTICE.txt Outdated Show resolved Hide resolved
Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
(opinion on package name == neutral... people who actively use CocoaPods would have opinions / ideas I guess)

@allenhumphreys
Copy link
Contributor

allenhumphreys commented Apr 17, 2019

Why is the iOS/tvOS deployment target 12 and macOS 10.14? Are there any unavailable APIs in use? (I'm currently using all this source deploying to iOS 11)

In fact, I can compile all the way down to iOS 8 :-)

@weissi
Copy link
Member Author

weissi commented Apr 17, 2019

Why is the iOS/tvOS deployment target 12 and macOS 10.14? Are there any unavailable APIs in use? (I'm currently using all this source deploying to iOS 11)

Very happy to relax the version here if you tell me what the lowest version is that you could confirm actually working.

@allenhumphreys
Copy link
Contributor

@weissi I'll report back shortly

@allenhumphreys
Copy link
Contributor

@weissi Here is a git repo that has a rather large set of Apple targets. It uses a local version of the podspec with the following deployment targets:

s.ios.deployment_target = '8.0'
s.osx.deployment_target = '10.9'
s.tvos.deployment_target = '9.0'
s.watchos.deployment_target = '2.0'

iOS, tvOS, and watchOS are informed by Xcode 10.2's minimums that the GUI allows. macOS is 10.9 because Swift isn't supported with a lower deployment target.

I did not actually test on the old platforms running these versions as that could be a rather complicated endeavor. The key thing is that it compiles correctly with these deployment targets. If users actually have issues with running on those older platform versions, that'll be the most informative to any other changes I'd think.

I did verify functionality on all the latest simulators as well as my mac @ 10.14.

@allenhumphreys
Copy link
Contributor

Also wanted to mention there was a lot of conversation about the Pod name being SwiftLogAPI, but the actual module name in code is still Logging which means import Logging in actual source code which aligns with SPM integration method. 👍

@weissi
Copy link
Member Author

weissi commented Apr 17, 2019

@weissi Here is a git repo that has a rather large set of Apple targets. It uses a local version of the podspec with the following deployment targets:

Thank you so much! I updated this PR, please check :).

Also wanted to mention there was a lot of conversation about the Pod name being SwiftLogAPI, but the actual module name in code is still Logging which means import Logging in actual source code which aligns with SPM integration method. 👍

I mean we can also name the pod Logging if that's what you're suggesting, that name is still available. @tomerd / @ianpartridge / @ktoso / @tanner0101 ?

@ktoso
Copy link
Member

ktoso commented Apr 17, 2019

A bit hard for me to comment on package naming -- don't know that ecosystem very well... whichever you think works best 👍

@allenhumphreys
Copy link
Contributor

In the pods ecosystem, having module names match the pod names is pretty common if it's possible. It actually took me a few minutes to realize I shouldn't be doing import SwiftLogAPI.

@shuoli84
Copy link

@weissi I believe it is better to use Logging as package name. It will lock the name to prevent other lib use this general name. Sorry for the late comment.

@weissi
Copy link
Member Author

weissi commented Apr 23, 2019

Thanks @shuoli84 . @tomerd / @ianpartridge okay with making the CocoaPod just Logging then?

@ianpartridge
Copy link
Contributor

Fine by me. Really hope we don't have to do CocoaPods for very long!

@weissi
Copy link
Member Author

weissi commented Apr 23, 2019

@ianpartridge thanks, I updated the PR to use Logging. If @tomerd is happy with this I think I'll pull the trigger and upload it.

@tomerd
Copy link
Member

tomerd commented Apr 23, 2019

no strong feeling here, but is it worth reaching out to the owner of SwiftLog pod and see if he wants to work with us on this

@tomerd
Copy link
Member

tomerd commented Apr 23, 2019

@daltoniam ^^

scripts/build_podspec.sh Outdated Show resolved Hide resolved
Motivation:

Users ask for a CocoaPod. Unfortunately, the CocodPod `SwiftLog` is
already taken, so I went for `SwiftLogAPI`.

Modification:

Add a podspec generator.

Result:

Happier users.
@weissi
Copy link
Member Author

weissi commented Apr 25, 2019

@tomerd the SwiftLog pod has already a published version 1.0 so I'm not sure if we should use that (even if we could). What do you propose to go forward here? pod name Logging?

@tomerd
Copy link
Member

tomerd commented Apr 25, 2019

What do you propose to go forward here? pod name Logging?

yes, lets do that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CocoaPods / Carthage support
7 participants