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

[rlabrecque]

Bug Report

Version

master: afa9d9d

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?