Improve C# support, set GRPC_CSHARP_VERSION=1.28.1

Falco20019 commented 4 years ago

Summary

Fixes #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.

annanay25 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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?

Falco20019 commented 4 years ago

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.

Falco20019 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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...

Falco20019 commented 4 years ago

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 https://github.com/jaegertracing/jaeger-idl/pull/63 and https://github.com/jaegertracing/jaeger-idl/pull/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 commented 4 years ago

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.

Falco20019 commented 4 years ago

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.

yurishkuro commented 4 years ago

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.

jaegertracing / docker-protobuf