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

Support cosmos-sdk-proto without transport feature of tonic #226

Closed
devashishdxt opened this issue May 13, 2022 · 7 comments · Fixed by #230
Closed

Support cosmos-sdk-proto without transport feature of tonic #226

devashishdxt opened this issue May 13, 2022 · 7 comments · Fixed by #230

Comments

@devashishdxt
Copy link
Contributor

To call gRPC methods from browsers with grpc-web using webassembly, we need to disable transport feature of tonic which is not currently exposed in cosmos-sdk-proto.

@tony-iqlusion
Copy link
Member

There have been a few PRs that attempted to do this: #88 #90 #97 #157

It's a feature we're open to supporting, however none of the PRs actually implemented the functionality in such a way that avoided breaking non-WASM gRPC clients

@devashishdxt
Copy link
Contributor Author

devashishdxt commented May 14, 2022

I think the main reason for this is the use of git submodules to pull proto files. If we generate code for .proto files in build.rs, we can manage tonic and tonic-build features at compile time.

It is a bit difficult to do in current design because code for .proto files is generated by proto-build that doesn't know anything about compile time features of cosmos-sdk-proto. Also, we cannot generate code for .proto files in build.rs of cosmos-sdk-proto because we're using submodules and I'm not sure how that will work when someone adds cosmos-sdk-proto as their dependency and build it.

The right solution should be to include .proto files in comsos-sdk-proto's source and build Rust code using build.rs.

@tony-iqlusion
Copy link
Member

tony-iqlusion commented May 14, 2022

The downside of this (beyond longer build times) is protoc must be installed to compile the crate

I think the main reason for this is the use of git submodules to pull proto files.

Even if we did move to a build.rs-based approach, we would still need to vendor the proto files into the crate somehow, similar to how the generated code is currently vendored, and that requires a source of truth for the protos, which is currently a 3rd party git repository (i.e. cosmos-sdk)

Whether or not we use a git submodule to do that seems orthogonal, but also, given that requirement I'm not sure what approach would be better than a git submodule.

@devashishdxt
Copy link
Contributor Author

protoc must be installed to compile the crate

prost-build builds it if it is not installed.

Even if we did move to a build.rs-based approach, we would still need to vendor the proto files into the crate somehow, similar to how the generated code is currently vendored, and that requires a source of truth for the protos, which is currently a 3rd party git repository (i.e. cosmos-sdk)

Yes. We'll have to include .proto files in crate sources. The problem with git submodules is that it pulls all the source code of the repository and we only need .proto files. It'll be better if we can only extract .proto files from git repository and include those in sources of cosmos-sdk-proto.

Whether or not we use a git submodule to do that seems orthogonal, but also, given that requirement I'm not sure what approach would be better than a git submodule.

One approach is that instead of having a proto-build crate, just create a proto-copy script which copies .proto files from each git submodule to cosmos-sdk-proto based on configured versions. Then cosmos-sdk-proto's build.rs can generate rust code.

@tony-iqlusion
Copy link
Member

prost-build builds it if it is not installed.

It does? That must be new. Does it compile protoc from source, and if so, does that require an entire C++ toolchain must be present? If so, that still seems bad.

One approach is that instead of having a proto-build crate, just create a proto-copy script which copies .proto files from each git submodule to cosmos-sdk-proto based on configured versions. Then cosmos-sdk-proto's build.rs can generate rust code.

That sounds exactly like what I just described. Note that this approach does not get rid of the git submodule which is the source of truth for the protos. It just replaces copying the generated Rust code with copying the protos.

@tony-iqlusion
Copy link
Member

Here's what the prost-build docs say:

https://github.com/tokio-rs/prost/blob/master/prost-build/README.md

prost-build uses protoc to parse the proto files. There are a few ways to make protoc
available for prost-build.

The first option is to include protoc in your PATH this
can be done by following the protoc install instructions. In addition, its possible to
pass the PROTOC=<my/path/to/protoc> environment variable.

The second option is to provide the vendored feature flag to prost-build. This will
force prost-build to compile protoc from the bundled source. This will require that
you have the correct dependencies installed include a C++ toolchain, cmake, etc. For
more info on what the required dependencies are check here.

...so there is a vendored option we could enable, but it would require compiling protoc from source, which would add considerably to compile times and requires an entire C++ toolchain is available

tony-iqlusion added a commit that referenced this issue May 16, 2022
Closes #226

Adds an on-by-default `grpc-transport` feature which enables the
corresponding `transport` feature in `tonic`.

To make this work with generated code, the generated protos are patched
to feature-gate the `tonic::transport`-dependent code.
@tony-iqlusion
Copy link
Member

I opened a PR to add a grpc-transport feature: #230

We can potentially investigate moving to a build.rs-based approach, but I think that's a bigger decision than just this feature, and it wasn't too difficult to add on top of the pre-generated code.

tony-iqlusion added a commit that referenced this issue May 16, 2022
Closes #226

Adds an on-by-default `grpc-transport` feature which enables the
corresponding `transport` feature in `tonic`.

To make this work with generated code, the generated protos are patched
to feature-gate the `tonic::transport`-dependent code.
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

Successfully merging a pull request may close this issue.

2 participants