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

oneof types are not properly rendered with protoc-gen-doc #11761

Closed
gerolf-da opened this issue Nov 18, 2021 · 1 comment
Closed

oneof types are not properly rendered with protoc-gen-doc #11761

gerolf-da opened this issue Nov 18, 2021 · 1 comment
Assignees

Comments

@gerolf-da
Copy link
Contributor

gerolf-da commented Nov 18, 2021

The current version of protoc-gen-doc we're using (1.1) doesn't support rendering oneof types. This was fixed in protoc-gen-doc 1.4 or later.

The oneof in commands.proto is currently rendered like this:
image

cocreature added a commit that referenced this issue Nov 18, 2021
Backport of #11761

changelog_begin
changelog_end
cocreature added a commit that referenced this issue Nov 18, 2021
Backport of #11761

changelog_begin
changelog_end
@cocreature
Copy link
Contributor

I pushed a branch with an attempt to https://github.com/digital-asset/daml/tree/bump-go-rules-docs

However, this currently fails to build:

bazel build //ledger-api/grpc-definitions:ledger-api-docs
INFO: Invocation ID: 764554f8-dcd4-4cb0-bee5-14c84ab9ee8a
DEBUG: /home/moritz/.cache/bazel/_bazel_moritz/bb4e4404f889dc1b816f246b08c0d9ea/external/rules_haskell/haskell/private/versions.bzl:60:10: WARNING: bazel version is too recent. Supported versions range from 2.1.0 to 4.1.0, but found: 4.2.1- (@non-git)
DEBUG: /home/moritz/.cache/bazel/_bazel_moritz/bb4e4404f889dc1b816f246b08c0d9ea/external/bazel_gazelle/internal/go_repository.bzl:209:18: com_github_pseudomuto_protokit: gazelle: /home/moritz/.cache/bazel/_bazel_moritz/bb4e4404f889dc1b816f246b08c0d9ea/external/com_github_pseudomuto_protokit/examples/jsonator: directory contains multiple proto packages. Gazelle can only generate a proto_library for one package.
DEBUG: /home/moritz/.cache/bazel/_bazel_moritz/bb4e4404f889dc1b816f246b08c0d9ea/external/bazel_gazelle/internal/go_repository.bzl:209:18: org_golang_google_protobuf: gazelle: /home/moritz/.cache/bazel/_bazel_moritz/bb4e4404f889dc1b816f246b08c0d9ea/external/org_golang_google_protobuf/cmd/protoc-gen-go/testdata/imports/test_b_1: directory contains multiple proto packages. Gazelle can only generate a proto_library for one package.
INFO: Analyzed target //ledger-api/grpc-definitions:ledger-api-docs (1 packages loaded, 26 targets configured).
INFO: Found 1 target...
ERROR: /home/moritz/daml/ledger-api/grpc-definitions/BUILD.bazel:244:10: ProtoGen ledger-api/grpc-definitions/ledger-api-docs-sources failed: (Exit 1): bash failed: error executing command
  (cd /home/moritz/.cache/bazel/_bazel_moritz/bb4e4404f889dc1b816f246b08c0d9ea/sandbox/linux-sandbox/343/execroot/com_github_digital_asset_daml && \
  exec env - \
  /nix/store/30wkf1yqkj6a95vfl1iwarwb37phc0f5-bash/bin/bash -c '/nix/store/haq92xsmm6vfhjbi7039d3mh91wd4vap-posix-toolchain/bin/mkdir -p bazel-out/k8-opt/bin/ledger-api/grpc-definitions/ledger-api-docs-sources && bazel-out/k8-opt/bin/external/com_google_protobuf/protoc --descriptor_set_in=bazel-out/k8-opt/bin/external/com_google_protobuf/duration_proto-descriptor-set.proto.bin:bazel-out/k8-opt/bin/external/go_googleapis/google/rpc/errdetails_proto-descriptor-set.proto.bin:bazel-out/k8-opt/bin/external/com_google_protobuf/any_proto-descriptor-set.proto.bin:bazel-out/k8-opt/bin/external/go_googleapis/google/rpc/status_proto-descriptor-set.proto.bin:bazel-out/k8-opt/bin/external/com_google_protobuf/descriptor_proto-descriptor-set.proto.bin:bazel-out/k8-opt/bin/external/com_google_protobuf/empty_proto-descriptor-set.proto.bin:bazel-out/k8-opt/bin/external/com_google_protobuf/timestamp_proto-descriptor-set.proto.bin:bazel-out/k8-opt/bin/external/com_google_protobuf/wrappers_proto-descriptor-set.proto.bin:bazel-out/k8-opt/bin/ledger-api/grpc-definitions/ledger_api_proto-descriptor-set.proto.bin --doc_out=ledger-api/grpc-definitions/rst_mmd.tmpl,docs.rst:bazel-out/k8-opt/bin/ledger-api/grpc-definitions/ledger-api-docs-sources --plugin=protoc-gen-doc=bazel-out/k8-opt/bin/external/com_github_pseudomuto_protoc_gen_doc/cmd/protoc-gen-doc/protoc-gen-doc_/protoc-gen-doc com/daml/ledger/api/v1/active_contracts_service.proto com/daml/ledger/api/v1/admin/config_management_service.proto com/daml/ledger/api/v1/admin/package_management_service.proto com/daml/ledger/api/v1/admin/participant_pruning_service.proto com/daml/ledger/api/v1/admin/party_management_service.proto com/daml/ledger/api/v1/command_completion_service.proto com/daml/ledger/api/v1/command_service.proto com/daml/ledger/api/v1/command_submission_service.proto com/daml/ledger/api/v1/commands.proto com/daml/ledger/api/v1/completion.proto com/daml/ledger/api/v1/event.proto com/daml/ledger/api/v1/experimental_features.proto com/daml/ledger/api/v1/ledger_configuration_service.proto com/daml/ledger/api/v1/ledger_identity_service.proto com/daml/ledger/api/v1/ledger_offset.proto com/daml/ledger/api/v1/package_service.proto com/daml/ledger/api/v1/testing/reset_service.proto com/daml/ledger/api/v1/testing/time_service.proto com/daml/ledger/api/v1/transaction.proto com/daml/ledger/api/v1/transaction_filter.proto com/daml/ledger/api/v1/transaction_service.proto com/daml/ledger/api/v1/value.proto com/daml/ledger/api/v1/version_service.proto')
