build: ditch proto-compiler, use buf #2619

conorsch · 2023-05-22T23:49:43Z

Overhauls the protobuf codegen by standardizing on buf, along with associated tooling like protoc-gen-prost and proto-gen-tonic. The output dirs for the supported langs are now:

Rust: ./crates/proto/src/gen/
Go: ./proto/go/gen/

We've removed the proto-compiler tool from the repo. Also, we no longer vendor upstream protos by committing directly to our repo; instead, we declare dependencies in the buf yaml files and offload the resolution to buf generate.

One major regression is that gRPC server reflection no longer works. These changes remove the proto_descriptor.bin build.rs logic, with no replacement in the new buf logic. As of right now, preview supports reflection:

    ❯ grpcurl grpc.testnet-preview.penumbra.zone:443 list
    cosmos.base.tendermint.v1beta1.Service
    grpc.reflection.v1alpha.ServerReflection
    penumbra.client.v1alpha1.ObliviousQueryService
    penumbra.client.v1alpha1.SpecificQueryService
    penumbra.client.v1alpha1.TendermintProxyService
    penumbra.custody.v1alpha1.CustodyProtocolService
    penumbra.narsil.ledger.v1alpha1.LedgerService
    penumbra.view.v1alpha1.ViewAuthService
    penumbra.view.v1alpha1.ViewProtocolService

but this code does not:

    ❯ grpcurl -plaintext localhost:8080 list
    Failed to list services: server does not support the reflection API

We'll circle back on reflection support.

Notably the tonic rpc defs are now in their own files, a side-effect of using the prost-gen-tonic plugin for buf. The original proto files, without the .tonic.proto suffix, now automatically include! their tonic neighbors, so the interface hasn't changed.

Refs #2184.

hdevalence · 2023-05-24T03:15:42Z

@conorsch Question in re: #2184 (comment) : is there any other regression other than the reflection data not being updated? If not, I think it could be good to merge this now, so that we don't generate conflicts with other changes to .proto files, and loop back on the reflection issue.

conorsch · 2023-05-24T15:35:06Z

So far, so good. I'll get this cleaned up and RFR today, then circle back on the reflection!

Overhauls the protobuf codegen by standardizing on `buf`, along with associated tooling like `protoc-gen-prost` and `proto-gen-tonic`. The output dirs for the supported langs are now: * Rust: ./crates/proto/src/gen/ * Go: ./proto/go/gen/ We've removed the proto-compiler tool from the repo. Also, we no longer vendor upstream protos by committing directly to our repo; instead, we declare dependencies in the buf yaml files and offload the resolution to `buf generate`. One major regression is that gRPC server reflection no longer works. These changes remove the `proto_descriptor.bin` build.rs logic, with no replacement in the new buf logic. As of right now, preview supports reflection: ❯ grpcurl grpc.testnet-preview.penumbra.zone:443 list cosmos.base.tendermint.v1beta1.Service grpc.reflection.v1alpha.ServerReflection penumbra.client.v1alpha1.ObliviousQueryService penumbra.client.v1alpha1.SpecificQueryService penumbra.client.v1alpha1.TendermintProxyService penumbra.custody.v1alpha1.CustodyProtocolService penumbra.narsil.ledger.v1alpha1.LedgerService penumbra.view.v1alpha1.ViewAuthService penumbra.view.v1alpha1.ViewProtocolService but this code does not: ❯ grpcurl -plaintext localhost:8080 list Failed to list services: server does not support the reflection API We'll circle back on reflection support. Notably the tonic rpc defs are now in their own files, a side-effect of using the `prost-gen-tonic` plugin for buf. The original proto files, without the `.tonic.proto` suffix, now automatically `include!` their tonic neighbors, so the interface hasn't changed. Refs #2184.

conorsch · 2023-05-24T17:04:17Z

The CI check for breaking changes in buf is failing, reporting a single change to the TM types, due to the switch from vendoring to dynamic dependency resolution. Running locally, I see:

