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 CocoaPods support #5

Merged
merged 6 commits into from
Sep 28, 2016
Merged

Add CocoaPods support #5

merged 6 commits into from
Sep 28, 2016

Conversation

radex
Copy link
Contributor

@radex radex commented Sep 24, 2016

This resolves #1.

There's just one problem before this can just work: I named the Pod "SwiftProtobufRuntime", but the generated .pb.swift files do import Protobuf — they think this library should be called just "Protobuf".

There's a few possible solutions:

  1. I could rename the Pod to "Protobuf". However, there already exists a Pod named Protobuf. If Apple isn't interested in pushing the pod to the CocoaPods trunk, this would work fine. But it would still be confusing to users, and if someone has an existing iOS app that uses the Protobuf pod, they couldn't also use this library.
  2. An option could be added to swift-protobuf-plugin to customize the generated import, so people using CocoaPods or already using Protobuf in their apps (with other libraries) could resolve the naming conflict
  3. The runtime module name could be changed to something unambiguous, like ProtobufSwift or ProtobufRuntime for everyone.
  4. I think it should be possible to customize the Podspec so that a module name "Protobuf" is generated, while the Pod itself is still called "SwiftProtobufRuntime" — but I'm still working out how to do that exactly ;)

WDYT?

@jcanizales
Copy link

My 2c:

  • Option 1 would indeed be confusing :)
  • Option 4 is possible in ObjC and C with the podspec attribute module_name. I haven't tried it with Swift sources, but I imagine it works the same. It's what I did for gRPC-Core.podspec and BoringSSL.podspec, that needed to generate frameworks called grpc and openssl respectively. It still would make it impossible to use the Swift and ObjC runtimes in the same app, which is unfortunate.
  • I thought option 2 would face the same problem as the ObjC version: Because the runtime includes generated protos (empty.proto etc.), that fixes the name of the module. But none of those generated files import Protobuf! Maybe they were hand-edited after generation?

My opinion is ProtobufSwift would be the simplest option. We already have libraries named like RxSwift, so it wouldn't be an outlier.

@radex radex changed the title [WIP] Add CocoaPods support Add CocoaPods support Sep 25, 2016
@radex
Copy link
Contributor Author

radex commented Sep 25, 2016

Okay, I added module_name option to the Podspec (as @jcanizales and a few others suggested), so the Pod generates a module name "Protobuf" and it just works.

I'd still suggest considering options (2) and (3), but the PR is mergeable as it is ;)

@tbkka
Copy link
Collaborator

tbkka commented Sep 26, 2016

@jcanizales : The generated files in the runtime library are in fact edited to remove the import statements. See https://github.com/apple/swift-protobuf-runtime/blob/master/Makefile#L95

@tbkka
Copy link
Collaborator

tbkka commented Sep 26, 2016

I'll see about renaming the module to "SwiftProtobuf". That will be a little disruptive in the short-term, but seems the best long-term fix. I'd like to do that before we merge this, if that's okay. I'll get to it soon.

@tbkka
Copy link
Collaborator

tbkka commented Sep 27, 2016

I just committed the name change; the module is now called 'SwiftProtobuf'.

Please update your merge request accordingly and I'll merge it.

Thanks!

@radex
Copy link
Contributor Author

radex commented Sep 27, 2016

Thank you — 'SwiftProtobuf' makes a ton of sense!

I updated the Podspec and Readme — should be good to merge!

@tbkka tbkka merged commit ccd6522 into apple:master Sep 28, 2016
@tbkka
Copy link
Collaborator

tbkka commented Sep 28, 2016

I've merged this, but we may need to update it after Issue #8 gets finished.

@radex radex mentioned this pull request Sep 28, 2016
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.

[question] init(protobuf: )
3 participants