feat: Registry merging gogo and protov2 file descriptors #37
Conversation
…o into am/single-registry
proto/merge.go
Outdated
| protoregistry.GlobalFiles.RangeFiles(func(fileDescriptor protoreflect.FileDescriptor) bool { | ||
| fd := protodesc.ToFileDescriptorProto(fileDescriptor) | ||
| if err := CheckImportPath(*fd.Name, *fd.Package); err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Let's collect any issues in a multi-error and then return it if it's non-empty rather than panicking
There was a problem hiding this comment.
What's a multi-error? Just do some string concatenation with err.Error()?
There was a problem hiding this comment.
https://github.com/hashicorp/go-multierror
In go 1.20 we can use https://pkg.go.dev/errors@master#Join
There was a problem hiding this comment.
Talked offline to not use Go 1.20.
I'm not sure about introducing a new dependency here in gogo, seems overkill for this one error case. I used strings.Join with some line breaks instead, lmk if that works for you.
proto/merge.go
Outdated
| if found { | ||
| if !protov2.Equal(gogoFd, fd) { | ||
| diff := cmp.Diff(fd, gogoFd, protocmp.Transform()) | ||
| panic(fmt.Errorf("got different file descriptors for %s; %s", *fd.Name, diff)) |
There was a problem hiding this comment.
For this error, I initially just returned a 1-line error. But when debugging this PR with the SDK, I found it useful to have the full diff. A snippet looks like:
got different file descriptors for cosmos/gov/v1beta1/tx.proto; (*descriptorpb.FileDescriptorProto)(Inverse(protocmp.Transform, protocmp.Message{
// -- snip --
"number": int32(1),
"options": protocmp.Message{
"@type": s"google.protobuf.FieldOptions",
- "[cosmos_proto.accepts_interface]": string("Content"),
+ "[cosmos_proto.accepts_interface]": string("cosmos.gov.v1beta1.Content"),
},
"type": s"TYPE_MESSAGE",
"type_name": string(".google.protobuf.Any"),
// -- snip --
The diff is verbose, so joining all errors in one go can be overwhelming and unreadable. I propose here to return one error at a time, with diff.
proto/merge.go
Outdated
| // MergedRegistry returns a protodesc.Resolver that acts as a single registry | ||
| // which contains all the file descriptors registered with both gogoproto and | ||
| // protoregistry. | ||
| func MergedRegistry() (protodesc.Resolver, error) { |
There was a problem hiding this comment.
This returns a protodesc.Resolver, which is what protoregistry.GlobalFiles implements.
There's also a protodesc.MessageTypeResolver, which is what protoregistry.GlobalTypes implements (and used in the SDK).
Is there an easy wasy to go from Files to Types? Theoretically yes, but I didn't find any API in proto v2. Should I add a MergedTypes() function in this file too?
There was a problem hiding this comment.
Not that I know of... that would be quite hard I think
There was a problem hiding this comment.
We use GlobalTypes in 3 instances in the SDK. What can we do if pulsar files are not registered?
There was a problem hiding this comment.
Where do we use? We can use dynamicpb
There was a problem hiding this comment.
For AppConfig I guess we can use the merged registry if we need to support gogo there
For textual we should allow parameterizing the type resolver and also default to dynamicpb if not found
| // "google.protobuf" pacakges all have different go package names | ||
| // (typepb, structpb, anypb...), so running protoc here will error. | ||
| // As a hack, we skip them here. It should be safe, as they packages | ||
| // are generated internally in this repo. | ||
| if *f.Package == "google.protobuf" { | ||
| continue | ||
| } |
There was a problem hiding this comment.
FYI, here's another hack unfortunately. when running protoc-gen-gogo on google.protobuf.* stuff in this repo, I get an error with inconsistent go package names. This is expected, the new proto files are generated in separate packages: anypb, durationpb...
So the hack is to skip validation for google.protobuf.*. Then in the Makefile, I do sed to modify the package name
|
I'm going to go ahead and merge this. In cosmos/cosmos-sdk#14886 I still got some file descriptor mismatches, but afaict they are all SDK proto files, not gogo's. So this PR should be fine. Let's not tag a new gogo version asap, but wait until the SDK PR is r4r. |
Merge gogo and protoregistry (protov2) file descriptors. Also expose them as a protodesc.Resolver.
Additionally, it performs at runtime the import path checks (same checks as in the protoc-gen-gogo generator).
For reviewers
proto/merge.goprotobuf/google/protobuf/*.prototo v3.15.3 (the one that's in protobuf v1.28.1)Depends on :
Accompanying PR on the SDK: