Skip to content

[Dev Tools][CI] Pin protobuf codegen toolchain and verify generated .pb.go files are up to date #1029

Description

@zeroshade

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 curls 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

Component(s)

Continuous Integration, Developer Tools

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions