golang / protobuf

Go support for Google's protocol buffers
BSD 3-Clause "New" or "Revised" License
9.73k stars 1.58k forks source link

Passing "--go_opt=M<src>:<go_pkg>" does not mute "WARNING: Missing 'go_package' option..." #1158

Closed dcow closed 4 years ago

dcow commented 4 years ago

edit for clarity I'm confused about the intended remediation for:

WARNING: Missing 'go_package' option in "example.proto"
please specify it with the full Go package path as
a future release of protoc-gen-go will require this be specified.

I've been generating protobufs by specifying the go package on the command line when compiling the protobufs using the M option (like --go_out=M<src.proto>:<go_package>,out. This is advantageous for many reasons some of which are highlighted below. However, when upgrading my tooling recently and moving over to the new packages, I've started seeing the warning above. Looking around, I no longer see the M option referenced in the README. And a little bit of digging around the issue tracker uncovers comments like below.

original prose

From https://github.com/golang/protobuf/issues/791:

We're moving towards a world where we double down on the presence of go_package. Any logic that tries to derive the Go package path from the information in protobuf alone leads to insanity in some way or another. The go_package option is the only way to have a sane understanding of how proto files relate to one another in the Go language.

While it may be generally true that some prefix is required, why must go_package be specified in the protobuf source file, and not manually (or automatically by reading the go.mod) when the ultimate source is generated?

I can understand how it might get a little hairy for public packages if someone were to include a package that also included its own generation of types off the same set of proto files (so you'd end up with duplicate types derived from identical proto sources), but this is hardly the common and only use case for protobufs, no?

For example: we find it very handy to be able to generate protobufs in a package of our choosing on the server so we can decorate the types such that they support interfaces required to double as database models whilst generating them more simply on the client in their own way. With a fixed go_package, the client would need to import the server (and all of its transitive dependencies and whatnot) to be able to use the generated code.

Forcing a go_package option to live in the proto source file generally prevents compile-time code generation and thus prevents builds from being structured in a way that treats generated source files as build artifacts. Java's gradle protobuf plugin does exactly this, and everything seems to work just fine for that implementation. Why must go projects depend on checked-in code-gen?

It also strikes me as a little backwards that a source proto file should ever even lay rightful claim to being required let alone able able to assert anything about the ultimate generated source packaging structure. The only invariant that need be maintained, as #791 points out, is that the golang proto compiler plugin knows how to generate source such that the go compiler can make sense of import paths. Why does it matter if two packages want to generate the source files in slightly different packages or using different build strategies if, at the end of the day, the types and service definitions remain compatible?

I totally understand the need to require a go_package option to be specified, even manually, at some point. I don't understand why it has to live in the source proto file.

summary

Maybe I'm just swept up in the whole transition and have a few signals crossed? But it would be really nice to know what the intended end state looks like for projects using the protobuf-go tooling.

dsnet commented 4 years ago

I recommend giving @neild's comment here a read: https://github.com/golang/protobuf/issues/1151#issuecomment-646176703, it's not directly related to your question, but sheds light on the technical complexities involved.

Fundamentally, protoc-gen-go lacks information to derive the Go package path correctly. It works for simple programs, but breaks down as the program scales in complexity. It is better to avoid these troubles by having users provide the information that protoc-gen-go needs to make the right choices from the beginning.

automatically by reading the go.mod

This is an interesting idea, but a challenge is that protoc-gen-go doesn't have access to that information. The only information it has is what the protoc binary passes it, and information about go.mod files is not available because protoc doesn't understand anything about Go modules.

dsnet commented 4 years ago

BTW, the title changed since I started writing my response.

The current title of "Allow go_package to be specified as a --go_opt" is actually already available through the "M" flags. For example:

protoc --go_out=Mpath/to/source.proto=path/to/go/protopackage,$ARGS $SOURCES
dcow commented 4 years ago

Yep! I currently use the M option to specify my go package. I guess I was assuming that the M option was going away since I can no longer find documentation on the option in this repo and since the go plugin prints out the

$ protoc --proto_path=src --go_opt=Msrc/foo/foo.proto=example.com/rpc/gen/foo --go_out=gen src/foo/foo.proto 
WARNING: Missing 'go_package' option in "foo/foo.proto",
please specify it with the full Go package path as
a future release of protoc-gen-go will require this be specified.

message regardless of whether there's an M option or not. If this option will remain in the future, perhaps the warning is simply more aggressive than need be?

dsnet commented 4 years ago

The "M" options are here to stay (they are not going away).

If properly used, they will suppress the warning you are seeing. The problem at hand is a mismatch between what's specified in your M flag, and what's being processed. Notice, that the M flag is specified with "src/foo/foo.proto", while the warning is complaining about "foo/foo.proto"? In other words, protoc-gen-go still hasn't been informed about what Go package path to treat "foo/foo.proto" as.

dcow commented 4 years ago

Okay I've tried to edit the original post for clarity BTW. Anyway, it looks like the proto source path passed to M needs to be relative to the proto_path. Removing src/ from the command above indeed mutes the warning!

So that leave me with two thoughts:

  1. Where can I find documentation on the M option? The documentation I had been using previously seems to be missing... leading me to try and infer intentions.
  2. Perhaps a more precise error message is in order indicating that the source file value passed to M can't be found under the proto_path? I probably would have been able to diagnose the problem and could have avoided opening this issue (and a bunch of extra work moving protobufs around to try and accommodate a hard-coded go_package) had the actual problem been clear.
dcow commented 4 years ago

I also think that for M to useful in the common case it needs to not be a file-specific option but rather simply an import prefix option applying to all imported protos. The source commentary has identified as much: https://github.com/protocolbuffers/protobuf-go/blob/master/compiler/protogen/protogen.go#L295-L299.

In fact, why is the existing import_prefix option deprecated? I see module. Perhaps you should just be able to combine module=<go_package(prefix)> and paths=source_relative such that when specified together it means: "treat all proto import paths relative to the source directory and generate filenames as such, however, add module to all go import paths so that the protos can consistently live together as a go module. And then leave M as is to all total granular control on a file-by-file basis of the generated source.

dcow commented 4 years ago

I took a stab at making it happen: https://go-review.googlesource.com/c/protobuf/+/240238. Regardless of whether you ultimately want to allow this or not, hopefully it spurs further discussion (:

dsnet commented 4 years ago

Where can I find documentation on the M option? The documentation I had been using previously seems to be missing... leading me to try and infer intentions.

I seriously thought we had these documented somewhere, but I can't seem to find them. I filed #1161 to address this.

I also think that for M to useful in the common case it needs to not be a file-specific option but rather simply an import prefix option applying to all imported protos. ... I took a stab at making it happen: https://go-review.googlesource.com/c/protobuf/+/240238. Regardless of whether you ultimately want to allow this or not, hopefully it spurs further discussion (:

Thanks for implementing your proposed flags. There seems to be overlap between with your proposed changes at that of https://github.com/golang/protobuf/issues/992. Are you okay with closing this issue and continuing discussion over there? I think there's improvement that can be made for cases where the go_package option is not used, but we want to have a unified approach for how that is handled.

dsnet commented 4 years ago

Closing this as we can continue any discussion on #992. There's improvement for users who do not use the go_package option and we'd like a single cohesive story around how that is presented. https://go-review.googlesource.com/c/protobuf/+/240238 presents a different approach, but it overlaps in functionality.

dcow commented 4 years ago

@dsnet yes that's fine. Thanks for pointing me to https://github.com/golang/protobuf/issues/992