vanity.NotInPackageGoogleProtobuf overly restrictive?

[bufdev]

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:

gogo/google/type/color.pb.go:202: m.Alpha.Size undefined (type *google_protobuf.FloatValue has no field or method Size)
gogo/google/type/color.pb.go:203: m.Alpha.MarshalTo undefined (type *google_protobuf.FloatValue has no field or method MarshalTo)
gogo/google/type/color.pb.go:252: m.Alpha.Size undefined (type *google_protobuf.FloatValue has no field or method Size)
gogo/google/type/color.pb.go:371: m.Alpha.Unmarshal undefined (type *google_protobuf.FloatValue has no field or method Unmarshal)

This is because of https://github.com/gogo/protobuf/commit/c948aac0178a73864b46b0dd098cf9075947570c, 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.

[awalterschulze]

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 ..." github.com/gogo/protobuf/protoc-gen-gogo/descriptor is just the generated gogo code and google/protobuf/descriptor.proto is where the descriptor.proto should be located.

[bufdev]

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.

[awalterschulze]

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.

[bufdev]

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.

[awalterschulze]

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.

[bufdev]

Ah, yes.

FYI I've had to note these exceptions for a while https://github.com/peter-edge/pb/blob/master/README.md#exceptions, I wish there was a better mechanism we could develop.

On Tue, Jan 26, 2016 at 1:35 PM, Walter Schulze notifications@github.com wrote:

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.

— Reply to this email directly or view it on GitHub https://github.com/gogo/protobuf/issues/137#issuecomment-174986929.

[awalterschulze]

Sorry, but I am really finding all this quite hard to understand. Maybe if you show me the solution I'll understand better.

[bufdev]

https://github.com/gogo/protobuf/blob/master/proto/properties.go#L903 https://github.com/gogo/protobuf/blob/master/protoc-gen-gogo/descriptor/descriptor.pb.go#L1798

If you generate another descriptor.pb.proto, and the package it is in is included, properties.go#L903 will print an error.

If you have enums, which descriptor.proto does https://github.com/gogo/protobuf/blob/master/protoc-gen-gogo/descriptor/descriptor.pb.go#L1820 , https://github.com/gogo/protobuf/blob/master/proto/properties.go#L876 will panic.

On Tue, Jan 26, 2016 at 1:51 PM, Walter Schulze notifications@github.com wrote:

Sorry, but I am really finding all this quite hard to understand. Maybe if you show me the solution I'll understand better.

— Reply to this email directly or view it on GitHub https://github.com/gogo/protobuf/issues/137#issuecomment-174998311.

[awalterschulze]

Yes, but I still think you can have a descriptor per library. You can have protoc-gen-go generated descriptor.pb.go as well as a protoc-gen-gogo generated descriptor.pb.go in the same code base without any problems.

[bufdev]

Well that case is fine, because golang/protobuf and gogo/protobuf are different libraries, so they have two different sets of registration maps.

On Tue, Jan 26, 2016 at 2:11 PM, Walter Schulze notifications@github.com wrote:

Yes, but I still think you can have a descriptor per library. You can have protoc-gen-go generated descriptor.pb.go as well as a protoc-gen-gogo generated descriptor.pb.go in the same code base without any problems.

— Reply to this email directly or view it on GitHub https://github.com/gogo/protobuf/issues/137#issuecomment-175004690.

[awalterschulze]

So what case isn't fine?

[bufdev]

When someone does protoc --gogo_out=. google/protobuf/descriptor.proto, and uses ./descriptor.pb.go

On Tue, Jan 26, 2016 at 2:15 PM, Walter Schulze notifications@github.com wrote:

So what case isn't fine?

— Reply to this email directly or view it on GitHub https://github.com/gogo/protobuf/issues/137#issuecomment-175006557.

[awalterschulze]

Ok so the user should not generate duplicate protocol buffers using the same code generator.

[bufdev]

