zeroshade opened a new issue, #1029:
URL: https://github.com/apache/arrow-go/issues/1029
### 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
