Skip to content

[grpc][upb] Update to newer versions#17422

Merged
ras0219-msft merged 8 commits intomicrosoft:masterfrom
ras0219:dev/roschuma/grpc
Apr 23, 2021
Merged

[grpc][upb] Update to newer versions#17422
ras0219-msft merged 8 commits intomicrosoft:masterfrom
ras0219:dev/roschuma/grpc

Conversation

@ras0219
Copy link
Copy Markdown
Contributor

@ras0219 ras0219 commented Apr 21, 2021

This PR rolls together the changes from #15078 and #17151 which each have issues on their own.

@coryan
Copy link
Copy Markdown
Contributor

coryan commented Apr 22, 2021

TL;DR; there is code missing in upb that gRPC expects is found in its "vendored" version of upb.

AFAICT, the build problems on google-cloud-cpp are caused by missing symbols in the gRPC and upb libraries. It just happens that google-cloud-cpp wants to link upb and that is what breaks.

To recap: this PR is trying to gRPC to 1.36.4 and upb to the corresponding version. I say "corresponding" because gRPC has a copy of upb (not a submodule). This copy builds the upb library using more than just the coded included with upb, gRPC also compiles code generated by upb:

https://github.com/grpc/grpc/blob/3e53dbe8213137d2c731ecd4d88ebd2948941d75/CMakeLists.txt#L3798

The generator for these files cannot be compiled with upb's CMake files:

"However there is no support for building the upb compiler or for generating .upb.c/upb.h files. "

https://github.com/protocolbuffers/upb/blob/master/cmake/README.md

Meanwhile gRPC includes these generated files in its source, e.g.:

https://github.com/grpc/grpc/tree/master/src/core/ext/upb-generated/google/protobuf
https://github.com/grpc/grpc/tree/master/src/core/ext/upbdefs-generated/google/protobuf

That is, gRPC includes a copy of upb and a copy of files generated by upb. I think we would be shaving a very large yak if we insist of using "unvendoring" upb from gRPC.

We may find more success if we used the upb that is bundled with gRPC. This creates a problem, as now you cannot link both upb and gRPC in a vcpkg-based project. That seems fraught with other dangers and I hesitate to send a patch to do so without some discussion.

I created an (incomplete) patch, it does not work, but has fewer missing symbols:

https://github.com/coryan/vcpkg/commit/c2ce02aee2fb9f344212f90eae2978975a8cf336

PS: I do not understand gRPC's rationale to adopt upb when upb is so immature with respect to packaging and stability (as compared to gRPC itself), but that is probably just my ignorance of the other issues facing the gRPC team.

@JackBoosY JackBoosY added category:port-update The issue is with a library, which is requesting update new revision info:internal labels Apr 22, 2021
@ras0219
Copy link
Copy Markdown
Contributor Author

ras0219 commented Apr 22, 2021

Thanks for the description -- that really helped to understand what's going on!

Obviously, it would be best if upb was more mature and it was clearer how to generate sources plus where those sources should live, however for now I think the approach I've pushed in 39d2b38 is the best we have available.

Essentially, I've added a small static library to grpc's build that builds the checked-in generated files. There is the possibility of these being out-of-sync with the underlying upb library, but until we're able to generate the files ourselves I don't see a better option.

Now that we have host dependencies, it wouldn't be too terrible to do the generation in vcpkg if I knew where the original .proto files were + how to build the generator.

@ras0219 ras0219 marked this pull request as ready for review April 22, 2021 20:00
@JackBoosY JackBoosY added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Apr 23, 2021
@ras0219-msft ras0219-msft merged commit e4096d6 into microsoft:master Apr 23, 2021
@ras0219-msft ras0219-msft deleted the dev/roschuma/grpc branch April 23, 2021 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-update The issue is with a library, which is requesting update new revision info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[upb] update to 2020-12-19

6 participants