Skip to content
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

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

Closed
dcow opened this issue Jun 28, 2020 · 10 comments

Comments

@dcow
Copy link

dcow commented Jun 28, 2020

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 #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.

@dcow dcow changed the title Don't require go_package in a future release! Allow go_package to be specified as a --go_opt. Jun 28, 2020
@dsnet
Copy link
Member

dsnet commented Jun 28, 2020

I recommend giving @neild's comment here a read: #1151 (comment), 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
Copy link
Member

dsnet commented Jun 28, 2020

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
Copy link
Author

dcow commented Jun 28, 2020

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?

@dcow dcow changed the title Allow go_package to be specified as a --go_opt. Specifying --go_opt=M... should remove WARNING: Missing 'go_package' option... Jun 28, 2020
@dcow dcow changed the title Specifying --go_opt=M... should remove WARNING: Missing 'go_package' option... Passing "--go_opt=M<src>:<go_pkg>" does not mute "WARNING: Missing 'go_package' option..." Jun 28, 2020
@dsnet
Copy link
Member

dsnet commented Jun 28, 2020

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
Copy link
Author

dcow commented Jun 28, 2020

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
Copy link
Author

dcow commented Jun 28, 2020

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 added a commit to dcow/protobuf-go that referenced this issue Jun 28, 2020
The natural behavior should the user want to specify a module but also
leverage source_relative paths output is to consider the module to be
the prefix for all import paths written to the generated go code.

In a `module example.com/proto/gen`, it's possible to do:

    $ tree
    go.mod
    go.sum
    src
      + types
        + foo.proto
	+ bar.proto
      + widget
        + service.proto
    gen

    $ protoc $(find src -name "*.proto")
        --proto_path=src --go_out=gen
        --go_opt=paths=source_relative,module=example.com/types/gen

without specifying the example.com/proto/gen/... module prefix in the
acutal proto files. In order to do this with the `M` option, you would
have to build the package list manually and pass a dedicated `M` option
for each source proto. This gets rather cumbersome.

How is this different than the deprecated import_path option? The use
case solved here is very specific to a common pattern of generating
Go sources from a shared set of proto files into a single root module
path while still retaining the source directory/package structure. This
use case is simple enough and useful enough that it makes sense to
support it specifically without reverting to trying to support a fully
generalized import_path type of option that likely caused more confusion
than desired. It also makes sense of a previously incompatible option
set which is an aesthetically welcome reduciton in error output.

Related discussion: golang/protobuf#1158
dcow added a commit to dcow/protobuf-go that referenced this issue Jun 28, 2020
The natural behavior should the user want to specify a module but also
leverage source_relative paths output is to consider the module to be
the prefix for all import paths written to the generated go code.

In a `module example.com/proto/gen`, it's possible to do:

    $ tree
    go.mod
    go.sum
    src
      + types
        + foo.proto
        + bar.proto
      + widget
        + service.proto
    gen

    $ protoc $(find src -name "*.proto")
        --proto_path=src --go_out=gen
        --go_opt=paths=source_relative,module=example.com/types/gen

without specifying the example.com/proto/gen/... module prefix in the
acutal proto files. In order to do this with the `M` option, you would
have to build the package list manually and pass a dedicated `M` option
for each source proto. This gets rather cumbersome.

How is this different than the deprecated import_path option? The use
case solved here is very specific to a common pattern of generating
Go sources from a shared set of proto files into a single root module
path while still retaining the source directory/package structure. This
use case is simple enough and useful enough that it makes sense to
support it specifically without reverting to trying to support a fully
generalized import_path type of option that likely caused more confusion
than desired. It also makes sense of a previously incompatible option
set which is an aesthetically welcome reduciton in error output.

Related discussion: golang/protobuf#1158
dcow added a commit to dcow/protobuf-go that referenced this issue Jun 28, 2020
The natural behavior should the user want to specify a module but also
leverage source_relative paths output is to consider the module to be
the prefix for all import paths written to the generated go code.

In a `module example.com/proto/gen`, it's possible to do:

    $ tree
    go.mod
    go.sum
    src
      + types
        + foo.proto
        + bar.proto
      + widget
        + service.proto
    gen

    $ protoc $(find src -name "*.proto")
        --proto_path=src --go_out=gen
        --go_opt=paths=source_relative,module=example.com/types/gen

    $ tree gen
    gen
      + types
        + foo.pb.go
	+ bar.pb.go
      + widget
        + service.pb.go

without specifying the example.com/proto/gen/... module prefix in the
acutal proto files. In order to do this with the `M` option, you would
have to build the package list manually and pass a dedicated `M` option
for each source proto. This gets rather cumbersome.

How is this different than the deprecated import_path option? The use
case solved here is very specific to a common pattern of generating
Go sources from a shared set of proto files into a single root module
path while still retaining the source directory/package structure. This
use case is simple enough and useful enough that it makes sense to
support it specifically without reverting to trying to support a fully
generalized import_path type of option that likely caused more confusion
than desired. It also makes sense of a previously incompatible option
set which is an aesthetically welcome reduciton in error output.

