Draft: Update to protobuf APIv2#226
Open
elboulangero wants to merge 5 commits into
Open
Conversation
For "empty", it's straightforward, we just need to update the import
statement, and then rename:
empty.Empty{} -> emptypb.Empty{}
For "timestamp", it's a bit more complicated as the API changed.
To convert from the protobuf Timestamp message to a Go time, we use
`AsTime()`. We need to check first that the variable is a valid time,
either via `IsValid()` or `CheckValid()`. `CheckValid()` produces an
error, which allow us to keep the logic of the code unchanged (either
return this error, or terminate the program with `log.Fatal()`.
To convert from a Go time to a protobuf Timestamp message, we can now
use `timestamppb.New()`, which works all the time, so no more error
checking.
Documentation at:
- https://pkg.go.dev/google.golang.org/protobuf/types/known/emptypb
- https://pkg.go.dev/google.golang.org/protobuf/types/known/timestamppb
This is a change required by grpc-go >= v1.30.0, cf. grpc/grpc-go@ad51f57 Otherwise build fails with: ``` rpc/rpc.go:61:30: cannot use c (variable of type *CLI) as CLIServer value in argument to RegisterCLIServer: *CLI does not implement CLIServer (missing method mustEmbedUnimplementedCLIServer) ```
Switch from github.com/golang/protobuf to google.golang.org/protobuf to transition to protobuf API v2. Update the commands to generate the code in Makefile. Details below. === go.mod: protobuf v1.20.0 marked the transition to the protobuf API v2, this is the minimul version we can use, in theory. v1.25.0 introduced the method CheckValid() for the protobuf Timestamp type, and we want to use it. === go.mod: grpc We need version 1.32 at least, otherwise the code generated fails to compile with: ``` rpc/rpc_grpc.pb.go:20:16: undefined: grpc.SupportPackageIsVersion7 rpc/rpc_grpc.pb.go:296:31: undefined: grpc.ServiceRegistrar ``` === Makefile: protoc-gen-go-grpc The version of protoc-gen-go-grpc is not aligned with the versions of go-grpc, even though it all comes from https://github.com/grpc/grpc-go/. The list of the protoc-gen-go-grpc versions available can be found at: https://pkg.go.dev/google.golang.org/grpc/cmd/protoc-gen-go-grpc?tab=versions
Commands run from a golang:1.18 container
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Protobuf / grpc is what mirrorbits uses for the cli <-> daemon communication. It's defined in
rpc/rpc.protoand then the client / server code is generated viaprotoc-gen-go(seeregen-prototarget in Makefile), ending up inrpc/rpc.pb.go. Mirrorbits generates APIv1 code. This MR is about switching to APIv2.Back in 2020 Golang published version v2 of the protobuf Go API: https://go.dev/blog/protobuf-apiv2. This is not backward compatible with APIv1. At the same time, Golang team promised: « We intend to maintain support for APIv1 indefinitely ». So there's no pressure to switch to APIv2.
Still, Mirrorbits is a simple cli / daemon, so migrating to new API should be straightforward. The upside of switching is that we're not stuck forever to an old stack. It helps us to move forward with #200.
It's also easier if any contributor can re-generate the RPC code with current tools, otherwise we end up with #172, where it took a while to find the right tools at the right versions to re-generate the RPC code. Thanks to @lazka for fixing it.
Draft
For now I mark it as draft. I think the work is done, it seems to work on the surface. More tests are needed.
Scope of work