fix(proto): generate yarpc stubs with v2 codec to match V2 messages#194
Closed
albertywu wants to merge 3 commits into
Closed
fix(proto): generate yarpc stubs with v2 codec to match V2 messages#194albertywu wants to merge 3 commits into
albertywu wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the committed YARPC protobuf stubs to use YARPC’s protobuf v2 codec/runtime (google.golang.org/protobuf) instead of the legacy gogo-based codec, fixing a runtime incompatibility where gogo decoding can panic when given V2-generated message structs.
Changes:
- Switches YARPC stub generation to
protoc-gen-yarpc-go-v2(Makefileprototarget). - Regenerates
*.pb.yarpc.gostubs to usego.uber.org/yarpc/encoding/protobuf/v2andgoogle.golang.org/protobuf/proto. - Updates Bazel
go_librarydeps to drop gogo protobuf deps and add protobuf-v2/YARPC-v2 deps.
Reviewed changes
Copilot reviewed 5 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
Makefile |
Updates proto generation to use --yarpc-go-v2_out/opt. |
submitqueue/gateway/protopb/gateway.pb.yarpc.go |
Regenerated YARPC stubs using YARPC protobuf v2 codec/runtime. |
submitqueue/gateway/protopb/BUILD.bazel |
Adjusts Bazel deps to match regenerated v2 YARPC stubs. |
submitqueue/orchestrator/protopb/orchestrator.pb.yarpc.go |
Regenerated YARPC stubs using YARPC protobuf v2 codec/runtime. |
submitqueue/orchestrator/protopb/BUILD.bazel |
Adjusts Bazel deps to match regenerated v2 YARPC stubs. |
stovepipe/gateway/protopb/gateway.pb.yarpc.go |
Regenerated YARPC stubs using YARPC protobuf v2 codec/runtime. |
stovepipe/gateway/protopb/BUILD.bazel |
Adjusts Bazel deps to match regenerated v2 YARPC stubs. |
stovepipe/orchestrator/protopb/orchestrator.pb.yarpc.go |
Regenerated YARPC stubs using YARPC protobuf v2 codec/runtime. |
stovepipe/orchestrator/protopb/BUILD.bazel |
Adjusts Bazel deps to match regenerated v2 YARPC stubs. |
Files not reviewed (4)
- stovepipe/gateway/protopb/gateway.pb.yarpc.go: Language not supported
- stovepipe/orchestrator/protopb/orchestrator.pb.yarpc.go: Language not supported
- submitqueue/gateway/protopb/gateway.pb.yarpc.go: Language not supported
- submitqueue/orchestrator/protopb/orchestrator.pb.yarpc.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+205
to
209
| ReflectionMeta: reflection.ServerMeta{ | ||
| ServiceName: "uber.submitqueue.gateway.SubmitQueueGateway", | ||
| FileDescriptors: yarpcFileDescriptorClosuref1a937782ebbded5, | ||
| }, | ||
| } |
Comment on lines
+169
to
173
| ReflectionMeta: reflection.ServerMeta{ | ||
| ServiceName: "uber.submitqueue.orchestrator.SubmitQueueOrchestrator", | ||
| FileDescriptors: yarpcFileDescriptorClosure96b6e6782baaa298, | ||
| }, | ||
| } |
Comment on lines
+169
to
173
| ReflectionMeta: reflection.ServerMeta{ | ||
| ServiceName: "uber.submitqueue.stovepipe.StovepipeGateway", | ||
| FileDescriptors: yarpcFileDescriptorClosuref1a937782ebbded5, | ||
| }, | ||
| } |
Comment on lines
+169
to
173
| ReflectionMeta: reflection.ServerMeta{ | ||
| ServiceName: "uber.submitqueue.stovepipe.orchestrator.StovepipeOrchestrator", | ||
| FileDescriptors: yarpcFileDescriptorClosure96b6e6782baaa298, | ||
| }, | ||
| } |
Comment on lines
340
to
+344
| proto: ## Generate protobuf files from .proto definitions | ||
| @echo "Generating protobuf files with protoc..." | ||
| @protoc --go_out=submitqueue/gateway/protopb --go_opt=paths=source_relative \ | ||
| --go-grpc_out=submitqueue/gateway/protopb --go-grpc_opt=paths=source_relative \ | ||
| --yarpc-go_out=submitqueue/gateway/protopb --yarpc-go_opt=paths=source_relative \ | ||
| --yarpc-go-v2_out=submitqueue/gateway/protopb --yarpc-go-v2_opt=paths=source_relative \ |
Collaborator
There was a problem hiding this comment.
maybe we can add a check/install for this?
Contributor
Author
There was a problem hiding this comment.
Updated toolchain to be hermetic (not dependent on system-installed protoc / plugins)
The committed *.pb.yarpc.go stubs were produced by the legacy
protoc-gen-yarpc-go, which wires yarpc's gogo-based protobuf codec
(github.com/gogo/protobuf). The sibling *.pb.go messages, however, are
generated by protoc-gen-go (google.golang.org/protobuf V2 runtime, with
the protoimpl `state`/`sizeCache`/`unknownFields` fields). The two halves
disagree: the gogo codec assumes gogo-style structs, but is handed V2
ones.
This is dormant in this repo because the example servers run the
V2-native grpc-go stub; the yarpc stubs are unused here. But when the
yarpc procedures are consumed elsewhere (e.g. a yarpc monorepo), the gogo
codec's reflection unmarshaler panics on the first untagged V2 field:
protobuf tag not enough fields in LandRequest.state:
(gogo's encode side tolerates V2 structs by skipping untagged fields, but
the decode side parses every field's protobuf tag and panics — so the
server inbound / client response path breaks.)
Switch the Makefile proto target to protoc-gen-yarpc-go-v2 and regenerate.
The v2 generator emits an API-compatible stub (same NewFx*YARPCProcedures
symbols) whose codec uses the google.golang.org/protobuf V2 runtime,
agreeing with the V2 messages. The codec is per-procedure, so these
procedures interoperate with gogo-based services in the same process; the
wire bytes are identical. gazelle drops the gogo dep and adds
encoding/protobuf/v2; no go.mod/MODULE.bazel change is required.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
albertywu
force-pushed
the
fix-yarpc-go-v2-codec
branch
from
16eaa0f to
f1392ff
Compare
`make proto` previously shelled out to whatever protoc-gen-go,
protoc-gen-go-grpc, and protoc-gen-yarpc-go-v2 happened to be on the host
$PATH (installed via `go install ...@latest`), so generated stubs drifted
between machines. protoc itself was already pinned by the ./tool/protoc
wrapper; this closes the remaining gap by pinning the plugins too.
- Pin all three plugins via `tool` directives in go.mod, at the versions
stamped in the committed output (protoc-gen-go v1.36.10,
protoc-gen-go-grpc v1.5.1, protoc-gen-yarpc-go-v2 from yarpc v1.81.0).
protoc-gen-go-grpc is a separate module, now added to the graph.
- Add tool/protoc-gen-{go,go-grpc,yarpc-go-v2} wrappers that run the pinned
plugins via `go tool`, mirroring ./tool/protoc.
- Pass them to protoc with explicit --plugin= flags so $PATH is never
consulted, and run a pinned goimports pass so `make proto` emits the
committed (formatted) form in one command.
- Add a `check-proto` target (regenerate + assert clean) wired into the CI
`tidy` job so stale generated files fail the required-checks gate.
- Drop the host protoc/plugin install steps from DEVELOPMENT.md.
`go mod tidy` also drops the now-unused gogo/protobuf family, which the
yarpc v1->v2 codec migration on this branch made dead.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 20 changed files in this pull request and generated 5 comments.
Files not reviewed (4)
- stovepipe/gateway/protopb/gateway.pb.yarpc.go: Language not supported
- stovepipe/orchestrator/protopb/orchestrator.pb.yarpc.go: Language not supported
- submitqueue/gateway/protopb/gateway.pb.yarpc.go: Language not supported
- submitqueue/orchestrator/protopb/orchestrator.pb.yarpc.go: Language not supported
Comment on lines
+205
to
209
| ReflectionMeta: reflection.ServerMeta{ | ||
| ServiceName: "uber.submitqueue.gateway.SubmitQueueGateway", | ||
| FileDescriptors: yarpcFileDescriptorClosuref1a937782ebbded5, | ||
| }, | ||
| } |
Comment on lines
+169
to
173
| ReflectionMeta: reflection.ServerMeta{ | ||
| ServiceName: "uber.submitqueue.orchestrator.SubmitQueueOrchestrator", | ||
| FileDescriptors: yarpcFileDescriptorClosure96b6e6782baaa298, | ||
| }, | ||
| } |
Comment on lines
+169
to
173
| ReflectionMeta: reflection.ServerMeta{ | ||
| ServiceName: "uber.submitqueue.stovepipe.StovepipeGateway", | ||
| FileDescriptors: yarpcFileDescriptorClosuref1a937782ebbded5, | ||
| }, | ||
| } |
Comment on lines
+169
to
173
| ReflectionMeta: reflection.ServerMeta{ | ||
| ServiceName: "uber.submitqueue.stovepipe.orchestrator.StovepipeOrchestrator", | ||
| FileDescriptors: yarpcFileDescriptorClosure96b6e6782baaa298, | ||
| }, | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 20 changed files in this pull request and generated 6 comments.
Files not reviewed (4)
- stovepipe/gateway/protopb/gateway.pb.yarpc.go: Language not supported
- stovepipe/orchestrator/protopb/orchestrator.pb.yarpc.go: Language not supported
- submitqueue/gateway/protopb/gateway.pb.yarpc.go: Language not supported
- submitqueue/orchestrator/protopb/orchestrator.pb.yarpc.go: Language not supported
Comment on lines
+205
to
+208
| ReflectionMeta: reflection.ServerMeta{ | ||
| ServiceName: "uber.submitqueue.gateway.SubmitQueueGateway", | ||
| FileDescriptors: yarpcFileDescriptorClosuref1a937782ebbded5, | ||
| }, |
Comment on lines
+169
to
+172
| ReflectionMeta: reflection.ServerMeta{ | ||
| ServiceName: "uber.submitqueue.orchestrator.SubmitQueueOrchestrator", | ||
| FileDescriptors: yarpcFileDescriptorClosure96b6e6782baaa298, | ||
| }, |
Comment on lines
+169
to
+172
| ReflectionMeta: reflection.ServerMeta{ | ||
| ServiceName: "uber.submitqueue.stovepipe.StovepipeGateway", | ||
| FileDescriptors: yarpcFileDescriptorClosuref1a937782ebbded5, | ||
| }, |
Comment on lines
+169
to
+172
| ReflectionMeta: reflection.ServerMeta{ | ||
| ServiceName: "uber.submitqueue.stovepipe.orchestrator.StovepipeOrchestrator", | ||
| FileDescriptors: yarpcFileDescriptorClosure96b6e6782baaa298, | ||
| }, |
Comment on lines
+92
to
+97
| Proto generation is fully hermetic and needs no manual installs: `make proto` | ||
| downloads a pinned `protoc` via `./tool/protoc` (see `.protocversion`) and runs | ||
| the `protoc-gen-go`, `protoc-gen-go-grpc`, and `protoc-gen-yarpc-go-v2` plugins | ||
| at the versions pinned by the `tool` directives in `go.mod` (via `go tool`). A | ||
| Go toolchain (and network access on the first run, to fetch protoc and the | ||
| plugin modules) is the only requirement. |
Comment on lines
142
to
+144
| **Proto generation fails:** | ||
| - Ensure all three protoc plugins are installed (see Optional Tools above) | ||
| - Check that `protoc` is in your PATH: `which protoc` | ||
| - `make proto` is hermetic — it needs no host `protoc` or plugins, only a Go toolchain and (on the first run) network access to download pinned `protoc` and the plugin modules. | ||
| - To bump versions: edit `.protocversion` (and add the new platform checksums in `./tool/protoc`) for protoc, or `go get -tool <plugin>@<version>` followed by `make tidy` for a plugin. |
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
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.
Make Proto generation fully hermetic without need for manual installs:
make protodownloads a pinnedprotocvia./tool/protoc(uses.protocversion) and runs theprotoc-gen-go,protoc-gen-go-grpc, andprotoc-gen-yarpc-go-v2plugins at the versions pinned by thetooldirectives ingo.mod(viago tool).This ensures that our generated proto does not vary based on the user's system versions.
Validation
✅
make fmt && make proto && make build && make test && make e2e-test