-
Notifications
You must be signed in to change notification settings - Fork 420
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 Cocoapod Support for SwiftGRPC #764
Add Cocoapod Support for SwiftGRPC #764
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.
Thanks @Jake-Prickett -- great start!
Would it be better to leverage something similar to what is done in SwiftNIO build_podspecs.sh?
Yes: I think it would make life easier in the long run.
Who would be the best people to add as Maintainers for the CGRPCZlib CocoaPod?
You (if you don't mind!), me, @MrMage and @Lukasa
The maintainers for the gRPC Swift CocoaPod should be the same.
15840a7
to
2d42af7
Compare
2d42af7
to
cb338e8
Compare
0a0a00d
to
4a7dd5e
Compare
I'm not sure this is related to this PR but the license file referenced in https://github.com/CocoaPods/Specs/blob/master/Specs/1/e/a/CGRPCZlib/1.0.0-alpha.11/CGRPCZlib.podspec.json is not right. It should be
Thank you |
Good catch @SebastianThiebaud! I noticed the same thing when working through the Podspec Generation script. I've made the changes in script so it won't appear in the future and believe that should be fixed with the next release :) |
Awesome. That's what I thought, just wanted to make sure this script would also apply to |
4a7dd5e
to
2741421
Compare
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.
Looks great @Jake-Prickett, there are a couple of minor issues to fix but I'm happy with it otherwise!
Do you plan to also add the resulting Podspec file to the repo? If so, should that be part of this PR? |
@glbrntt - Comments have been addressed, thank you for the review! @MrMage - I was not planning on adding the Podspec file to the repository. The goal of the script is to generate the Podspec files ( |
I prefer to have the Podspec file in the repo. This is common practice among most packages, allows for quick inspection of the file by anyone, and having it in version control allows us to better review any changes to it. |
I'm not opposed to having them checked in in addition to the script. To that end it would be useful if the script could take a path argument indicating where the podspecs should be generated. |
7dac322
to
177dea2
Compare
@MrMage @glbrntt That sounds like a fair middle ground. 👍 Added a path parameter (
or
|
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.
Spotted a small bug which I missed last time around (it's been a while since I looked at Python...)
Hey every one. Me also need grps for cocoapods. |
Hi @mefilt, the gRPC-Swift CocoaPod is available now if you would like to consume 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.
This is awesome; thanks for working on this @Jake-Prickett! Over to you @MrMage.
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.
Thank you for all the work that went into this!
Just nits; feel free to push back if you disagree with any of them.
s.module_name = 'CGRPCZlib' | ||
s.version = '1.0.0-alpha.11' | ||
s.license = { :type => 'Apache 2.0', :file => 'LICENSE' } | ||
s.summary = 'Swift gRPC code generator plugin and runtime library' |
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.
Should CGRPCZlib have a different summary?
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 can, I was thinking having something more generic that might fit both would keep the generation script simple. What do you think @MrMage? I'm open to either way 👍
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.
Personally I would use a slightly different description for each, to avoid confusing anyone who looks up either package at the CocoaPods site. I understand that would need a bit of additional code, though.
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 agree, that does make sense. Would you mind if I took that one up as a follow-up item and address it separately?
a23777f
to
feb557a
Compare
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.
No mandatory changes from my side; feel free to change the pod descriptions in a follow-up PR.
Leaving the merge to @glbrntt.
CGRPCZlib.podspec
to enable the gRPC-Swift CocoaPodgRPC-Swift.podspec
🎉