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

Update bundled gRPC to 1.18.0 and nanopb to 0.3.6 #7368

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@tetromino
Copy link
Contributor

tetromino commented Feb 6, 2019

And take the opportunity to align third_party/grpc/BUILD closer with
gRPC upstream's BUILD file to make importing easier next time.

Fixes #2804.

@tetromino tetromino requested a review from lberki Feb 6, 2019

@googlebot googlebot added the cla: yes label Feb 6, 2019

@tetromino tetromino force-pushed the tetromino:grpc-v1.18.0 branch from 63ae146 to 4652bd1 Feb 6, 2019

@tetromino tetromino self-assigned this Feb 6, 2019

@lberki

lberki approved these changes Feb 7, 2019

Copy link
Contributor

lberki left a comment

Wow! I'd ask why not the latest nanopb version, but far be it from the to let the perfect be the enemy of the good :)

@tetromino

This comment has been minimized.

Copy link
Contributor Author

tetromino commented Feb 7, 2019

Nanopb 0.3.6 is what the gRPC upstream currently bundles with their code. I thought we should use the same for consistency.

@kwasimensah

This comment has been minimized.

Copy link

kwasimensah commented Feb 7, 2019

A couple of non-blocking questions about how these deps are currently set up

  1. How are there not ODR issues with Bazel and GRPC using possibly different versions of protobuf?

  2. Isn't there also a code bloat/longer build times from having two different versions of it linked in? This is a desktop app so I don't think anyone truly cares but it seems unnecessary.

tetromino added some commits Feb 7, 2019

Add explicit dependency on protoc_lib in cc_grpc_library
Required prerequisite for updating bundled gRPC to 1.18.
Update bundled gRPC to 1.18.0 and nanopb to 0.3.6
And take the opportunity to align third_party/grpc/BUILD closer with
gRPC upstream's BUILD file (by adopting the same intermediate targets)
to make importing easier next time.

The MSYS2 patch is no longer needed judging by discussion in
https://groups.google.com/forum/#!msg/grpc-io/gd6sIuo6rjQ/eyEFarhABgAJ

Fixes #2804.

@tetromino tetromino force-pushed the tetromino:grpc-v1.18.0 branch from 4652bd1 to d0db33b Feb 7, 2019

@tetromino

This comment has been minimized.

Copy link
Contributor Author

tetromino commented Feb 7, 2019

@kwasimensah - the bundled third_party/grpc should be using the same third_party/protobuf and third_party/nanopb as the other of c++ code in bazel. At least, that's how the modified third_party/grpc/BUILD file is set up.

@tetromino

This comment has been minimized.

Copy link
Contributor Author

tetromino commented Feb 7, 2019

Updating to split third_party/ and tools/ changes into separate commits as required by review processes. (And changing the grpc_cpp_plugin label - unfortunately, deviating from gRPC upstream - to make this order of commits possible.)

@tetromino

This comment has been minimized.

Copy link
Contributor Author

tetromino commented Feb 7, 2019

Commits got applied but in the wrong order due to review process complications; I do apologize for breaking master for 3 commits (458ab03, 6267510, 4c3aeb0).

All should be ok now.

@tetromino tetromino closed this Feb 7, 2019

@tetromino tetromino deleted the tetromino:grpc-v1.18.0 branch Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.