Tonic: Client/Server /can be/ duplicated into their include files

[rlabrecque]

Bug Report

Version

master: afa9d9d236b7ca96d70d696ab38aadac7cdf41ff

Platform

Linux RILEY-LT 4.19.43-microsoft-standard #1 SMP Mon May 20 19:35:22 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

Crates

cargo-build

Description

When using includes it seems like the client/server are generated into the include file output. This only seems to happen under certain conditions, I believe related to compiling multiple protos at once via tonic_build::configure().compile(&["proto/health.proto", "proto/test.proto"], &["proto"]).unwrap();

Repro here:

https://github.com/rlabrecque/tonic_include_repro

Specifically see: https://github.com/rlabrecque/tonic_include_repro/blob/master/src/proto/google.protobuf.rs

[LucioFranco]

@rlabrecque So I think this is an artifact of how prost builds its protobufs. I'm not actually sure why that file is even empty. So I think to fix this we must go into prost-build.

@danburkert hey! you might have an idea about this?

[rlabrecque]

It's actually slightly different than I originally thought, everything inside mod client/server gets dumped into every file compiled. The request/response types properly go into their package's output file.

See: https://github.com/rlabrecque/tonic_include_repro/blob/master/src/proto/test.rs This contains both HealthClient and TestClient

[LucioFranco]

So I think the issue I ran into with the codegen was that https://docs.rs/prost-build/0.5.0/prost_build/trait.ServiceGenerator.html#tymethod.generate you don't know if this is being called on the same file or a different file. So the goal was on one compile_protos step to aggregate ALL the services and throw them into a general client/server mod.

https://docs.rs/prost-build/0.5.0/prost_build/trait.ServiceGenerator.html#tymethod.generate

I'm not sure what the best we can do without the context from prost? It may be worth it to go into prost and modify it to provide some sort of context so you can know which file's buf you might be writing too.

[uschen]

Using tower-grpc with the same build file (gathers all proto files with services in to the array) doesn't have the same issue

[uschen]

wonder the priority of this issue can be bumped

[LucioFranco]

@uschen not much we can do right now, currently, looking at different strategies to support other types of message types via codegen. This should allow us to explore ways to decouple our build process from that of prost-build.

Is there a reason you can't merge all the services into one file?

[rlabrecque]

I'm curious why it worked fine on tower-grpc as well though. It seems like it's possible and this is just a regression in the rewrite.

[mineralres]

@LucioFranco We have defined many services before, it's much more clear to split them into seperate files

[LucioFranco]

So I took a deeper look at this morning.

@rlabrecque looking at your repro example, I got it working by changing two things.

diff --git a/build.rs b/build.rs
index b3de304..e95da8e 100644
--- a/build.rs
+++ b/build.rs
@@ -1,5 +1,11 @@
 fn main() {
     tonic_build::configure()
-        .out_dir("src/proto")
-        .compile(&["proto/health.proto", "proto/test.proto"], &["proto"]).unwrap();
+        // .out_dir("src/proto")
+        .compile(&["proto/health.proto"], &["proto"])
+        .unwrap();
+
+    tonic_build::configure()
+        // .out_dir("src/proto")
+        .compile(&["proto/test.proto"], &["proto"])
+        .unwrap();
 }
diff --git a/src/lib.rs b/src/lib.rs
index 3664c35..59fb401 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1,4 +1,9 @@
 pub mod pb {
-    tonic::include_proto!("grpc.health.v1");
-    tonic::include_proto!("test");
-}
\ No newline at end of file
+    mod health {
+        tonic::include_proto!("grpc.health.v1");
+    }
+
+    mod test {
+        tonic::include_proto!("test");
+    }
+}

So the issue I think is that when you invoke compile in build.rs it expects that all protos are within the same package. The issue you have here is that test.proto is in its own package. So I believe that prost here understood that the last proto file in the list is the one to build the package for. By changing the build to do two separate compile stages I was able to build a codegen for both packages with their services.

Now, there will be duplicated services like the health one showing up in the test package but I'm not sure there is much we can do about that since we don't know which services directly you want.

So please let me know if that solution works for you both.

In the long run I want to revaluate how we do codegen and how prost does it as well. This has already been started with #98.

[uschen]

So the issue I think is that when you invoke compile in build.rs it expects that all protos are within the same package. The issue you have here is that test.proto is in its own package. So I believe that prost here understood that the last proto file in the list is the one to build the package for. By changing the build to do two separate compile stages I was able to build a codegen for both packages with their services.

