Conversation
Signed-off-by: lance6716 <lance6716@gmail.com>
Signed-off-by: lance6716 <lance6716@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR standardizes .proto formatting across the repo by adopting buf format, updates CI/local checks to enforce the formatting, and applies the formatter output to existing proto sources (including regenerated Go outputs where applicable).
Changes:
- Add a pinned
buf-based formatter script plusmake proto-fmt/make proto-fmt-checktargets. - Update check/codegen scripts to incorporate formatting checks and handle
buf’s multi-line option formatting for C++ generation. - Reformat many
.protofiles and update generated Go descriptor blobs accordingly.
Reviewed changes
Copilot reviewed 56 out of 61 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/proto_format.sh | New pinned buf download + buf format runner for --check/--write. |
| scripts/check.sh | Runs proto formatting check as part of make check; adds proto option checks (currently problematic). |
| scripts/common.sh | Reworks clean_gogo_proto to strip gogoproto options robustly under buf formatting. |
| scripts/generate_cpp.sh | Uses clean_gogo_proto per file after copying protos for C++ generation. |
| Makefile | Adds proto-fmt and proto-fmt-check targets wired to the formatter script. |
| README.md | Documents the new formatting workflow and how to fix formatting failures. |
| .editorconfig | Enforces 2-space indentation for *.proto files in editors. |
| proto/tsopb.proto | Reformatted via buf format. |
| proto/tracepb.proto | Reformatted via buf format. |
| proto/schedulingpb.proto | Reformatted via buf format. |
| proto/resource_usage_agent.proto | Reformatted via buf format. |
| proto/resource_manager.proto | Reformatted via buf format. |
| proto/replication_modepb.proto | Reformatted via buf format. |
| proto/recoverdatapb.proto | Reformatted via buf format. |
| proto/raft_serverpb.proto | Reformatted via buf format. |
| proto/raft_cmdpb.proto | Reformatted via buf format. |
| proto/mpp.proto | Reformatted via buf format. |
| proto/metapb.proto | Reformatted via buf format. |
| proto/meta_storagepb.proto | Reformatted via buf format. |
| proto/logbackuppb.proto | Reformatted via buf format. |
| proto/keyspacepb.proto | Reformatted via buf format. |
| proto/import_kvpb.proto | Reformatted via buf format. |
| proto/gcpb.proto | Reformatted via buf format. |
| proto/errorpb.proto | Reformatted via buf format. |
| proto/enginepb.proto | Reformatted via buf format. |
| proto/encryptionpb.proto | Reformatted via buf format. |
| proto/disk_usage.proto | Reformatted via buf format. |
| proto/disaggregated.proto | Reformatted via buf format. |
| proto/diagnosticspb.proto | Reformatted via buf format. |
| proto/debugpb.proto | Reformatted via buf format. |
| proto/deadlock.proto | Reformatted via buf format. |
| proto/coprocessor.proto | Reformatted via buf format (notably multi-line field options formatting). |
| proto/configpb.proto | Reformatted via buf format. |
| proto/cdcpb.proto | Reformatted via buf format. |
| proto/autoid.proto | Reformatted via buf format. |
| include/rustproto.proto | Reformatted via buf format. |
| include/eraftpb.proto | Reformatted via buf format. |
| pkg/logbackuppb/logbackuppb.pb.go | Updated generated descriptor blob due to proto formatting/re-ordering effects. |
| pkg/keyspacepb/keyspacepb.pb.go | Updated generated descriptor blob due to proto formatting/re-ordering effects. |
| pkg/configpb/configpb.pb.go | Updated generated descriptor blob due to proto formatting/re-ordering effects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for option in "${options[@]}"; do | ||
| if ! grep -q "$option" "$pb"; then | ||
| echo "Please add option \"$option\" to $pb" | ||
| return 1 |
There was a problem hiding this comment.
grep -q "$option" treats the option string as a regex (so . matches any character, etc.). Since you’re searching for exact lines, use fixed-string matching (e.g. grep -Fq -- "$option") to avoid false positives/negatives.
| install_buf() { | ||
| local found_version url tmp_buf expected_sha actual_sha | ||
|
|
There was a problem hiding this comment.
install_buf declares expected_sha and actual_sha but never uses them. This looks like leftover scaffolding; please remove unused locals to keep the script easy to maintain (or implement checksum verification if that was the intent).
| check-protos-options() { | ||
| local options=( | ||
| 'import "gogoproto/gogo.proto";' | ||
| 'import "rustproto.proto";' | ||
|
|
||
| 'option (gogoproto.sizer_all) = true;' | ||
| 'option (gogoproto.marshaler_all) = true;' | ||
| 'option (gogoproto.unmarshaler_all) = true;' | ||
| # Remove unnecessary fields from pb structs. | ||
| # XXX_NoUnkeyedLiteral struct{} `json:"-"` | ||
| # XXX_unrecognized []byte `json:"-"` | ||
| # XXX_sizecache int32 `json:"-"` | ||
| 'option (gogoproto.goproto_unkeyed_all) = false;' | ||
| 'option (gogoproto.goproto_unrecognized_all) = false;' | ||
| 'option (gogoproto.goproto_sizecache_all) = false;' | ||
| # TiKV does not need reflection and descriptor data. | ||
| 'option (rustproto.lite_runtime_all) = true;' | ||
| ) |
There was a problem hiding this comment.
check-protos-options enforces several gogoproto options (e.g. goproto_unkeyed_all, goproto_unrecognized_all, goproto_sizecache_all) that are not present in any current proto/*.proto files, so make check will fail unconditionally. Either remove this check, or scope it to only the protos that actually require these options / update the protos to include them consistently (and keep the list in sync with reality).
| if ! curl -sSfL --retry "${BUF_CURL_RETRY}" --retry-delay 1 "${url}" -o "${tmp_buf}"; then | ||
| echo "Failed to download buf from: ${url}" >&2 | ||
| rm -f "${tmp_buf}" | ||
| return 1 | ||
| fi | ||
|
|
||
| chmod +x "${tmp_buf}" | ||
| mv "${tmp_buf}" "${BUF_BIN}" |
There was a problem hiding this comment.
install_buf downloads and executes a prebuilt buf binary directly from GitHub via curl without any checksum or signature verification. If the GitHub release, network path, or TLS trust chain are compromised, an attacker could deliver a malicious binary that will be marked executable and run as part of your build/check tooling. To harden the supply chain, fetch buf via a mechanism that verifies integrity (for example, pinned checksums or signatures for the specific version) before chmod +x and mv-ing it into BUF_BIN.
|
Just to avoid the formatting causing some problems, I want to wait some weeks after master branch's merging before continue release branch formatting. |
this PR can be reproduced by
bufas formatter #1408