Execution platform: @io_tweag_rules_nixpkgs//nixpkgs/platforms:host

Use --sandbox_debug to see verbose messages from the sandbox
panic: proto: extension number 65020 is already registered on message google.protobuf.FieldOptions
See https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict


goroutine 1 [running]:
google.golang.org/protobuf/reflect/protoregistry.glob..func1(0x7fd196a5f980, 0xc0000eb410, 0x8a78a0, 0xc000099550, 0xc0000eb410)
        external/org_golang_google_protobuf/reflect/protoregistry/registry.go:54 +0x25f
google.golang.org/protobuf/reflect/protoregistry.(*Types).RegisterExtension(0xc00009a540, 0x8af468, 0xc0000eb3f0, 0x0, 0x0)
        external/org_golang_google_protobuf/reflect/protoregistry/registry.go:552 +0x6ae
github.com/golang/protobuf/proto.RegisterExtension(...)
        external/com_github_golang_protobuf/proto/registry.go:279
github.com/pseudomuto/protoc-gen-doc/extensions/validator_field.init.0()
        external/com_github_pseudomuto_protoc_gen_doc/extensions/validator_field/validator_field.go:19 +0x127
--doc_out: protoc-gen-doc: Plugin failed with status code 2.
Target //ledger-api/grpc-definitions:ledger-api-docs failed to build
INFO: Elapsed time: 0.627s, Critical Path: 0.31s
INFO: 2 processes: 2 internal.
FAILED: Build did NOT complete successfully

This looks like somehow we are including multiple variations of the same proto but I don’t understand how. I tried to play around with how we include the googleapis which seems one potential culprit but not the only one.

aherrmann-da pushed a commit that referenced this issue Nov 25, 2021
Building `//ledger-api/grpc-definitions:ledger-api-docs` [failed
with](#11761 (comment))
```
panic: proto: extension number 65020 is already registered on message google.protobuf.FieldOptions
```

Go dependencies are now pulled in via Gazelle. By default Gazelle will
generate new proto rules for any `.proto` files encountered in third
party Go dependencies. However, many of these already have pregenerated
`.pb.go` files generate with the appropriate configuration.

The problem can be avoided by configuring Gazelle to not generate new
proto rules, but instead use pre-existing `.pb.go` files.

For reference the field number is set in
[go-proto-validators](https://github.com/mwitkow/go-proto-validators/blob/32a686adf8b5194d3ea07d632d49b6fb344af678/validator.proto#L19)
which is an indirect dependency through protoc-gen-doc.

In this case we need to update protoc-gen-validate to v0.6.2 to include
bufbuild/protoc-gen-validate@4f41f10
which fixes unknown label errors.
aherrmann-da added a commit that referenced this issue Nov 25, 2021
* Try to upgrade protobuf docs plugin

changelog_begin
changelog_end

* Fix extension number 65020 is already registered

Building `//ledger-api/grpc-definitions:ledger-api-docs` [failed
with](#11761 (comment))
```
panic: proto: extension number 65020 is already registered on message google.protobuf.FieldOptions
```

Go dependencies are now pulled in via Gazelle. By default Gazelle will
generate new proto rules for any `.proto` files encountered in third
party Go dependencies. However, many of these already have pregenerated
`.pb.go` files generate with the appropriate configuration.

The problem can be avoided by configuring Gazelle to not generate new
proto rules, but instead use pre-existing `.pb.go` files.

For reference the field number is set in
[go-proto-validators](https://github.com/mwitkow/go-proto-validators/blob/32a686adf8b5194d3ea07d632d49b6fb344af678/validator.proto#L19)
which is an indirect dependency through protoc-gen-doc.

In this case we need to update protoc-gen-validate to v0.6.2 to include
bufbuild/protoc-gen-validate@4f41f10
which fixes unknown label errors.

* ./fmt

* Expose gRPC status.proto for Haskell bindings

* Update Gazelle to support embedsrcs on Windows

`protoc-gen-doc` relies on `go:embed` file embedding
https://github.com/pseudomuto/protoc-gen-doc/blob/2dde01902b28e8b2201d935467bc4308ec912255/resources.go#L8.
Gazelle supports `embedsrcs`, however, it did not generate the attribute
correctly on Windows due to the different directory separator. This is
fixed in bazel-contrib/bazel-gazelle#1101.

* Add gazelle to compatibility workspace

It's loaded into `@daml`'s top-level `BUILD` file and ends up being a
dependency of the compatibility workspace as well.

* shift go_googleapis import

* Delete dead code

protobuf is imported transitively.

* Document how to add Go dependencies

Co-authored-by: Moritz Kiefer <[email protected]>
Co-authored-by: Andreas Herrmann <[email protected]>
cocreature added a commit that referenced this issue Nov 25, 2021
Builds upon #1180 to render the oneof fields a bit nicer.

Note that we also need to kill newlines in the descriptions. That’s
not an issue in the plugin but in how rst renders tables.

fixes #11761

changelog_begin
changelog_end
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

4 participants