still have to use 'tower-grpc-build' as the argument. If pass all protos across multiple folders/services, in one call, it works.

    // generate
    let mut prost_config = prost_build::Config::new();
    prost_config.out_dir(OUT_DIR).retain_enum_prefix();
    tower_grpc_build::Config::from_prost(prost_config)
        .enable_client(true)
        .build(
            &_protos.iter().map(AsRef::as_ref).collect::<Vec<_>>(),
            &[
                "..",
                "../vendor/ssomethjing",
                "../vendor/github.com/gogo/protobuf",
            ],
        )
        .unwrap_or_else(|e| panic!("protobuf compilation failed: {}", e));

[LucioFranco]

still have to use 'tower-grpc-build' as the argument. If pass all protos across multiple folders/services, in one call, it works.

Sure, though I don't think the way tower-grpc-build worked was the correct approach. I think the isolated approach that tonic provides is better. Though, maybe I'm missing what you are looking to achieve? Does the provided solution not work?

[uschen]

@LucioFranco let me give your solution a try. My bad, haven't get a chance yet. Should have mentioned that

[parasyte]

I hit this problem with a different repro case. Compiling two proto files with the same package name defining unique services will output Rust source code like:

// services for first proto file in package "foo"
pub mod client {}
pub mod server {}

// services for second proto file in package "foo"
pub mod client {}
pub mod server {}

Which then leads to errors:

error[E0428]: the name `client` is defined multiple times
error[E0428]: the name `server` is defined multiple times

I have to merge all proto files for the single package into one file to workaround this issue.

[parasyte]

There are a few different issues that I've found so far. First, take this set of proto files:

Proto files

proto/hello.proto

syntax = "proto3";

import "types.proto";

package helloworld;

service Greeting {
  rpc Hello (Message) returns (Response) {}
}

proto/goodbye.proto

syntax = "proto3";

import "types.proto";

package helloworld;

service Farewell {
  rpc Goodbye (Message) returns (Response) {}
}

proto/types.proto

syntax = "proto3";

package helloworld;

message Message {
  string say = 1;
}

message Response {
  string say = 1;
}

And the following build.rs:

fn main() {
    tonic_build::compile_protos("proto/hello.proto").unwrap();
    tonic_build::compile_protos("proto/goodbye.proto").unwrap();
    println!("cargo:rerun-if-changed=proto/hello.proto");
    println!("cargo:rerun-if-changed=proto/goodbye.proto");
    println!("cargo:rerun-if-changed=proto/types.proto");

This fails to work because building goodbye.proto completely overwrites the output file generated for hello.proto.

Ok, maybe the Builder type is smarter, since you can provide a list of paths to multiple proto files? Let's update build.rs:

fn main() {
    tonic_build::configure()
        .compile(&["proto/hello.proto", "proto/goodbye.proto"], &["proto"])
        .unwrap();
    println!("cargo:rerun-if-changed=proto/hello.proto");
    println!("cargo:rerun-if-changed=proto/goodbye.proto");
    println!("cargo:rerun-if-changed=proto/types.proto");
}

And _this_ also fails because now the generated output has duplicate client and server modules for the two different services defined in the helloworld package.

[rib]

I've just hit this issue too but in my case the proto files are from Open Match, (a third-party that's not conceptually under my control), so it's really not ideal to have to concatenate files.

Incidentally, I'm also seeing that dependencies seem a bit awkward to deal with currently because you have to manually check what dependencies some .proto spec has and make sure to include those with the right mod nesting. For example; open match's backend.proto includes api/messages.proto which then includes a third-party google/rpc/status.proto (which declares its package as google.rpc). Before I can include/use generated openmatch code I have to explicitly include the google.rpc code under a mod google { mod rpc { }} } namespace like:

pub mod google {
    pub mod rpc {
        tonic::include_proto!("google.rpc");
    }
}
pub mod openmatch {
    tonic::include_proto!("openmatch");
}

Maybe this is expected/intentional? I figured I'd mention it either way in case it's relevent to also consider in relation to any changes that might be necessary to support packages split between multiple .proto files.

[rib]

For reference here's a minimal reproduction for the case I have: https://github.com/rib/tonic-issue-38-repro

[LucioFranco]

Hi all, I believe #173 fixed this issue, please let me know if you run into any issues.

[LucioFranco]

I am gonna close this, if someone is seeing this again we can reopen, so if you do see it please comment here, thanks! :)