Related discussion: golang/protobuf#1158
dcow added a commit to dcow/protobuf-go that referenced this issue Jun 28, 2020
The natural behavior should the user want to specify a module but also
leverage source_relative paths output is to consider the module to be
the prefix for all import paths written to the generated go code.

In a `module example.com/proto/gen`, it's possible to do:

    $ tree
    go.mod
    go.sum
    src
      + types
        + foo.proto
        + bar.proto
      + widget
        + service.proto
    gen

    $ protoc $(find src -name "*.proto")
        --proto_path=src --go_out=gen
        --go_opt=paths=source_relative,module=example.com/proto/gen

    $ tree gen
    gen
      + types
        + foo.pb.go
	+ bar.pb.go
      + widget
        + service.pb.go

without specifying the example.com/proto/gen/... module prefix in the
acutal proto files. In order to do this with the `M` option, you would
have to build the package list manually and pass a dedicated `M` option
for each source proto. This gets rather cumbersome.

How is this different than the deprecated import_path option? The use
case solved here is very specific to a common pattern of generating
Go sources from a shared set of proto files into a single root module
path while still retaining the source directory/package structure. This
use case is simple enough and useful enough that it makes sense to
support it specifically without reverting to trying to support a fully
generalized import_path type of option that likely caused more confusion
than desired. It also makes sense of a previously incompatible option
set which is an aesthetically welcome reduciton in error output.

Related discussion: golang/protobuf#1158
dcow added a commit to dcow/protobuf-go that referenced this issue Jun 28, 2020
The natural behavior should the user want to specify a module but also
leverage source_relative paths output is to consider the module to be
the prefix for all import paths written to the generated go code.

In a `module example.com/proto/gen`, it's possible to do:

    $ tree
    go.mod
    go.sum
    src
      + types
        + foo.proto
        + bar.proto
      + widget
        + service.proto
    gen

    $ protoc $(find src -name "*.proto")
        --proto_path=src --go_out=gen
        --go_opt=paths=source_relative,module=example.com/proto/gen

    $ tree gen
    gen
      + types
        + foo.pb.go
	+ bar.pb.go
      + widget
        + service.pb.go

without specifying the `"example.com/proto/gen/..."` module prefix in
the acutal proto files. In order to do this with the `M` option, you
would have to build the package list manually and pass a dedicated `M`
option for each source proto. This gets rather cumbersome.

How is this different than the deprecated import_path option? The use
case solved here is very specific to a common pattern of generating
Go sources from a shared set of proto files into a single root module
path while still retaining the source directory/package structure. This
use case is simple enough and useful enough that it makes sense to
support it specifically without reverting to trying to support a fully
generalized import_path type of option that likely caused more confusion
than desired. It also makes sense of a previously incompatible option
set which is an aesthetically welcome reduciton in error output.

Related discussion: golang/protobuf#1158
dcow added a commit to dcow/protobuf-go that referenced this issue Jun 28, 2020
The natural behavior should the user want to specify a module but also
leverage source_relative paths output is to consider the module to be
the prefix for all import paths written to the generated go code.

In a `module example.com/proto/gen`, it's possible to do:

    $ tree
    go.mod
    go.sum
    src
      + types
        + foo.proto
        + bar.proto
      + widget
        + service.proto
    gen

    $ protoc $(find src -name "*.proto")
        --proto_path=src --go_out=gen
        --go_opt=paths=source_relative,module=example.com/proto/gen

    $ tree gen
    gen
      + types
        + foo.pb.go
	+ bar.pb.go
      + widget
        + service.pb.go

without specifying the `"example.com/proto/gen/..."` module prefix in
the acutal proto files. In order to do this with the `M` option, you
would have to build the package list manually and pass a dedicated `M`
option for each source proto. This gets rather cumbersome.

How is this different than the deprecated import_path option? The use
case solved here is very specific to a common pattern of generating
Go sources from a shared set of proto files into a single root module
path while still retaining the source directory/package structure. This
use case is simple enough and useful enough that it makes sense to
support it specifically without reverting to trying to support a fully
generalized import_path type of option that likely caused more confusion
than desired. It also makes sense of a previously incompatible option
set which is an aesthetically welcome reduciton in error output.

Related discussion: golang/protobuf#1158
@dcow
Copy link
Author

dcow commented Jun 28, 2020

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
Copy link
Member

dsnet commented Jun 30, 2020

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 #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
Copy link
Member

dsnet commented Jul 6, 2020

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.

@dsnet dsnet closed this as completed Jul 6, 2020
@dcow
Copy link
Author

dcow commented Sep 9, 2020

@dsnet yes that's fine. Thanks for pointing me to #992

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants