chore: update c++ base images #6073
Conversation
jcferretti
added
NoDocumentationNeeded
NoReleaseNotesNeeded
| // Need to disable until the proto base image can catch up to protobuf 28.2 | ||
| tasks.getByName('compareProtobuf').enabled = false |
There was a problem hiding this comment.
I'm confused why this is necessary; do we not use the same code to generate the c++ protobuf that we use to compare it?
There was a problem hiding this comment.
The proto targets generate code for all languages with the protoc image. The C++ build has never used that output, there is a separate script
that is used to generate code under cpp-client/deephaven/proto. This code in the cpo-client/build.gradle was added to force both to be consistent. But given the current issues in protobuf versions across languages and the state of chaos the protubuf team created with their breaking changes, this is not advisable anymore. It actually never was advisable; there is no reason to force different languages to use the same proto version. We arguably should not be generating C++ code if it is not used, but forcing something generated and not used to be consistent with something that is used and works and pass tests is not the solution to that IMHO
There was a problem hiding this comment.
Went back and forth with Cristian a bit on this, and my understanding is that we previously had two ways to generate cpp code from protos, and they happened to have the same output - proto/proto-backplane-grpc's gradle/docker wiring, and the build-cpp-protos.sh script.
The fact that they generated the same output was wonderful, but ultimately irrelevant - the build-cpp-protos.sh script is going to use the protoc in the cpp docker image, and so is "more" correct.
My proposal is that this merge as-is, with a new issue to be filed to follow-up here to restore this task. Both the updateProto and compareProto tasks should instead read from build-cpp-protos.sh output in its own docker task, and the rest of the cpp codegen in proto/proto-backplane-grpc should be removed.
We can revert this later and return to proto/proto-backplane-grpc generating cpp bindings, but if we do, we should at the same time delete build-cpp-protos.sh so there is only one way to generate this code. I'm not sure this is a step worth taking, especially given the current protobuf versioning ... scheme.
| // Need to disable until the proto base image can catch up to protobuf 28.2 | ||
| tasks.getByName('compareProtobuf').enabled = false |
There was a problem hiding this comment.
Went back and forth with Cristian a bit on this, and my understanding is that we previously had two ways to generate cpp code from protos, and they happened to have the same output - proto/proto-backplane-grpc's gradle/docker wiring, and the build-cpp-protos.sh script.
The fact that they generated the same output was wonderful, but ultimately irrelevant - the build-cpp-protos.sh script is going to use the protoc in the cpp docker image, and so is "more" correct.
My proposal is that this merge as-is, with a new issue to be filed to follow-up here to restore this task. Both the updateProto and compareProto tasks should instead read from build-cpp-protos.sh output in its own docker task, and the rest of the cpp codegen in proto/proto-backplane-grpc should be removed.
We can revert this later and return to proto/proto-backplane-grpc generating cpp bindings, but if we do, we should at the same time delete build-cpp-protos.sh so there is only one way to generate this code. I'm not sure this is a step worth taking, especially given the current protobuf versioning ... scheme.
No description provided.