bufbuild / buf

The best way of working with Protocol Buffers.
https://buf.build
Apache License 2.0
9.08k stars 276 forks source link

include_imports does not respect 'strategy: directory' #3056

Open abhinav opened 5 months ago

abhinav commented 5 months ago

GitHub Repository

https://github.com/abhinav/buf-include_imports-strategy-repro

Commands

buf generate

Output

protoc: common/v1/value.proto, keyvalue/v1/service.proto

Expected Output

protoc: common/v1/value.proto
protoc: keyvalue/v1/service.proto

Anything else?

I wrote a minimal plugin in the reproduction repo that just logs the files to generate to demonstrate the problem.

I'm using the latest release of Buf as of writing this.

❯ buf --version
1.32.2

The only workaround right now is to manually list imports in the inputs section. Without this, old plugins like Twirp explode during package name resolution.

doriable commented 5 months ago

Thank you for providing a clear example. In the use-case of Twirp, specifically, is there a reason why you would need include_imports for the protoc-gen-twirp plugin? For that particular use-case, it seems protoc-gen-twirp should not need to generate additional code for the import file(s) since it should only care about service definitions.

That being said, the behaviour outlined here is a valid UX difference between buf and protoc -- we provide the imports alongside definitions so that they can be used together in a single code generator request since buf is aware of the imports.

bufdev commented 5 months ago

Thanks for the detailed issue here.

I'll try to explain what is happening here, and why I think this may be a documentation issue on our part - in effect, include_imports for stub generation isn't actually a feature available in protoc, and there is no equivalent to include_imports: true with strategy: directory in protoc.

strategy: directory is an easy way to split up your generation invocation into multiple independent protoc plugin requests, by directory, which is as you mentioned a common need in the Golang ecosystem for many plugins (including Twirp). The logic in a nutshell (a little more obscured under the hood but):

include_imports: true modifies this CodeGeneratorRequest by just adding all the imports also into file_to_generate.

In protoc land, include_imports: true only exists if you use -o / --descriptor_set_out. Assuming you have a.proto that imports import.proto, You aren't able to say something like protoc --include_imports --go_out=gen a.proto and expect that code will be generated for both a.proto and import.proto, the only way to do that is to specify both in the invocation, i.e. protoc --go_out=gen a.proto import.proto, which...if they have different package names, will blow up. In theory, you could do something like protoc --include-imports -o /dev/stdout a.proto | convert_to_code_generator_request_in_some_way | protoc-gen-twirp, but...of course in uncharted lands.

The equivalent in buf land is to specify inputs explicitly that contain both a.proto and import.proto. If you do this, then strategy: directory will obviously do what you want, as you pointed out :-)

include_imports: true isn't really intended for standard Golang plugins, at least of the go/grpc/twirp/connect/etc variety that use the standard Golang generation patterns. It's a feature really intended for i.e. JS/TS plugins that don't have imports generated somewhere else.

Not sure if this answer is satisfying or makes sense, but the short of it:

abhinav commented 5 months ago

Hey!

In the use-case of Twirp, specifically, is there a reason why you would need include_imports for the protoc-gen-twirp plugin?

Yeah, so the setup is a bit non-standard because it's a monorepo with multiple independent projects, but a shared proto/ directory.

Roughly like this: we have a single directory of .proto files (say "proto"), and multiple independent sub-projects. e.g.

$repoRoot
 |- proto/
 |    |- proj1/foo.proto
 |    |- proj2/bar.proto
 |- proj1/
 |    |- go.mod
 |    |- ...
 |- proj2/
      |- go.mod
      |- ...

I'm trying to set it up so that each project can have its own copy of the generated code that it cares about by having each project have its own buf.gen.yaml. (The projects cannot share the generated code because they're separate Go modules.)


proj2/
  |- go.mod
  |- buf.gen.yaml
  |- gen/go
      |- proj2/bar.pb.go
buf.gen.yaml ```yaml version: v2 managed: enabled: true override: - file_option: go_package_prefix value: example.com/proj2/gen/go plugins: - local: protoc-gen-go out: pb opt: paths=source_relative include_imports: true - local: protoc-gen-twirp out: pb opt: paths=source_relative include_imports: true inputs: - directory: .. paths: - ../proto/proj2 ```

So include_imports is being used as a convenience for when proj2 imports a file (e.g. proto/common/whatever.proto).

we provide the imports alongside definitions so that they can be used together in a single code generator request since buf is aware of the imports.

I want to clarify that the issue isn't that the information about the imported protos is present, but that the imported .proto is in the set of files that it requested codegen for.

abhinav commented 5 months ago

include_imports: true modifies this CodeGeneratorRequest by just adding all the imports also into file_to_generate.

Ah. So that part is intended behavior.

bufdev commented 5 months ago

Talked offline: it turns out we sctually should make strategy: directory respect include_imports: true and we have a path forward to do so, so we will do this.