Improve C# support, set GRPC_CSHARP_VERSION=1.28.1 #15

Falco20019 · 2020-04-15T13:21:35Z

Summary

Fixes Get C# to working #14

Changes

Update versions to latest
linux-headers is necessary as abseil is used as dependency
Use cmake instead of make (deprecated)

Notes for Reviewers

We need at least 1.27.1 of gRPC to generate C# code for proto2 syntax (used in gogo.proto). We should make sure that all clients are able to use those 1.28.1 generated classes.

gRPC 1.26.0 used Protocol Buffers 3.8.0 and gRPC 1.28.1 uses Protocol Buffers 3.11.2. So there might be some requirements to update the minimum version or Protocol Buffers in the libraries.

* GRPC uses cmake now * linux-headers is necessary as abseil is used as dependency * Use cmake instead of make (deprecated) Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@jungheinrich.de>

annanay25 · 2020-04-15T13:56:35Z

Hi @Falco20019 - Thanks for this PR!

Could you please confirm that using this image generates the same *.pb.go files in the core repo?

Falco20019 · 2020-04-15T14:20:49Z

It can't, since protobuf and therefore protoc was updated. So the generated files will change to some extend and we would have to test this for all clients. But there is no other way as C# won't work with 1.26 that we currently use. I will add path files for all languages to make testing easier.

Falco20019 · 2020-04-15T14:33:59Z

I can't upload them directly as .diff is not an allowed file extension... There is one patch file for each language.

patches.zip

Falco20019 · 2020-04-15T14:50:40Z

It's a bit problematic to force all libraries to use the same min. gRPC version.

For what I can see, these are the most significant changes between 1.26.1 and 1.28.1 (including protobuf changes):

C++
- Mostly related to experimental API
- Min. PROTOBUF_VERSION increased from 3008000 to 3011000
Go
- MarshalToSizedBuffer replaced MarshalTo
- GoGoProtoPackageIsVersion2 increased to GoGoProtoPackageIsVersion
- ErrUnexpectedEndOfGroupCollector was added
Java
- Mostly just documentation updates
JS
- Setter return the type now for chaining
- jspb.Map.deserializeBinary signature was changes (new field added)
Python
- _b replaced with b
- Experimental APIs added

Falco20019 · 2020-04-15T14:58:22Z

@yurishkuro I would propose that the library maintainers check, if their code base is usable with Protocol Buffer 3.11 as more and more users are updating their libraries.

If it's compatible without increasing the minimal protobuf version, this change should make no problems. If there are compatibility problems, we can also generate those libraries with the old protoc for now in jaeger-idl to keep the version as low as possible. But in the long run, we should be able to upgrade. And this sadly is the minimum version of gRPC / ProtoBuf that C# needs to be able to compile gogo.proto (which we don't even need but must contain to be able to generate the protos from this).

yurishkuro · 2020-04-15T16:09:10Z

I don't understand why there needs to be a cross-language dependency. I understand dependency on the common version of protoc, but gRPC version can be independent for each language, can it not?

yurishkuro · 2020-04-15T16:09:57Z

Dockerfile

+ARG GO_VERSION=1.14.2
+ARG GRPC_GATEWAY_VERSION=1.14.3
+ARG GRPC_JAVA_VERSION=1.28.1
+ARG GRPC_VERSION=1.28.1


what does this mean? Is this grpc-go version?

Yes, since only Java and Go are in separate repos, we could keep those on old versions as long as the interface to protoc is compatible. All other (cpp, csharp, python) are generated from the regular grpc/protobuf repo and bound to the version that also creates protoc as there are lots of build dependencies.

We could try to find out if it’s possible to create the static libs for protoc and use different versions of the protoc-gen-x to build. Don’t know the compatibility as especially for the c based languages, they are intended to be build at one time.

That’s why usually in C# we just use a separate build chain. But this would also require us to have the gogo.proto and so on included or to modify the files (like first done in my other PR for the client). There, we can explicitly select which compiler version we want to use through NuGET.

I spent half a day doing tests today. Since we are compiling libproto and libprotobuf from this base, all generators build on top of that use this. The regular protoc generators (cpp, csharp, java, js, objc, php, python, ruby) are already compiled into it and not distributed as plugins. For Java, only the gRPC part is a separate repository. All others are in one repository.

Technically, I can keep Java and Go gRPC on the old version, but this might break since the protos for Java will be generated against a newer version. protoc is totally fine with those generators. So the problem would only be the generated code. Only the go generators (proto + grpc) are completely separated and could be kept completely on the old version if preferred.

Right now, I'm looking into having a second build of protoc contained in the image and having the protoc-wrapper making the differentiation. A bit ugly, but seems like the only option if we do not want to have one protoc for all generators (like we do right now). It would be a lot cleaner if we could raise the minimal protocol buffers version as described before.

The differences in https://github.com/jaegertracing/docker-protobuf/pull/15/files#issuecomment-614086203 come from the common protoc, not from the different grpc versions.

Falco20019 · 2020-06-24T13:50:51Z

