feat(build): Support compiling well-known protobuf types

[tdyas]

Motivation

Tonic does not currently handle the case when Prost is configured to compile well-known protobuf types instead of just referencing prost_types. See #521 for a full description of the problem.

Solution

Allow the user to configure Tonic to compile well-known protobuf types. That configuration is passed through to Prost.

[tdyas]

Caveats:

• This still needs a proper test. I did not see code generation tests within the tonic-build crate. Are they somewhere else in the code base? Never mind, I see them under tests/.
• Now that there are two options (proto_path and compile_well_known_types) as arguments everywhere, it would probably be useful to pass around some sort of a context struct instead.

[tdyas]

Added a test of using tonic-build with compiling well-known types.

Note: I had to modify some of the well-known protos to add doctest ignore attributes on indented code blocks in the proto comments because Rust was treating them as Rust doctests. This is probably an issue that should be reported to Prost.

[LucioFranco]

With prost releasing a new version supporting Bytes (not sure if prost-types got an update) can we just update prost-types instead of trying to side step it here? This seems like a pretty extreme change for something that should honestly be a pretty trivial update to prost?

[tdyas]

With prost releasing a new version supporting Bytes (not sure if prost-types got an update) can we just update prost-types instead of trying to side step it here? This seems like a pretty extreme change for something that should honestly be a pretty trivial update to prost?

No, because I do not believe prost-types will be compiled with the Bytes support because Bytes support is entirely optional, Vec<u8> is still the default. First, note that enabling Bytes is a non-default config option: https://docs.rs/prost-build/0.7.0/prost_build/struct.Config.html#method.bytes

Moreover, you can see the updated version of google.protobuf.Any in prost-types here: https://github.com/danburkert/prost/blob/79f0dfd8ed703fcce27f053c3b29c360c7ead5cb/prost-types/src/protobuf.rs#L1033

Note that the value field in google.protobuf.Any is still compiled as a Vec<u8>, not a Bytes.

Also, I submit that the fact that my ultimate purpose is to gain access to Bytes support is somewhat irrelevant to this PR. The goal of this PR is to allow users of tonic-build the option of compiling the well-known protobuf types with Prost, which is still a bug in tonic-build, regardless of my ultimate purpose.

[LucioFranco]

Okay, that is fair, I need to think about this a bit more but I'd like to first ship a new version of tonic ASAP that works with tokio 1.0 then I plan on working on some enhancements. Since I don't consider this a major blocker to that I would like to look at this once we get that version out. Hopefully that version will go out this week. I really appreciate the time you took to look into this :)

[tdyas]

Since I don't consider this a major blocker to that I would like to look at this once we get that version out.

I agree, this PR is not a major blocker.

[tdyas]

Just wanted to provide a gentle nudge now that v0.4 is out. I'd love to get some feedback on this PR to keep it moving.

My main question is how to thread through the compile_well_known_types value. Should the generation methods take a context struct carrying proto_path and compile_well_known_types instead of those being passed as separate parameters?

[tustvold]

FYI instead of copying the full proto definitions, it is sufficient to create a dummy proto file imports them from the environment

syntax = "proto3";

package google.protobuf;

import "google/protobuf/any.proto";
import "google/protobuf/api.proto";
import "google/protobuf/descriptor.proto";
import "google/protobuf/duration.proto";
import "google/protobuf/empty.proto";
import "google/protobuf/field_mask.proto";
import "google/protobuf/source_context.proto";
import "google/protobuf/struct.proto";
import "google/protobuf/timestamp.proto";
import "google/protobuf/type.proto";
import "google/protobuf/wrappers.proto";

[tdyas]

I removed the vendored proto files as suggested, and then rebased (with some churn for some typos of mine).

Thoughts on whether a context structure makes sense? With three parameters now in some of the generation methods (e.g., emit_package, proto_path, and compile_well_known_types), that might be a good refactor to simplify the function signatures.

[davidpdrsn]

I'm not super familiar with the code but the changes look pretty simple. Sounds like a useful feature to have as well.

The final 👍 should probably come from @LucioFranco though 😊

[dfreese]

Apologies for the drive by comment, but I believe this functionality was already supported with Builder::compile_with_config. That's what we used to handle more specific configurations. Here is an example of how we used compile_with_config to specify this.

[tdyas]

Apologies for the drive by comment, but I believe this functionality was already supported with Builder::compile_with_config. That's what we used to handle more specific configurations. Here is an example of how we used compile_with_config to specify this.

So how did you work-around #521 then? Or was there another solution?

[dfreese]

Apologies for the drive by comment, but I believe this functionality was already supported with Builder::compile_with_config. That's what we used to handle more specific configurations. Here is an example of how we used compile_with_config to specify this.

So how did you work-around #521 then? Or was there another solution?

Thanks for pointing that out. I needed to look more closely. Where things differed is I was specifying the well known types in a different crate and then pulling them into the current crate using extern_path. That doesn't appear to run into the same issue.