[Dev Tools][CI] Pin protobuf codegen toolchain and verify generated .pb.go files are up to date
- Dominant language
- Assembly
- Stars
- 404
- Forks
- 145
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 87
Description
### Problem
We have four generated protobuf files in the repo and they were produced by **three different toolchain combinations**. Nothing pins the generator versions, nothing documents which versions to use, and no CI job verifies that committed generated code matches the proto sources.
| Generated file | protoc-gen-go | protoc |
|---|---|---|
| `arrow/flight/gen/flight/FlightSql.pb.go` | v1.36.11 | v7.34.0 |
| `arrow/flight/gen/flight/Flight.pb.go` | v1.31.0 | v4.25.3 |
| `arrow/flight/gen/flight/Flight_grpc.pb.go` | protoc-gen-go-grpc v1.2.0 | v4.25.3 |
| `arrow/util/util_message/types.pb.go` | v1.31.0 | v4.24.4 |
Contributing factors:
1. **Generator versions are unpinned.** `arrow/flight/gen.go` invokes bare `protoc` from the contributor's `PATH`:
```
//go:generate protoc --experimental_allow_proto3_optional -I../../../format --go_out=./gen/flight --go-grpc_out=./gen/flight --go_opt=paths=source_relative --go-grpc_opt=paths=source_relative Flight.proto
//go:generate protoc --experimental_allow_proto3_optional -I../../../format --go_out=./gen/flight --go-grpc_out=./gen/flight --go_opt=paths=source_relative --go-grpc_opt=paths=source_relative FlightSql.proto
```
Whatever `protoc`/`protoc-gen-go` a contributor happens to have installed is what lands in the commit.
2. **Proto sources are not vendored and their origin is undocumented.** `-I../../../format` points outside the repo, implying a sibling `apache/arrow` checkout. There's no documentation stating that, no pinned revision, and no fetch step.
3. **No CI verification.** Nothing regenerates and diffs, so both stale generated code and unrelated codegen churn pass review silently.
4. **`arrow/util/messages/types.proto` has no `//go:generate` directive at all.** It's generated manually per `arrow/util/messages/README.md`, whose instructions are stale — they say `cd go/arrow/util/`, a path that predates the arrow-go repo split. Output lands in `arrow/util/util_message/` only because the proto carries `option go_package = "../util_message";`.
### Why this matters (concrete cost)
In #732 — a one-field addition to `ActionCreatePreparedStatementResult` — the contributor regenerated with a newer toolchain than the rest of the tree, producing a **+765/-1633** diff on `FlightSql.pb.go` for what should have been ~10 lines.
Reviewing that safely required manually diffing every `protobuf:"..."` struct tag, every exported generated method, every generated type, and every enum constant against the merge base to prove the only semantic change was the intended field. That check shouldn't be a manual reviewer burden, and the failure mode is asymmetric: a genuine unintended semantic change (a dropped field, a renumbered tag, a changed enum value) would be invisible inside that much cosmetic churn.
Note the PR itself was fine — the newer generator actually *matched* our pinned `google.golang.org/protobuf v1.36.11` runtime, so `Flight.pb.go` is the stale file, not the regenerated one. That's exactly the confusion worth eliminating.
### Proposed work
Ordered; each step depends on the previous.
**1. Prerequisite — bring every proto under `go:generate` and fix the docs.**
A generation-freshness CI check can only cover files that have a canonical generate command, so this has to land first.
- Add a `//go:generate` directive for `arrow/util/messages/types.proto` so it isn't generated from hand-copied README commands.
- Fix the stale `cd go/arrow/util/` path in `arrow/util/messages/README.md` (or fold its contents into `CONTRIBUTING.md` and drop the README).
- Re-evaluate `--experimental_allow_proto3_optional` in `arrow/flight/gen.go`; proto3 optional has been stable for many protoc releases and the flag is likely unnecessary now. Confirm removing it is a no-op before doing so.
**2. Document and pin the toolchain.**
Add a "Generating Protobuf" section to `CONTRIBUTING.md`, mirroring the existing **"Generating Thrift"** section, which already establishes the right pattern: install a specific compiler, download the upstream schema, don't commit the input, commit only generated output. It should record:
- the exact `protoc` version;
- `protoc-gen-go` / `protoc-gen-go-grpc` versions, installed via a `tools.go` + `go.mod` tool dependency so they're version-controlled rather than ambient;
- where `format/*.proto` comes from and which `apache/arrow` revision;
- that `google.golang.org/protobuf` in `go.mod` and the `protoc-gen-go` version must be kept in lockstep.
**3. Make generation reproducible without a sibling checkout.**
Fetch `Flight.proto` and `FlightSql.proto` from a **pinned `apache/arrow` ref** as part of generation, matching the Thrift flow (which `curl`s `parquet.thrift` and explicitly does not commit it). Record the pinned ref somewhere greppable so bumping it is a deliberate, reviewable change.
**4. Add a CI check that regenerates and fails on diff.**
Covers both generated locations, which step 1 makes possible:
```
go generate ./arrow/flight ./arrow/util/...
git diff --exit-code -- arrow/flight/gen/flight/ arrow/util/util_message/
```
Pinned generator versions (step 2) are a hard prerequisite, otherwise the job flaps on purely cosmetic codegen differences.
**5. Normalize the existing files in one dedicated PR.**
Regenerate all four with the agreed toolchain so the headers agree. This will be large and purely cosmetic; it should be reviewed as such and kept strictly separate from any feature work. (Intentionally *not* asked of #732.)
### Non-goals
- Not changing any wire format, generated API, or public Go API.
- Not vendoring the proto sources — fetching from a pinned upstream ref keeps a single source of truth and matches the existing Thrift precedent.
- Not switching to the protobuf Opaque API, `buf`, or any new build system — this is strictly about pinning, documenting, and verifying what we already do.
### Notes
- This does **not** block #732. It's the root cause behind the review friction there, filed separately so that PR isn't expanded in scope.
- Step 5 is the only large diff, and it's mechanical.
### Component(s)
Continuous Integration, Developer Tools
Contributor guide
Assessment
This issue has not been assessed yet.