I sadly was not able to find a better solution in the meantime. As those changes come from the common protoc, it's hard without having different protoc versions for each language contained. Right now, all use a common protoc. I'm not sure if having multiple protoc in this container would help us, since we also include the header files in the image.

I will still try to adjust this PR to include a second protoc.

Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@jungheinrich.de>

Falco20019 · 2020-06-24T15:42:19Z

I was able to create a version that fulfills your requirement of keeping all other versions (protobuf + grpc) the same. It involves a separate protoc (as expected) just for C# and an adjusted protoc-wrapper.

yurishkuro · 2020-06-24T15:45:17Z

I wonder if it would make sense to do a bit of circular dependency and pull jaeger-idl repo during CI and try running the image against it. Otherwise it's a bit difficult to test & ascertain if the changes are good.

Falco20019 · 2020-06-24T16:04:16Z

As jaeger-idl is just a Makefile, you can adjust the PROTOC_VER to the one generated here and call make proto. Takes just a couple of seconds. Already did that test after building this docker image locally (which takes ~40-50 minutes as the old protobuf make for 3.8.0 is super slow compared to the cmake for 3.11.2...).

yurishkuro · 2020-06-24T20:32:36Z

@Falco20019 did you try using the new image to regenerate types in the main repo https://github.com/jaegertracing/jaeger/ ? It would be useful to validate that there would be no changes to generated files, especially with the version bumps.

Falco20019 · 2020-06-24T21:53:59Z

I will do a comparison tomorrow. The only changed (introduced) version now is for protoc-csharp. All other versions have been reverted in the PR. So no files other then cs should be affected. I even use the old alpine image.

Falco20019 · 2020-06-25T06:40:33Z

@yurishkuro The docker image build here is nowhere available, right? Then I need to rebuild it locally... Did a cleanup yesterday since the build increases dockers virtual disc by 12 GB on Windows due to all the intermediate containers necessary until packing...

Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@jungheinrich.de>

Falco20019 · 2020-06-25T08:11:46Z

I tested it as discussed. Here is the diff file for the changes resulting in using the old and the new image. As you can see, it only affects proto-gen-csharp. Without also using the IDL modifications from jaegertracing/jaeger-idl#63 and jaegertracing/jaeger-idl#64 the generated files will still not be usable as the annotations will be missing. But for what this PR is for, it looks good.

diff.txt

yurishkuro · 2020-06-26T00:02:16Z

You were not kidding about 45min docker build... I don't understand why these are taking so long:

Cloning into '/grpc/third_party/benchmark'...
Cloning into '/grpc/third_party/bloaty'...
Cloning into '/grpc/third_party/boringssl'...
Cloning into '/grpc/third_party/boringssl-with-bazel'...
Cloning into '/grpc/third_party/cares/cares'...
Cloning into '/grpc/third_party/envoy-api'...
Cloning into '/grpc/third_party/gflags'...
Cloning into '/grpc/third_party/googleapis'...

Perhaps because they are being done in the Docker layered file system. What if we mount a local directory from the host and run all the temp steps there?

Also, it would be useful to alter the GH Action so that Docker images from PRs are also uploaded to Docker Hub, e.g. as jaegertracing/docker-protobuf-snapshot/${commit_sha}.

NB: these are just musings, not suggestions to change this PR.

yurishkuro · 2020-06-26T00:29:41Z

Dockerfile

 COPY --from=protoc_builder /out/ /out/
+# Use protoc and plugin from protoc_cs_builder
+COPY --from=protoc_cs_builder /out/usr/bin/protoc-3.11.2.0 /out/usr/bin/protoc-csharp


where does 3.11.2 number come from?

This is the build version of the protobuf included as submodule in /third_party/protobuf. So we don't have it in any variable. But I will try to use protoc-* for the copying.

I also will try to use the -DgRPC_BUILD_GRPC_XXX_PLUGIN=OFF flags to speed up the cmake build to only generate the C# plugin. Might speed it up a couple more minutes.

Falco20019 · 2020-06-26T06:24:52Z

Perhaps because they are being done in the Docker layered file system. What if we mount a local directory from the host and run all the temp steps there?

Sorry to disappoint you, but on my local machine it also takes around 30 minutes without docker. Cloning takes some time due to a lot of submodules. And because it's 800MB total.

The main problem is, that the old Makefile + Bazel build is preeeeeeetty slow (being responsible for 30 of the 45 minutes). Building the same stuff with CMake (which only really works reliably starting with gRPC 1.28.0, see here) takes only 10 minutes. The remaining 5 minutes were building Java, Go, GoGo and packing everything together. So the whole bottleneck is having to build Protobuf 3.8.0 as part of gRPC 1.26.0.

* Disable unused plugins * Use generic version for copying protoc Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@jungheinrich.de>

yurishkuro · 2020-06-26T16:13:50Z

I did the following test:

build image from this branch: docker build . -t build
regenerate types in the main Jaeger repo: make proto JAEGER_DOCKER_PROTOBUF=build
There were no differences in the generated files.

* Update versions * GRPC uses cmake now * linux-headers is necessary as abseil is used as dependency * Use cmake instead of make (deprecated) Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@jungheinrich.de> * Use separate protoc for C# Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@jungheinrich.de> * Improved argument check Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@jungheinrich.de> * Add review comments * Disable unused plugins * Use generic version for copying protoc Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@jungheinrich.de>

Update versions

59c5ff8

* GRPC uses cmake now * linux-headers is necessary as abseil is used as dependency * Use cmake instead of make (deprecated) Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@jungheinrich.de>

Falco20019 force-pushed the update-versions branch from f02d137 to 59c5ff8 Compare April 15, 2020 13:23

Falco20019 mentioned this pull request Apr 15, 2020

Improve csharp support jaegertracing/jaeger-idl#63

Merged

yurishkuro reviewed Apr 15, 2020

View reviewed changes

Falco20019 mentioned this pull request Jun 23, 2020

Add GrpcSender for jaeger-collector jaegertracing/jaeger-client-csharp#169

Merged

Use separate protoc for C#

04457d6

Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@jungheinrich.de>

Falco20019 requested a review from yurishkuro June 24, 2020 15:42

Falco20019 mentioned this pull request Jun 25, 2020

Use Thrift 0.13.0 jaegertracing/jaeger-idl#64

Merged

Improved argument check

9e7fdd5

Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@jungheinrich.de>

yurishkuro reviewed Jun 26, 2020

View reviewed changes

Add review comments

9940864

* Disable unused plugins * Use generic version for copying protoc Signed-off-by: Kraemer, Benjamin <benjamin.kraemer@jungheinrich.de>

Falco20019 force-pushed the update-versions branch from 0da8f25 to 9940864 Compare June 26, 2020 07:17

yurishkuro approved these changes Jun 26, 2020

View reviewed changes

yurishkuro changed the title ~~Update versions & Improve C# support~~ Improve C# support, set GRPC_CSHARP_VERSION=1.28.1 Jun 26, 2020

yurishkuro merged commit 1f7a586 into jaegertracing:master Jun 26, 2020

Falco20019 deleted the update-versions branch June 26, 2020 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve C# support, set GRPC_CSHARP_VERSION=1.28.1 #15

Improve C# support, set GRPC_CSHARP_VERSION=1.28.1 #15

Falco20019 commented Apr 15, 2020 •

edited

Loading

annanay25 commented Apr 15, 2020

Falco20019 commented Apr 15, 2020

Falco20019 commented Apr 15, 2020

Falco20019 commented Apr 15, 2020

Falco20019 commented Apr 15, 2020

yurishkuro commented Apr 15, 2020

yurishkuro Apr 15, 2020

Falco20019 Apr 15, 2020 •

edited

Loading

Falco20019 Jun 24, 2020 •

edited

Loading

Falco20019 commented Jun 24, 2020

Falco20019 commented Jun 24, 2020

yurishkuro commented Jun 24, 2020

Falco20019 commented Jun 24, 2020 •

edited

Loading

yurishkuro commented Jun 24, 2020

Falco20019 commented Jun 24, 2020

Falco20019 commented Jun 25, 2020

Falco20019 commented Jun 25, 2020

yurishkuro commented Jun 26, 2020 •

edited

Loading

yurishkuro Jun 26, 2020

Falco20019 Jun 26, 2020

Falco20019 commented Jun 26, 2020 •

edited

Loading

yurishkuro commented Jun 26, 2020

Improve C# support, set GRPC_CSHARP_VERSION=1.28.1 #15

Improve C# support, set GRPC_CSHARP_VERSION=1.28.1 #15

Conversation

Falco20019 commented Apr 15, 2020 • edited Loading

Summary

Changes

Notes for Reviewers

annanay25 commented Apr 15, 2020

Falco20019 commented Apr 15, 2020

Falco20019 commented Apr 15, 2020

Falco20019 commented Apr 15, 2020

Falco20019 commented Apr 15, 2020

yurishkuro commented Apr 15, 2020

yurishkuro Apr 15, 2020

Choose a reason for hiding this comment

Falco20019 Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

Falco20019 Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

Falco20019 commented Jun 24, 2020

Falco20019 commented Jun 24, 2020

yurishkuro commented Jun 24, 2020

Falco20019 commented Jun 24, 2020 • edited Loading

yurishkuro commented Jun 24, 2020

Falco20019 commented Jun 24, 2020

Falco20019 commented Jun 25, 2020

Falco20019 commented Jun 25, 2020

yurishkuro commented Jun 26, 2020 • edited Loading

yurishkuro Jun 26, 2020

Choose a reason for hiding this comment

Falco20019 Jun 26, 2020

Choose a reason for hiding this comment

Falco20019 commented Jun 26, 2020 • edited Loading

yurishkuro commented Jun 26, 2020

Falco20019 commented Apr 15, 2020 •

edited

Loading

Falco20019 Apr 15, 2020 •

edited

Loading

Falco20019 Jun 24, 2020 •

edited

Loading

Falco20019 commented Jun 24, 2020 •

edited

Loading

yurishkuro commented Jun 26, 2020 •

edited

Loading

Falco20019 commented Jun 26, 2020 •

edited

Loading