Yes. This is difficult to find sometimes (I found this issue when I had one of the above panics, can't lie). On the reverse side, with grpc-gateway for example, if both golang/protobuf and gogo/protobuf are used, a message generated with gogo will not be registered with golang, and vice versa. So a generated gogo message will not be registered for a library using golang/protobuf.

You might want to consider adding a "compatibility" flag to protoc-gen-gogo, that does registrations with golang/protobuf as well.

On Tue, Jan 26, 2016 at 2:43 PM, Walter Schulze notifications@github.com wrote:

Ok so the user should not generate duplicate protocol buffers using the same code generator.

— Reply to this email directly or view it on GitHub https://github.com/gogo/protobuf/issues/137#issuecomment-175019260.

[awalterschulze]

Yes you should use the generated message with the compatible library.

[bufdev]

Well this gets to the grpc-question (the other issue) - grpc-gateway uses golang/protobuf, and therefore users of gogo/protobuf generated messages have this bug.

On Tue, Jan 26, 2016 at 2:52 PM, Walter Schulze notifications@github.com wrote:

Yes you should use the generated message with the compatible library.

— Reply to this email directly or view it on GitHub https://github.com/gogo/protobuf/issues/137#issuecomment-175025220.

[awalterschulze]

Where does grpc-gateway use golang/protobuf? grpc-go stays codec agnostic.

[bufdev]

Simple grep

[~/go/src/github.com/gengo/grpc-gateway]
pedge$ agg golang/protobuf
protoc-gen-grpc-gateway/descriptor/registry.go:10:  descriptor "github.com/golang/protobuf/protoc-gen-go/descriptor"
protoc-gen-grpc-gateway/descriptor/registry.go:11:  plugin "github.com/golang/protobuf/protoc-gen-go/plugin"
protoc-gen-grpc-gateway/descriptor/services.go:10:  "github.com/golang/protobuf/proto"
protoc-gen-grpc-gateway/descriptor/services.go:11:  descriptor "github.com/golang/protobuf/protoc-gen-go/descriptor"
examples/integration_test.go:21:    "github.com/golang/protobuf/proto"
protoc-gen-grpc-gateway/descriptor/types_test.go:6: "github.com/golang/protobuf/proto"
protoc-gen-grpc-gateway/descriptor/types_test.go:7: descriptor "github.com/golang/protobuf/protoc-gen-go/descriptor"
protoc-gen-grpc-gateway/gengateway/generator.go:14: "github.com/golang/protobuf/proto"
protoc-gen-grpc-gateway/gengateway/generator.go:15: plugin "github.com/golang/protobuf/protoc-gen-go/plugin"
protoc-gen-grpc-gateway/gengateway/generator.go:37:     "github.com/golang/protobuf/proto",
protoc-gen-grpc-gateway/descriptor/registry_test.go:6:  "github.com/golang/protobuf/proto"
protoc-gen-grpc-gateway/descriptor/registry_test.go:7:  descriptor "github.com/golang/protobuf/protoc-gen-go/descriptor"
protoc-gen-grpc-gateway/descriptor/registry_test.go:8:  plugin "github.com/golang/protobuf/protoc-gen-go/plugin"
protoc-gen-grpc-gateway/descriptor/types.go:9:  descriptor "github.com/golang/protobuf/protoc-gen-go/descriptor"
protoc-gen-grpc-gateway/generator/generator.go:6:   plugin "github.com/golang/protobuf/protoc-gen-go/plugin"
protoc-gen-grpc-gateway/main.go:13: "github.com/golang/protobuf/proto"
protoc-gen-grpc-gateway/main.go:14: plugin "github.com/golang/protobuf/protoc-gen-go/plugin"
protoc-gen-grpc-gateway/descriptor/services_test.go:8:  "github.com/golang/protobuf/proto"
protoc-gen-grpc-gateway/descriptor/services_test.go:9:  descriptor "github.com/golang/protobuf/protoc-gen-go/descriptor"
runtime/handler.go:10:  "github.com/golang/protobuf/proto"
runtime/mux.go:10:  "github.com/golang/protobuf/proto"
protoc-gen-grpc-gateway/gengateway/template_test.go:9:  "github.com/golang/protobuf/proto"
protoc-gen-grpc-gateway/gengateway/template_test.go:10: protodescriptor "github.com/golang/protobuf/protoc-gen-go/descriptor"
runtime/proto2_convert.go:4:    "github.com/golang/protobuf/proto"
runtime/query.go:11:    "github.com/golang/protobuf/proto"
runtime/query_test.go:9:    "github.com/golang/protobuf/proto"
utilities/name.go:5:    // adopted from github.com/golang/protobuf/protoc-gen-go/generator/generator.go
utilities/name.go:8:    // https://github.com/golang/protobuf
[~/go/src/github.com/gengo/grpc-gateway]
pedge$ alias agg
alias agg='ag --nogroup --ignore '\''*\.pb\.go'\'' --ignore '\''*\.pb\.gw\.go'\'' --ignore '\''*\.gen\.go'\'' --ignore Godeps --ignore '\''vendor/'\'' --file-search-regex '\''.go'\'''

[bufdev]

The runtime/* files are not part of the generation tool. The above does not include .pb.go files, which are also included:

./third_party/googleapis/google/api/annotations.pb.go
./third_party/googleapis/google/api/http.pb.go

Which call RegisterExtension and RegisterType.

[awalterschulze]

I was wondering if the generated code uses golang/protobuf. The user shouldn't care what library grpc-gateway uses, only what the his/her generated code is using. So the grep doesn't help me much.

[bufdev]

This is the whole point of this thread - it DOES matter which generated library they are using, due to the registration functions as described above. I can help clarify further if needed, but I do believe this thread explains the issue well.

The grep shows that both the generated code and runtime code uses golang/protobuf.

On Tue, Jan 26, 2016 at 6:24 PM, Walter Schulze notifications@github.com wrote:

I was wondering if the generated code uses golang/protobuf. The user shouldn't care what library grpc-gateway uses, only what the his/her generated code is using. So the grep doesn't help me much.

— Reply to this email directly or view it on GitHub https://github.com/gogo/protobuf/issues/137#issuecomment-175128235.

[bufdev]

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

[bufdev]

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.

[awalterschulze]

Ok so the grpc-gateway compatibility case has been closed. This stays open until vanity changes can be made to other google.protobuf packages

[bufdev]

Yep :)

[bufdev]

I'll try to send a PR this week for the vanity stuff, I have a simple idea for it

[awalterschulze]

Blocked by https://github.com/golang/protobuf/issues/50 see the pull request for more details.

[awalterschulze]

@peter-edge Was this solved by merging https://github.com/gogo/protobuf/pull/141 ?

[awalterschulze]

I'll reopen if you say it wasn't fixed.