Skip to content

Conversation

@tsabirgaliev
Copy link
Contributor

@tsabirgaliev tsabirgaliev commented Aug 25, 2019

Motivation: gRPC v1.23.0 enables CFStream by default on iOS. This feature potentially can solve connectivity problems (#337)

Copy link
Collaborator

@MrMage MrMage left a comment

Choose a reason for hiding this comment

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

Upon a cursory look, this looks good. Were there any manual changes besides the redundant include that you needed to do, so we can document them?

@MrMage MrMage requested a review from rebello95 August 26, 2019 10:48
@MrMage
Copy link
Collaborator

MrMage commented Aug 26, 2019

I'd also like for @rebello95 to take a look before merging this.

@tsabirgaliev
Copy link
Contributor Author

@MrMage

Were there any manual changes besides the redundant include that you needed to do, so we can document them?

Nope. 9d8ab80 is the result of ./vendor-all.sh v1.23.0 and is reproducible (at least on my machine, I re-did it on clean HEAD a couple of times before committing). The only manual changes are in 12dbc2e and 338b680

@MrMage
Copy link
Collaborator

MrMage commented Aug 27, 2019

@tsabirgaliev thank you! Happy to merge this once @rebello95 approves as well.

@Westacular
Copy link

This implies it's safe to update the dependency on gRPC-Core in SwiftGRPC.podspec to ~> 1.23.0 as well. Should that also happen as part of this PR?

@tsabirgaliev
Copy link
Contributor Author

@Westacular is Pods the only package spec shipped here? I’m afraid I’m not enough competent to cover them all :)

@MrMage
Copy link
Collaborator

MrMage commented Aug 29, 2019 via email

@MrMage
Copy link
Collaborator

MrMage commented Aug 29, 2019 via email

@tsabirgaliev
Copy link
Contributor Author

Or just look at the previous update PR

@MrMage haha, good catch :) it is exactly what I did. I even copy-pasted the commit messages.

Copy link
Collaborator

@MrMage MrMage left a comment

Choose a reason for hiding this comment

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

I think we should indeed update the Podspec and the Carthage project in this PR as well.

@tsabirgaliev
Copy link
Contributor Author

@MrMage do we need to also bump the grpc-swift version in Podspec?

@MrMage
Copy link
Collaborator

MrMage commented Sep 2, 2019

@MrMage do we need to also bump the grpc-swift version in Podspec?

No, one of the team will do that when releasing the next version of Swift gRPC.

@tsabirgaliev
Copy link
Contributor Author

@MrMage I'm sorry, I don't get how to update the Carthage project. Is it just make project-carthage in this case?

@MrMage
Copy link
Collaborator

MrMage commented Sep 2, 2019

@MrMage I'm sorry, I don't get how to update the Carthage project. Is it just make project-carthage in this case?

Yes, that should be it.

@MrMage
Copy link
Collaborator

MrMage commented Sep 2, 2019

Thank you for the changes, @tsabirgaliev! Once the next release is cut, the library will officially use gRPC-Core 1.23. Until then, you can already use it by using the master branch.

@MrMage MrMage merged commit 44f194e into grpc:master Sep 2, 2019
@rebello95
Copy link
Collaborator

Thanks for doing this! Sorry I was unable to review - was OOO last week.

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.

4 participants