-
Notifications
You must be signed in to change notification settings - Fork 806
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
vanity.NotInPackageGoogleProtobuf overly restrictive? #137
Comments
Yes not generating code for any google.protobuf package is probably a bit excessive. A more restrictive exclusion would be welcome. I don't think I understand you sentence about "people should use ..." |
For go imports, people should use the github.com/gogo/protobuf/protoc-gen-gogo/descriptor package instead of generating their own code for google/protobuf/descriptor.proto. This is because of https://godoc.org/github.com/gogo/protobuf/proto#RegisterType and other Register* methods - if you generate two golang files for the same proto code, you have conflicts with gogo/protobuf (or golang/protobuf) global variables - either an error will occur or you will get an inconsistent result (if a map is overwritten and not checked, for example). It's an annoying part of the golang plugins, but understandable. So when I say "people should use", it means just that - github.com/gogo/protobuf/protoc-gen-gogo/descriptor (and github.com/golang/protobuf/protoc-gen-go/descriptor) are the standards at this point, so when using messages from google/protobuf/descriptor.proto, use those packages. |
Yes the user should probably not generate their own descriptor.pb.go I think if you use the protocol buffer library that was used to generate the code everything should work fine. Or I might still be missing something. Except if you use gofast_out, since then you should use the goprotobuf library, but its an obvious special case. |
What do you mean "the protocol buffer library that was used the generate the code"? Dont't understand :) Using a different package with descriptor.pb.go will result in registration errors. |
If you use protoc-gen-go you should use the golang/protobuf/proto library for marshaling and unmarshaling those messages and if you use protoc-gen-gogo you should use the gogo/protobuf/proto library. |
Ah, yes. FYI I've had to note these exceptions for a while On Tue, Jan 26, 2016 at 1:35 PM, Walter Schulze notifications@github.com
|
Sorry, but I am really finding all this quite hard to understand. Maybe if you show me the solution I'll understand better. |
https://github.com/gogo/protobuf/blob/master/proto/properties.go#L903 If you generate another descriptor.pb.proto, and the package it is in is If you have enums, which descriptor.proto does On Tue, Jan 26, 2016 at 1:51 PM, Walter Schulze notifications@github.com
|
Yes, but I still think you can have a descriptor per library. |
Well that case is fine, because golang/protobuf and gogo/protobuf are On Tue, Jan 26, 2016 at 2:11 PM, Walter Schulze notifications@github.com
|
So what case isn't fine? |
When someone does protoc --gogo_out=. google/protobuf/descriptor.proto, and On Tue, Jan 26, 2016 at 2:15 PM, Walter Schulze notifications@github.com
|
Ok so the user should not generate duplicate protocol buffers using the same code generator. |
Yes. This is difficult to find sometimes (I found this issue when I had one You might want to consider adding a "compatibility" flag to On Tue, Jan 26, 2016 at 2:43 PM, Walter Schulze notifications@github.com
|
Yes you should use the generated message with the compatible library. |
Well this gets to the grpc-question (the other issue) - grpc-gateway uses On Tue, Jan 26, 2016 at 2:52 PM, Walter Schulze notifications@github.com
|
Where does grpc-gateway use golang/protobuf? |
Simple grep
|
The runtime/* files are not part of the generation tool. The above does not include .pb.go files, which are also included:
Which call RegisterExtension and RegisterType. |
I was wondering if the generated code uses golang/protobuf. |
This is the whole point of this thread - it DOES matter which generated The grep shows that both the generated code and runtime code uses On Tue, Jan 26, 2016 at 6:24 PM, Walter Schulze notifications@github.com
|
Here's some example generated code: https://github.com/peter-edge/openflights-go/blob/master/openflights.pb.gw.go I can help explain further, but please look at the registration functions |
The open question here is whether it matters with grpc-gateway, as the runtime may not rely on any of this, which is what I'm going to investigate - the one area where I would have concern is if grpc-gateway is using the MessageType function, which it does not appear to. This is more of an overall concern - the global context in both golang/protobuf and gogo/protobuf presents issues when using both, and the bugs that could come from this will be hard to deduce down to this problem. |
Ok so the grpc-gateway compatibility case has been closed. This stays open until vanity changes can be made to other google.protobuf packages |
Yep :) |
I'll try to send a PR this week for the vanity stuff, I have a simple idea for it |
Blocked by golang/protobuf#50 see the pull request for more details. |
@peter-edge Was this solved by merging #141 ? |
I'll reopen if you say it wasn't fixed. |
I'm building go.pedge.io/pb, which is go.pedge.io/google-protobuf + go.pedge.io/googleapis + my own standard protobuf library, but for both golang and gogo this time. I am trying to use gogofast, but I got a ton of errors along the lines of:
This is because of c948aac, which if I understand, is basically meant to block out google/protobuf/descriptor.proto types, not the well-known types.
Can I send a PR that makes this less restrictive? google/protobuf/descriptor.proto is a special case in google/protobuf in general, so I'd want to special case those specifically. Without changing this, using well-known types with gogofast/gogofaster/gogoslick is not possible, and people should use github.com/gogo/protobuf/protoc-gen-gogo/descriptor for google/protobuf/descriptor.proto anyways.
The text was updated successfully, but these errors were encountered: