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

[discussion] Deprecating "SwiftGRPC" / switching to the NIO version #526

Closed
9 of 11 tasks
glbrntt opened this issue Jul 24, 2019 · 28 comments
Closed
9 of 11 tasks

[discussion] Deprecating "SwiftGRPC" / switching to the NIO version #526

glbrntt opened this issue Jul 24, 2019 · 28 comments
Labels

Comments

@glbrntt
Copy link
Collaborator

glbrntt commented Jul 24, 2019

We need to a more concrete plan on how we are going to support SwiftGRPC and switch to the NIO based version going forwards and when we will release a v1.0.0 of the NIO version.

I believe the general consensus so far is:

  • Iterating on the current nio branch with pre-release tags (1.0.0-alpha.1, ...) until we are happy with the content and API (and have resolved the issues in ☂️ Issue Checklist for GRPC v1.0 #492 below)
  • Announcing the NIO version in the README of the current master branch to give users some time to switch (~1 month ahead of time)
  • Providing a migration guide so that they can switch more easily (this might not be so extensive, given the APIs are inherently different)
  • Renaming the current master branch to cgrpc
  • Renaming the current nio branch to master
  • Accepting only bug fixes to the cgrpc branch
  • Feature requests may be accepted if we have a dedicated maintainer for the cgrpc branch
  • Future releases of cgrpc will remain on the 0.x series (the NIO variant will take 1.y)

EDIT: Inlining the contents of #492:

@MrMage
Copy link
Collaborator

MrMage commented Jul 24, 2019

Sounds good to me. From my point, we could start mentioning the new branch in the README right now; at this point, the new API should be sufficiently usable. It might make sense to have the "switching guide" ready before that, though. @timburks @rebello95, do you agree?

We could also cut a new 0.x release that introduces a temporary compiler warning asking users to switch to the NIO branch. That could be removed a month later to not offend users who need to stay on the cgrpc branch.

Providing a migration guide so that they can switch more easily (this might not be so extensive, given the APIs are inherently different)

I guess we could just list 10 old/new examples for the following use cases:

  • One each for connecting/starting a server
  • One each for making (client)/handling (server) each call type

Additional blockers for 1.0 might be CocoaPods, though. I haven't seen a Podspec in the SwiftNIO repo; if they had that, our own Podspec would probably easy enough.

We should also reach out to our Carthage users (@JonasVautherin, @WilliamIzzo83, @byuarus) whether they are interested in getting Carthage to work on the nio branch before that becomes the new master.

@glbrntt
Copy link
Collaborator Author

glbrntt commented Jul 24, 2019

We could also cut a new 0.x release that introduces a temporary compiler warning asking users to switch to the NIO branch. That could be removed a month later to not offend users who need to stay on the cgrpc branch.

Fine with me, although it's not ideal for users who have warnings as errors enabled. Not sure if dependency warnings count here or if there's a mechanism to disable a particular warning. Pinning the dependency to release without the warning is a temporary fix if nothing else suffices, however.

Providing a migration guide so that they can switch more easily (this might not be so extensive, given the APIs are inherently different)

I guess we could just list 10 old/new examples for the following use cases:

  • One each for connecting/starting a server
  • One each for making (client)/handling (server) each call type

Do you mean a side-by-side example for each? I think we have most examples covered in the README so we can just pull those out and add whatever is missing.

Additional blockers for 1.0 might be CocoaPods, though. I haven't seen a Podspec in the SwiftNIO repo; if they had that, our own Podspec would probably easy enough.

They have a script to generate podspecs. I'll add providing Podspec that to the checklist.

@JonasVautherin
Copy link
Contributor

We should also reach out to our Carthage users (@JonasVautherin, @WilliamIzzo83, @byuarus) whether they are interested in getting Carthage to work on the nio branch before that becomes the new master.

So with the NIO version removing the dependency on CgRPC, I'm wondering if we should not move to xcodegen. From what I could see on Swift and Carthage discussions, my feeling is that SwiftPM may never be compatible with Carthage, meaning that library maintainers will need to support CocoaPods, Carthage and SwiftPM for a long time. In my limited experience with xcodegen, I would hope for Carthage to move towards supporting that one day. Opinions?

@MrMage
Copy link
Collaborator

MrMage commented Jul 24, 2019

I've heard that Xcode 11 will make it easy to add SwiftPM packages to any Xcode project, including iOS ones. Maybe at that point we wouldn't need Carthage support at all?

@WilliamIzzo83
Copy link
Contributor

I've heard that Xcode 11 will make it easy to add SwiftPM packages to any Xcode project, including iOS ones. Maybe at that point we wouldn't need Carthage support at all?

I think this should be the natural evolution. With xcode support I think carthage and cocoapods support can be safely interrupted.

@Gurpartap
Copy link
Contributor

Gurpartap commented Jul 24, 2019

I've heard that Xcode 11 will make it easy to add SwiftPM packages to any Xcode project, including iOS ones. Maybe at that point we wouldn't need Carthage support at all?

Swift Package Manager UI in Xcode 11:

Swift Package Manager UI in Xcode 11

SwiftPM File Menu

With the advent of Swift Package Manager in Xcode 11, I personally don't know of a reason to use Carthage. The grpc-swift NIO client just works with SwiftPM, and is even easier than dealing with custom Podspec necessary with grpc-objc client.

@WilliamIzzo83
Copy link
Contributor

  • Renaming the current nio branch to master

Can't we just merge nio onto master? This way we know that up until a certain commit hash we have the "old" version, and from then on there is the new one.

@JonasVautherin
Copy link
Contributor

With the advent of Swift Package Manager in Xcode 11, I personally don't know of a reason to use Carthage.

Well, I'd say all the legacy projects relying on Carthage? 😄

@MrMage
Copy link
Collaborator

MrMage commented Jul 24, 2019

With the advent of Swift Package Manager in Xcode 11, I personally don't know of a reason to use Carthage.

Well, I'd say all the legacy projects relying on Carthage? 😄

The NIO switch will require huge changes, anyway. What would stop them from switching to including SwiftGRPC as a SwiftPM dependency while at it?

@MrMage
Copy link
Collaborator

MrMage commented Jul 24, 2019

BTW, I'd also suggest to apply as a Swift Server Working Group Project, either shortly before or after releasing 1.0. Before would have the advantage of gathering feedback while we can still make breaking changes.

@rebello95
Copy link
Collaborator

Sounds good to me. From my point, we could start mentioning the new branch in the README right now; at this point, the new API should be sufficiently usable. It might make sense to have the "switching guide" ready before that, though. @timburks @rebello95, do you agree?

Yep definitely think this is a good idea.

We could also cut a new 0.x release that introduces a temporary compiler warning asking users to switch to the NIO branch. That could be removed a month later to not offend users who need to stay on the cgrpc branch.

Fine with me, although it's not ideal for users who have warnings as errors enabled. Not sure if dependency warnings count here or if there's a mechanism to disable a particular warning. Pinning the dependency to release without the warning is a temporary fix if nothing else suffices, however.

I am also concerned about the Warnings as errors case. What about logging when gRPC starts up instead?

@JonasVautherin
Copy link
Contributor

The NIO switch will require huge changes, anyway. What would stop them from switching to including SwiftGRPC as a SwiftPM dependency while at it?

Some projects don't have a choice, and rely on more dependencies than only SwiftPM. Supporting Carthage means "having an xcodeproj on the repo" and "having a Cartfile declaring the dependencies". If you drop that, projects relying on Carthage won't be able to use gRPC-Swift anymore. I can believe that SwiftPM is the best and all, but it doesn't mean that all the Carthage projects online will magically move to SwiftPM when gRPC-Swift-NIO 1.0 gets released 😅. Such a process will take time, and I believe good libraries should continue supporting legacy systems during the transition period.

@MrMage
Copy link
Collaborator

MrMage commented Jul 25, 2019 via email

@WilliamIzzo83
Copy link
Contributor

The NIO switch will require huge changes, anyway. What would stop them from switching to including SwiftGRPC as a SwiftPM dependency while at it?

Some projects don't have a choice, and rely on more dependencies than only SwiftPM. Supporting Carthage means "having an xcodeproj on the repo" and "having a Cartfile declaring the dependencies". If you drop that, projects relying on Carthage won't be able to use gRPC-Swift anymore. I can believe that SwiftPM is the best and all, but it doesn't mean that all the Carthage projects online will magically move to SwiftPM when gRPC-Swift-NIO 1.0 gets released 😅. Such a process will take time, and I believe good libraries should continue supporting legacy systems during the transition period.

I don't think it's a problem: the Cartfile will specify the version, which point to a certain commit, where the xcodeproj and everything else is set up as expected. Also by introducing a legacy branch where bug fixes on legacy are addressed, the cartfile can point to that branch.

I don't see much of a problem if master goes forward with nio

@MrMage
Copy link
Collaborator

MrMage commented Jul 25, 2019

Do you mean a side-by-side example for each? I think we have most examples covered in the README so we can just pull those out and add whatever is missing.

Exactly.

I am also concerned about the Warnings as errors case. What about logging when gRPC starts up instead?

That makes sense, let's log an error at startup instead.

@JonasVautherin
Copy link
Contributor

What would stop them from using SwiftPM for SwiftGRPC and Carthage for old dependencies?

Also, they could keep using the legacy version indefinitely with Carthage.

Alright, let's not debate it here. I'll see what I can do when the time comes.

@MrMage
Copy link
Collaborator

MrMage commented Jul 25, 2019

Should we merge this checklist with the one in #492? /cc @glbrntt

@glbrntt
Copy link
Collaborator Author

glbrntt commented Jul 26, 2019

Should we merge this checklist with the one in #492? /cc @glbrntt

Sure.

@glerchundi
Copy link

Can any give a status update on this, especially @glbrntt? We plan to start a project in the coming weeks and knowing an expected timeline would help deciding what to do.

Thanks!

@ydnar
Copy link

ydnar commented Oct 31, 2019

@glerchundi we’ve been using the nio branch in our iOS app via Swift Package Manager for a couple months. So far, so good.

@glerchundi
Copy link

glerchundi commented Oct 31, 2019 via email

@glbrntt
Copy link
Collaborator Author

glbrntt commented Nov 1, 2019

Thanks for the ping on this @glerchundi - I definitely dropped the ball on updates, sorry about that!

Here's the current status of things:

  • We made a proposal to the SSWG (Swift Server Work Group) to have gRPC Swift listed as a recommended project (if you're interested you can check it out here). I think the feedback from that community is really important to ensure that this project is successful. The upshot of this is that there may be suggestions around some of the APIs, so we really need to finish this process before we release 1.0.0 (since API changes are a major version change). If there are any API changes before we release 1.0.0 we'll make sure we communicate them in release notes and provide shims so you can apply fixits in Xcode wherever possible.
  • In terms of features and stability I think gRPC Swift is in great shape and you shouldn't have any problems there. (But please let us know if you do!)

In terms of timelines it's very hard to say, but I'd be very disappointed if we didn't manage to tag 1.0.0 in 2019!

@glerchundi
Copy link

Thanks for the ping on this @glerchundi - I definitely dropped the ball on updates, sorry about that!

Really appreciate you taking the time for keeping us updated so nothing to sorry about!

  • We made a proposal to the SSWG (Swift Server Work Group) to have gRPC Swift listed as a recommended project (if you're interested you can check it out here). I think the feedback from that community is really important to ensure that this project is successful. The upshot of this is that there may be suggestions around some of the APIs, so we really need to finish this process before we release 1.0.0 (since API changes are a major version change). If there are any API changes before we release 1.0.0 we'll make sure we communicate them in release notes and provide shims so you can apply fixits in Xcode wherever possible.

Sounds like a really good strategy in order to have as stable as possible API before it lands in master.

  • In terms of features and stability I think gRPC Swift is in great shape and you shouldn't have any problems there. (But please let us know if you do!)

Good to know, will told you something once we make progress on using it!

In terms of timelines it's very hard to say, but I'd be very disappointed if we didn't manage to tag 1.0.0 in 2019!

Now that nobody is listening, thanks for sharing your expected timeline ;)

@kmarcell
Copy link

kmarcell commented Jan 6, 2020

I would like to ask about the new NIO version, now thatServerStreamingCall<RequestMessage: Message, ResponseMessage: Message> doesn't have receive and send how should we handle back pressure?

Am I missing something? Is the old (current) version not getting any support after 1.0.0 is released?

@MrMage
Copy link
Collaborator

MrMage commented Jan 6, 2020

The old version will not receive any new features after 1.0 is released. Also, I'm not sure whether it handles backpressure much better than the new version. Also, I think SwiftNIO has or had a backpressure channel handler; maybe @Lukasa can comment on that.

But besides that, you still provide a closure that is called when a new message is received, and if desired, send new messages in that closure. That should be somewhat equivalent to calling receive and send in an alternating fashion with the old version.

@Lukasa
Copy link
Collaborator

Lukasa commented Jan 6, 2020

NIO has an extremely simple backpressure handler, but it’s usually better to construct backpressure handling at a higher level if possible. In this case it absolutely should be possible to do that, as the relevant NIO components can propagate the backpressure, but we probably do need to design something that would affect the gRPC API. @glbrntt, thoughts?

@glbrntt
Copy link
Collaborator Author

glbrntt commented Jan 6, 2020

we probably do need to design something that would affect the gRPC API. @glbrntt, thoughts?

Most likely, yes. I'm not sure how backpressure is implemented for other languages, I'll have a look into this.

@MrMage
Copy link
Collaborator

MrMage commented Mar 19, 2020

We have officially moved to the NIO version now 🎉

@MrMage MrMage closed this as completed Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants