RSDK-2113 Align buf remote plugin proto version with local #224
Conversation
| @@ -339,6 +339,8 @@ find_package(gRPC ${VIAMCPPSDK_GRPC_VERSION_MINIMUM} CONFIG) | |||
| if (gRPC_FOUND) | |||
| get_target_property(VIAMCPPSDK_GRPC_CPP_PLUGIN gRPC::grpc_cpp_plugin LOCATION) | |||
| set(VIAMCPPSDK_PROTOBUF_VERSION ${Protobuf_VERSION}) | |||
| set(VIAMCPPSDK_PROTOBUF_VERSION_MAJOR ${Protobuf_VERSION_MAJOR}) | |||
There was a problem hiding this comment.
Buf's remote plugin versioning for protobuf only has major.minor, unlike grpc which has major.minor.patch. Compare the version drop-downs for:
So we need to break out major/minor for protobuf so we can interpolate them correctly. For the find_package case we get those variables set for us, but see below for the pkg-config case.
| @@ -366,6 +368,8 @@ else() | |||
| pkg_check_modules(GRPCXX REQUIRED grpc++>=${VIAMCPPSDK_GRPC_VERSION_MINIMUM}) | |||
|
|
|||
| set(VIAMCPPSDK_PROTOBUF_VERSION ${PROTOBUF_VERSION}) | |||
| string(REGEX REPLACE "^([0-9]+)\.([0-9]+)\.([0-9]+)$" "\\1" VIAMCPPSDK_PROTOBUF_VERSION_MAJOR ${VIAMCPPSDK_PROTOBUF_VERSION}) | |||
There was a problem hiding this comment.
When we obtain protobuf via pkg_check_modules it doesn't break out the major/minor versions for us, so we need to do that ourselves.
| @@ -394,6 +398,11 @@ else() | |||
| list(PREPEND VIAMCPPSDK_GRPCXX_REFLECTION_LIBRARIES ${VIAMCPPSDK_GRPCXX_REFLECTION_LIB}) | |||
| endif() | |||
|
|
|||
| # These are the minimum versions of gRPC and protobuf for which remote plugins with buf are supported. | |||
There was a problem hiding this comment.
I lifted this check into a new variable because we need this test in more than one place.
| @@ -49,7 +49,7 @@ RUN apt-get -y install cmake | |||
| RUN mkdir -p ${HOME}/opt/src | |||
|
|
|||
| RUN cd ${HOME}/opt/src && \ | |||
| git clone --recurse-submodules -b v1.52.0 --depth 1 --shallow-submodules https://github.com/grpc/grpc && \ | |||
| git clone --recurse-submodules -b v1.62.1 --depth 1 --shallow-submodules https://github.com/grpc/grpc && \ | |||
There was a problem hiding this comment.
This is the only docker image where we build grpc/protobuf from source so it makes sense to keep up with the latest stable on it, since we can.
| @@ -4,5 +4,5 @@ deps: | |||
| - remote: buf.build | |||
| owner: googleapis | |||
| repository: googleapis | |||
| commit: 7e6f6e774e29406da95bd61cdcdbc8bc | |||
| digest: shake256:fe43dd2265ea0c07d76bd925eeba612667cf4c948d2ce53d6e367e1b4b3cb5fa69a51e6acb1a6a50d32f894f054a35e6c0406f6808a483f2752e10c866ffbf73 | |||
| commit: ee48893a270147348e3edc6c1a03de0e | |||
There was a problem hiding this comment.
I'm actually not sure what this file does but it got changed as a side effect of building so I've included it. Please let me know if I should revert.
I'll run the tests in all 5 docker images and on my mac and it if works everywhere I'll merge with it changed, otherwise if there is trouble I'll back it out and we can do it later. |
This adds the missing interpolation of the local protubuf version into the request to do proto generation with buf remote plugins.
In theory, this should allow us to go back to defaulting to "online" proto generation.
I need to do a bunch of hand testing to confirm, since there are so many possible combinations here.
We can also opt to just fix the missing proto version interpolation and leave the OFFLINE as the default for now and revisit that under a new ticket.