❯ buf breaking --against 'https://github.com/penumbra-zone/penumbra.git#branch=main,subdir=crates/proto' proto/
tendermint/types/types.proto:1:1:Previously present enum "BlockIDFlag" was deleted from file.

Indeed, on main the vendored proto had a slightly different case to its type:

❯ rg -i 'enum BlockIDFlag'
crates/proto/rust-vendored/tendermint/types/types.proto
13:enum BlockIDFlag {

crates/proto/src/gen/tendermint.types.rs
228:pub enum BlockIdFlag {

Whereas on this branch, there's only the one casing:

❯ rg -i 'enum BlockIDFlag'
crates/proto/src/gen/tendermint.types.rs
228:pub enum BlockIdFlag {

Our code compiles fine, so I think we can disregard this as a blocker.

conorsch · 2023-05-24T17:08:17Z

Regarding testing to ensure we don't have regressions (other than disabling gRPC server reflection support), I compared the output from pcli q tx <id>, built from this branch and from a precompiled binary for v0.52.2. No differences.

conorsch · 2023-05-24T17:09:10Z

.github/workflows/buf-pull-request.yml

@@ -33,7 +33,7 @@ jobs:
      - name: Check for breaking changes
        uses: bufbuild/buf-breaking-action@v1
        with:
-          input: 'crates/proto'
+          input: 'proto'
          # The 'main' branch of the GitHub repository that defines the module.
          against: 'https://github.com/${GITHUB_REPOSITORY}.git#branch=main,subdir=crates/proto'


N.B. the against line should be updated post-merge, but is accurate as of right now, showing us potentially breaking changes in the CI output of this PR.

hdevalence · 2023-05-24T18:00:45Z

crates/bin/pclientd/src/lib.rs

-                    .add_service(tonic_web::enable(
-                        tonic_reflection::server::Builder::configure()
-                            .register_encoded_file_descriptor_set(
-                                penumbra_proto::FILE_DESCRIPTOR_SET,
-                            )
-                            .build()
-                            .with_context(|| "could not configure grpc reflection service")?,
-                    ))


Let's make a follow-up issue to restore grpc reflection, and copy this code snippet into the issue description, so that we don't lose track of it once it's deleted.

hdevalence · 2023-05-24T18:01:40Z

crates/proto/src/gen/penumbra.core.transparent_proofs.v1alpha1.rs

+/// Encoded file descriptor set for the `penumbra.core.transparent_proofs.v1alpha1` package
+pub const FILE_DESCRIPTOR_SET: &[u8] = &[
+    0x0a, 0xe1, 0x0b, 0x0a, 0x42, 0x70, 0x65, 0x6e, 0x75, 0x6d, 0x62, 0x72, 0x61, 0x2f, 0x63, 0x6f,
+    0x72, 0x65, 0x2f, 0x74, 0x72, 0x61, 0x6e, 0x73, 0x70, 0x61, 0x72, 0x65, 0x6e, 0x74, 0x5f, 0x70,
+    0x72, 0x6f, 0x6f, 0x66, 0x73, 0x2f, 0x76, 0x31, 0x61, 0x6c, 0x70, 0x68, 0x61, 0x31, 0x2f, 0x74,
+    0x72, 0x61, 0x6e, 0x73, 0x70, 0x61, 0x72, 0x65, 0x6e, 0x74, 0x5f, 0x70, 0x72, 0x6f, 0x6f, 0x66,


👀 Looks like the new setup is putting the file descriptor set in the output directly?

Follow up to #2619. Re-enables gRPC server reflection by gathering up each proto definitions `FILE_DESCRIPTOR_SET` and returning a vec of all those via a public function in the proto module. Closes #2629.

Follow-up post-merge to changes in #2619.

Follow up to #2619. Re-enables gRPC server reflection by gathering up each proto definitions `FILE_DESCRIPTOR_SET` and returning a vec of all those via a public function in the proto module. Closes #2629.

The overhaul of the protobuf codegen logic from #2619 was incomplete: we encountered problems with serde implementations. Wasn't able to sort them quickly, and we're about to release Testnet 53, so I'm backing out the recent changes to unblock in-flight PRs. Will return to disentangling the protobuf story on subsequent sprint. Revert "fix: re-enable grpc server reflection" This reverts commit f8d09f6. Revert "ci: update buf target dir" This reverts commit 3e7b4c7. Revert "dev: add buf breaking change detection" This reverts commit 724ccf6. Revert "ci: eliminate the chore of buf mod update" This reverts commit 1f28bb2. Revert "build: ditch proto-compiler, use buf" This reverts commit 53b56cb.

conorsch temporarily deployed to smoke-test May 22, 2023 23:49 — with GitHub Actions Inactive

conorsch force-pushed the 2184-let-there-be-buf branch from 3e346a4 to b90f613 Compare May 23, 2023 23:08

conorsch had a problem deploying to smoke-test May 23, 2023 23:08 — with GitHub Actions Error

conorsch force-pushed the 2184-let-there-be-buf branch from b90f613 to 9ed2138 Compare May 23, 2023 23:12

conorsch had a problem deploying to smoke-test May 23, 2023 23:12 — with GitHub Actions Error

conorsch force-pushed the 2184-let-there-be-buf branch from 9ed2138 to 011e5b3 Compare May 23, 2023 23:26

conorsch temporarily deployed to smoke-test May 23, 2023 23:26 — with GitHub Actions Inactive

conorsch force-pushed the 2184-let-there-be-buf branch from 011e5b3 to 374cb1f Compare May 24, 2023 16:42

conorsch had a problem deploying to smoke-test May 24, 2023 16:42 — with GitHub Actions Error

conorsch force-pushed the 2184-let-there-be-buf branch from 374cb1f to b35aef5 Compare May 24, 2023 16:45

conorsch temporarily deployed to smoke-test May 24, 2023 16:45 — with GitHub Actions Inactive

conorsch marked this pull request as ready for review May 24, 2023 17:08

conorsch commented May 24, 2023

View reviewed changes

hdevalence reviewed May 24, 2023

View reviewed changes

hdevalence approved these changes May 24, 2023

View reviewed changes

hdevalence merged commit 53b56cb into main May 24, 2023

hdevalence deleted the 2184-let-there-be-buf branch May 24, 2023 18:02

conorsch mentioned this pull request May 24, 2023

Re-enable gRPC server reflection #2629

Closed

conorsch mentioned this pull request May 24, 2023

fix: re-enable grpc server reflection #2631

Merged

conorsch added a commit that referenced this pull request May 24, 2023

ci: update buf target dir

dee4240

Follow-up post-merge to changes in #2619.

conorsch added a commit that referenced this pull request May 24, 2023

ci: update buf target dir

00a1629

Follow-up post-merge to changes in #2619.

conorsch added a commit that referenced this pull request May 24, 2023

ci: update buf target dir

3e7b4c7

Follow-up post-merge to changes in #2619.

conorsch mentioned this pull request May 25, 2023

build: revert to proto-compiler #2639

Merged

cratelyn added the protobuf-changes Makes changes to the protobuf definitions. label Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: ditch proto-compiler, use buf #2619

build: ditch proto-compiler, use buf #2619

conorsch commented May 22, 2023 •

edited

Loading

hdevalence commented May 24, 2023

conorsch commented May 24, 2023

conorsch commented May 24, 2023

conorsch commented May 24, 2023

conorsch May 24, 2023

hdevalence May 24, 2023

hdevalence May 24, 2023

build: ditch proto-compiler, use buf #2619

build: ditch proto-compiler, use buf #2619

Conversation

conorsch commented May 22, 2023 • edited Loading

hdevalence commented May 24, 2023

conorsch commented May 24, 2023

conorsch commented May 24, 2023

conorsch commented May 24, 2023

conorsch May 24, 2023

Choose a reason for hiding this comment

hdevalence May 24, 2023

Choose a reason for hiding this comment

hdevalence May 24, 2023

Choose a reason for hiding this comment

conorsch commented May 22, 2023 •

edited

Loading