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

tonic_build::prost::Builder proto_path function panics #357

Closed
petratbtl opened this issue May 20, 2020 · 7 comments
Closed

tonic_build::prost::Builder proto_path function panics #357

petratbtl opened this issue May 20, 2020 · 7 comments

Comments

@petratbtl
Copy link

Bug Report

Version

tonic-build = "0.2.0"
tonic = "0.2.1"
prost = "0.6"

Platform

Linux R1 5.3.0-51-generic #44~18.04.2-Ubuntu SMP Thu Apr 23 14:27:18 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

Crates

tonic-build

Description

I added the following code in build.rs:

fn main() -> Result<(), Box<dyn std::error::Error>> {
    tonic_build::configure()
        .build_server(false)
        .out_dir("proto/generated/")
        .proto_path("../proto/generated")
        .compile(&["proto/service.proto"], &["proto/"])?;

    Ok(())
}

I expected that I will be able to use the package "service" in following way in main.rs:

mod service {
    tonic::include_proto!("service");
}

Instead, the build failed with the following message:

--- stderr
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error("expected identifier")', src/libcore/result.rs:1165:5

Build, and everything else works if I'm not using proto_path() in Builder, but then I can't use include_proto! (I have to use include! instead).
Bug is probably somewhere in the Builder in compiling protos using proto_path(); or maybe I'm using it wrong.

@LucioFranco
Copy link
Member

Would you be able to show me what your proto looks like? (sorry for the late reply been on vacation)

@ajguerrer
Copy link

ajguerrer commented Jun 11, 2020

@petratbtl Using out_dir will generate the proto files outside of OUT_DIR, so you will need to follow the advice here and use include!.

include_proto! itself just does:

include!(concat!(env!("OUT_DIR"), concat!("/", $package, ".rs")));

@petratbtl
Copy link
Author

petratbtl commented Jun 11, 2020

@LucioFranco I was on the vacation, too so I haven't seen this, sorry. I'll find proto and paste it here. @ajguerrer tried that also, works but only if I comment out the line with proto_path.The documentation states the following for proto_path: "Set the path to where tonic will search for the Request/Response proto structs live relative to the module where you call include_proto!." So from this quote, include_proto! is supposed to be used with proto_path set relative to the module using include_proto? Or I'm getting it wrong.

@petratbtl
Copy link
Author

petratbtl commented Jun 11, 2020

Would you be able to show me what your proto looks like? (sorry for the late reply been on vacation)

syntax = "proto3";

package service.proto;


service Greeter {
    // Sends a greeting
    rpc SayHello (HelloRequest) returns (HelloReply) {}
  }
  
  // The request message containing the user's name.
  message HelloRequest {
    string name = 1;
  }
  
  // The response message containing the greetings
  message HelloReply {
    string message = 1;
  }

Here you go, it's proto from examples...

@ajguerrer
Copy link

@petratbtl I think proto_path is for situations in which the messages are in a separate location from the service that makes use of them.

Suppose the generated messages are output to some other module (or crate) messages::hello. Then you might need to generate the service with .proto_path("messages::hello") so that the messages can be found within the generated service code.

For your purposes, I don't think proto_path is necessary. If you ever end up using it, then the path arguments should be module path syntax, not file path syntax.

I just started looking at tonic, so I'm a little unsure if that's how this feature was intended to be used but I'm sure @LucioFranco can clear up my mistakes if needed.

@LucioFranco
Copy link
Member

Yup, that is correct, it is to be used to point to a different module path where structs with the correct names exist.

@petratbtl
Copy link
Author

@LucioFranco & @ajguerrer thanks for explanation and help! I'll close the issue.

dfinity-bot pushed a commit to dfinity/ic that referenced this issue Mar 2, 2022
Move public http adapter protobufs to ic-protobuf

- moves public parts of http adapter to ic-protobuf
- http adapter service stays inside `canister_http/adapter` since only needed for server
- versioned

Questions:
- [This](hyperium/tonic#357 (comment)) comment suggest that we could use `proto_path` to point to the message types in ic-protobuf instead of importing them. 
- Could use [build_server](https://docs.rs/tonic-build/0.4.1/tonic_build/struct.Builder.html#method.build_server) or [build_client](https://docs.rs/tonic-build/0.4.1/tonic_build/struct.Builder.html#method.build_server) to limit generation. In my case both server and client is needed for tests.

Jira: NET-884 

See merge request dfinity-lab/public/ic!3433
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

3 participants