Switch to protoc-gen-go #3905
Switch to protoc-gen-go #3905
Conversation
| DOMAIN_STATUS_REGISTERED DomainStatus = 1 | ||
| DOMAIN_STATUS_DEPRECATED DomainStatus = 2 | ||
| DOMAIN_STATUS_DELETED DomainStatus = 3 | ||
| DomainStatus_DOMAIN_STATUS_INVALID DomainStatus = 0 |
There was a problem hiding this comment.
One annoying thing with the official compiler, is that it prefix enum values with enum type. This results in duplicative prefix, as we already do that in .proto files to comply with buf lint rules. We could either:
- Ignore that (not that big a deal, since they will be mapped to internal types)
- Drop prefix in proto files. Not ideal, as this would require to deviate from default lint rules. Also _INVALID value would still need a prefix, as they the same enum value can not appear within single proto package.
- Use custom post-generation script to cut unwanted prefix away.
There was a problem hiding this comment.
hmmmmm. tbh I wonder if this is code-generator-specific: https://docs.buf.build/lint-rules#enum_value_prefix
Protobuf enums use C++ scoping rules, which makes it not possible to have two enums in the same package with the same enum value name (an exception is when enums are nested, in which case this rule applies within the given message).
I don't think that's correct for the spec, but it seems accurate for the v1 generator, but not v2 (since it adds this prefix). And it could go either way for other languages.
On the theory that other languages may use the public API proto files, I think I'll vote to keep it this verbose nonsense. Once mapped to internal types it really doesn't matter.
There was a problem hiding this comment.
Yes this is indeed code generator specific. Gogo compiler does not do this by default and also have option to change the behavior. Official protoc-gen-go however does not support it. It is clearly visible from source. There is an open issue: golang/protobuf#513
Linter is doing good job here, as it really depends on the compiler.
|
I can't seem to reproduce the output... after shuffling random combinations of flags I managed to get something close, but:
It kinda seems like the generated code here was generated with github.com/golang/protobuf/[email protected] (or earlier)? I get the same kind of output if I use that. That'd be the old generator though, AFAICT. |
|
fwiw this is what I'm using to test all this out, while fiddling with args / versions / etc. it's a kinda hacked-together mimic of what I'm doing in the makefile cleanup, I'll do something nicer in the end: BUF_BIN = $(BUILD)/bin/buf
PROTOC_BIN = $(BUILD)/bin/protoc
$(BUILD):
mkdir -p build/bin
$(BUF_BIN): | $(BUILD)
VERSION="0.36.0" && \
echo "Getting buf $${VERSION}" && \
curl -sSL \
"https://github.com/bufbuild/buf/releases/download/v$${VERSION}/buf-$$(uname -s)-$$(uname -m)" \
-o "$(BUF_BIN)" && \
chmod +x "$(BUF_BIN)"
# https://www.grpc.io/docs/languages/go/quickstart/
$(PROTOC_BIN): | $(BUILD)
VERSION="3.13.0" && \
echo "Getting protoc $${VERSION}" && \
curl -sSL \
https://github.com/protocolbuffers/protobuf/releases/download/v$${VERSION}/protoc-$${VERSION}-$$(uname -s)-$$(uname -m).zip \
-o $(PROTOC_BIN).zip && \
unzip $(PROTOC_BIN).zip -d $(PROTOC_BIN)-files && \
cp $(PROTOC_BIN)-files/bin/protoc $(PROTOC_BIN)
proto-lint: $(BUF_BIN)
cd $(PROTO_ROOT) && ../$(BUF_BIN) lint
p: ## quick re-gen helper for old protobuf
rm -rf .gen/proto
$(MAKE) proto-compile
$(MAKE) proto-lint
pn: ## quick re-gen helper for new protobuf
rm -rf .gen/proto
$(MAKE) proto-compile-new
$(MAKE) proto-lint
# mostly? reproduces the commit, using old plugin
proto-compile: $(PROTOC_BIN)
GOOS= GOARCH= gobin -mod=readonly github.com/golang/protobuf/[email protected]
mkdir -p $(PROTO_OUT)
$(foreach PROTO_DIR, $(PROTO_DIRS), \
$(PROTOC_BIN) \
-I=$(PROTO_ROOT)/public -I=$(PROTO_ROOT)/internal \
-I=$(PROTOC_BIN)-files/include \
--go_out=plugins=grpc:.gen/proto/ \
--go_opt=module=$(PROJECT_ROOT)/.gen/proto \
$(PROTO_DIR)*.proto \
$(NEWLINE))
# using new go plugin, i.e. google.golang.org/protobuf
proto-compile-new: $(PROTOC_BIN)
# https://www.grpc.io/docs/languages/go/quickstart/
GOOS= GOARCH= gobin -mod=readonly google.golang.org/protobuf/cmd/[email protected]
GOOS= GOARCH= gobin -mod=readonly google.golang.org/grpc/cmd/[email protected]
mkdir -p $(PROTO_OUT)
$(PROTOC_BIN) \
-I=$(PROTO_ROOT)/public \
-I=$(PROTO_ROOT)/internal \
-I=$(PROTOC_BIN)-files/include \
--go_out=.gen/proto/ \
--go_opt=module=$(PROJECT_ROOT)/.gen/proto \
--go-grpc_out=.gen/proto \
--go-grpc_opt=module=$(PROJECT_ROOT)/.gen/proto \
$(shell find $(PROTO_DIRS) -name '*.proto')Builds per version combination are nice and repeatable with Since that gives everything the package name Separately: holy cow, this whole protoc ecosystem is a nightmare. Undocumented options/tools/etc and almost-nonexistent |
|
Ah, and good news on the timestamp front: apparently the new generator uses timestamp which is an = alias for timestamppb -> timestamppb.(*Timestamp).AsTime() gives you a stdlib time. Though, given how weirdly invalid |
There was a problem hiding this comment.
Approving partly to unblock to simplify other proto work - the basic tactics work and we're not stuck with them forever, so whenever it's repeatable, I'm happy with it. The go_package renaming might be worth doing in this PR, especially since it'll impact import paths / other go code.
Codegen-version-wise, I'll vote for going "full v2 protoc codegen" as much as possible. It has almost been a year since it has been announced, and Uber has been trying to move off gogo for a while anyway. Without the gogo additions there's not much reason to stick with v1... as far as I'm aware, anyway. But if you find something that swings in favor of v1, it's still supported for the forseeable future, so we'll be fine with v1 as well.
The makefile tactic in #3905 (comment) can just be copied for now if you want, it should be pretty reliable / isolate everything that needs to be isolated. It'll get rewritten a bit as I change the makefile, so it doesn't matter if it's a bit hacky in the meantime.
Definitely change any versions you like, they're just ones I grabbed somewhat randomly.
I haven't read literally any changelogs or bug reports, so I may have missed something obvious.
Ok, I have updated:
|
What changed?
time.Timeandtime.Duration. However since we will be mapping them to internal types anyway, we can do the conversion within mapping layer.Use the latest version of protoc tooling available this day:
Why?
Since the release of protobuf api v2, the gogo maintainers announced that they stopped maintaining gogo and asked the community for new owners. Looks like new official API is becoming mainstream.
How did you test it?
Potential risks