uber / prototool

Your Swiss Army Knife for Protocol Buffers
MIT License
5.05k stars 345 forks source link

Fix `go_package` option behaviour on go proto API v2 #549

Open arunk-s opened 4 years ago

arunk-s commented 4 years ago

First, A big Thanks for open-sourcing and maintaining prototool.

Since the release of new protobuf API v2 for Golang, option go_package is now required to provide full import path.

This collides with the current recommendations from prototool.

When using protoc-gen-go APIv1 implemented in the terms of APIv2 at version 1.4.0 , prototool generates the following warning.

2020-04-10T13:57:41.333+0200    WARN    protoc returned a line we do not understand, please file this as an issue at https://github.com/uber/prototool/issues/new       {"protocLine": "2020/04/10 13:57:41 WARNING: Deprecated use of 'go_package' option without a full import path in \"foo/bar/baz.proto\", please specify:"}
2020-04-10T13:57:41.333+0200    WARN    protoc returned a line we do not understand, please file this as an issue at https://github.com/uber/prototool/issues/new       {"protocLine": "option go_package = \"foo/bar/baz;foo_bar_baz\";"}
2020-04-10T13:57:41.333+0200    WARN    protoc returned a line we do not understand, please file this as an issue at https://github.com/uber/prototool/issues/new       {"protocLine": "A future release of protoc-gen-go will require the import path be specified."}
2020-04-10T13:57:41.333+0200    WARN    protoc returned a line we do not understand, please file this as an issue at https://github.com/uber/prototool/issues/new       {"protocLine": "See https://developers.google.com/protocol-buffers/docs/reference/go-generated#package for more information."}

Note that there is a separate option to provide paths=source_relative flag to protoc-gen-go which changes the path of the output fields to relative paths. But that doesn't help with the above warnings.

I'll leave the final decision of how prototool will incorporate the changes required for the new APIv2 on the maintainers.

But what I would love to see as a suggested change is:

skyjia commented 4 years ago

meet the same issue

cat-turner commented 4 years ago

I have the same issue

cat-turner commented 4 years ago

it seems that there is a conflict between this import_path, output dir, and defining the path in the proto file. The weirdness can be seen in the import patth of the generated files, which import other generated files. I worked around this problem with bash (moving files up one directory)

sidh commented 4 years ago

I would like to chime in on this issue. We have found another issue with linting rules, specifically FILE_OPTIONS_EQUAL_GO_PACKAGE_V2_SUFFIX. The problem is it checks for equality only. So when you disable FILE_OPTIONS_GO_PACKAGE_NOT_LONG_FORM, you cannot use syntax like option go_package "foo/bar/v1;barv1 together with FILE_OPTIONS_EQUAL_GO_PACKAGE_V2_SUFFIX.

Since API v2 also requires full path in go_package, it would seem like fixing the issue above is also a requirement. Most likely by introducing FILE_OPTIONS_SUFFIX_GO_PACKAGE_V2_SUFFIX that checks go_package suffix or something like that.

bendiknesbo commented 4 years ago

We have the same issue.

sunwen18 commented 4 years ago

Seeing the same issue! And protobuf doc about this https://developers.google.com/protocol-buffers/docs/reference/go-generated#invocation

arunk-s commented 4 years ago

We found a workaround though I'm not sure if this works for everyone else. For everyone facing a similar issue they can try it out, if it fits with their project setup.

Note that this workaround require to have full go_package path defined in protobuf files in the recommended format prior to running prototool.

Ideally it'll be nice if the package path in proto files matches the import_path but since prototool explicitly uses -M option to map module names while generation, it should be okay.


generate:
  go_options:
    import_path: github.com/foo/protobuf
    extra_modifiers:
        google/api/annotations.proto: google.golang.org/genproto/googleapis/api/annotations
        google/api/http.proto: google.golang.org/genproto/googleapis/api/annotations
  plugins:
    # using relative path ../ and generate the protobuf/grpc code in the parent directory
  - name: go
    type: go
    flags: plugins=grpc,paths=source_relative
    output: ../protobuf

  - name: grpc-gateway
    type: go
    flags: paths=source_relative
    output: ../protobuf

On a side note, there is a new flag --go-opt=module to protoc that specifically tries to provide the same thing as prototool tries to do with overriding every module import with -M flag.

For more information on source_relative vs --go-opt=module see this issue.

AlekSi commented 4 years ago

Please consider this issue for v1.11.0

prateek2408 commented 4 years ago

We found a workaround though I'm not sure if this works for everyone else. For everyone facing a similar issue they can try it out, if it fits with their project setup.

Note that this workaround require to have full go_package path defined in protobuf files in the recommended format prior to running prototool.

Ideally it'll be nice if the package path in proto files matches the import_path but since prototool explicitly uses -M option to map module names while generation, it should be okay.

generate:
  go_options:
    import_path: github.com/foo/protobuf
    extra_modifiers:
        google/api/annotations.proto: google.golang.org/genproto/googleapis/api/annotations
        google/api/http.proto: google.golang.org/genproto/googleapis/api/annotations
  plugins:
    # using relative path ../ and generate the protobuf/grpc code in the parent directory
  - name: go
    type: go
    flags: plugins=grpc,paths=source_relative
    output: ../protobuf

  - name: grpc-gateway
    type: go
    flags: paths=source_relative
    output: ../protobuf

On a side note, there is a new flag --go-opt=module to protoc that specifically tries to provide the same thing as prototool tries to do with overriding every module import with -M flag.

For more information on source_relative vs --go-opt=module see this issue.

This solution does not work for me.

dackroyd commented 4 years ago

Managing to mostly work around this by applying a similar configuration to @arunk-s, but disabling the relevant linting rules:

protoc:
  version: 3.6.1 # could do with an update here... haven't tested with newer versions
generate:
  go_options:
    import_path: ... # my package here
  plugins:
    - name: go
      type: go
      flags: plugins=grpc # didn't appear to need 'paths=source_relative', but outputting to a different location
      output: ../generated/grpc
lint:
  group: uber2
  rules:
    remove:
      # Protogen now expects the full package path to be specified, which conflicts with the Uber2 style/linting
      # https://developers.google.com/protocol-buffers/docs/reference/go-generated#package
      - FILE_OPTIONS_EQUAL_GO_PACKAGE_V2_SUFFIX
      - FILE_OPTIONS_GO_PACKAGE_NOT_LONG_FORM

With this, code generation and linting checks are completing successfully, with package declarations like option go_package = "foo/bar/feature/v1;featurev1";

We are also running prototool format over our proto files however, and the formatting wipes out the changed go_package paths, reverting them to option go_package = "featurev1";