include_imports does not respect 'strategy: directory'

[abhinav]

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]

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]

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):

• Take your non-import input files and split them by directory.
• For each directory of non-import files:
  • Create a new CodeGeneratorRequest
  • Add the directory's non-import file names into file_to_generate.
  • Add the directory's non-import files AND all of those files' imports into proto_file. CodeGeneratorRequests need to be self-contained, so anything that the non-import files import need to be within proto_file, but won't have code generated for them.
  • Send this CodeGeneratorRequest to your plugin and get back the result.

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:

• include_imports: true isn't something you can do in protoc for generated stubs, only for when producing FileDescriptorSets.
• What you want is include_imports: false and to specify what you want to generate as inputs.

[abhinav]

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

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]

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]

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.