Remove build.rs #657
Remove build.rs #657
Conversation
|
For CI to pass there are a few upstream fixes that need to happen but locally everything works on my mac. cc @neoeinstein @benesch @Jake-Shadle @danburkert I would love some feedback from yall if you get the chance. |
|
Thanks so much for working on this, @LucioFranco! This seems like a great solution for application crates. The big question I have is how this will interact with other library crates. Let's assume tonic-build exposes the same two methods to configure protoc. What do downstream library crates like opentelemetry-otlp and console-api do? They have to pick some values for protoc/protoc_include. Is your thinking that they'll all just check the generated protobuf code into source control? (IIRC you recently switched console-api to do that?) |
|
Yah I have the same concern mentioned by @benesch, for applications this would be fine but seems like it will a bit of a nightmare with transitive dependencies since this method seems like it would need to leak down via eg feature flags otherwise you will run into cases where some libraries will compile from source and some will require protoc be in a know location or |
Yes, I think overall this is the right solution moving forward (also helps IDEs etc). That said, I think it still is a challenge, for example, how do you ensure that the proto and generated code are updated in lock step? I guess one solution, would be to feature flag the generation in a test (similar to what prost does for prost-types) and running that on CI only where you can install protoc very easily.
Yeah, this is my concern as well. Though I think libs should include their own way to fetch protoc and should/could provide ways for users to provide their own by exposing methods. The one thing I think going for us in this area is that I don't think there are a large amount of libs that use tonic directly and/or with gRPC users can generally include the protobuf themselves. That said, I think the real solution moving forward is to check in the generated protobuf. I wonder, if with this change writing a blog post as well might alleviate some of the pain introduced by explaining the right way to write libraries and applications. |
…o 0.3.0 (#5755) Note: This subsumes #5738, which just attempted to update prost-types to 0.8.0. For Reviewers: The only file I modified by hand is Cargo.toml. Everything else is auto-generated from subsequent commands (`cargo update`, `cargo raze`, `bazel run //tensorboard/data/server:update_protos`) #5738 attempted to update prost-types from 0.7.0 to 0.8.0. This did not work out-of-the-box for several reasons, the most important of which is that we could not successfully build and run the server with mixed versions of prost (0.7.0), prost-types (0.8.0), and prost-build (0.7.0). This PR attempts to upgrade all three of prost, prost-types, and prost-build to the same version at the same time. We also upgrade tonic, tonic-reflection, and tonic-build so that they, too, depend on the same prost version and we don't have multiple prost crates of different versions. * https://crates.io/crates/prost/0.9.0 * https://crates.io/crates/prost-types/0.9.0 * https://crates.io/crates/prost-build/0.9.0 * https://crates.io/crates/tonic/0.6.2 * https://crates.io/crates/tonic-build/0.6.2 * https://crates.io/crates/tonic-reflection/0.3.0 We choose to upgrade prost to 0.9.0 because, why not? We attempted to upgrade to 0.10.* (the latest version) but these versions of prost have a dependency on cmake, which seems to assume cmake is installed on your machine, which it typically is not within Google. There seems to be some work to further refine the dependency on cmake (tokio-rs/prost#620, tokio-rs/prost#657) so hopefully this gets resolved in a future release of prost and a future upgrade to, say, 0.11 will not have the cmake problems. The upgrade was not straightforward for me, a rust and rustboard newb. Several other edits had to be made and commands had to be run to get it to work. This was the incantation that finally worked for me: * Edit Cargo.toml to update versions of prost, prost-types, prost-build, tonic, tonic-reflection, tonic-build and to update any related raze metadata entries. * Run `cargo update -p prost -p prost-types -p prost-build -p tonic -p tonic-reflection -p tonic-build` * Inspect Cargo.lock and compare to raze metadata entries in Cargo.toml. If any of the versions changed (indexmap) then update the associated raze metadata entry. If any of the crates were removed (none were) then remove the associated raze metadata entry. Also, libc version didn't change but I updated its raze metadata anyway since its version seems to have changed in the past. * Run `cargo raze` * Run `bazel test //tensorboard/data/server:update_protos_test` (especially since this is updating proto-related libraries) and witness it FAIL and then run `bazel run //tensorboard/data/server:update_protos`. * Confirm server works with `bazel run -c opt //tensorboard/data/server -- --logdir <some logdir>`
There was a problem hiding this comment.
That said, I think the real solution moving forward is to check in the generated protobuf.
I tend to agree with this. I've moved to check in generated code in our own repositories too, albeit using buf to drive a prost-based generator. This is better than making a difficult-to-verify binary executable part of the distribution or spending several times longer building that executable from source just to do the transformation. Thus, I think the default should be to expect a user BYOprotoc or for the library to pre-generate.
| @@ -1289,6 +1345,7 @@ mod tests { | |||
| let _ = env_logger::try_init(); | |||
| Config::new() | |||
| .service_generator(Box::new(ServiceTraitGenerator)) | |||
| .out_dir(std::env::temp_dir()) | |||
There was a problem hiding this comment.
Would we want to use something like tempdir? It can ensure we don't have test case collisions (if we add more that would collide later) and then cleans up the generated temp directory on drop. Not vital by any means.
There was a problem hiding this comment.
Yeah, we can fix this in another PR but prob a good idea.
| import "google/protobuf/type.proto"; | ||
| import "google/protobuf/any.proto"; | ||
| import "google/protobuf/field_mask.proto"; | ||
| import "google/protobuf/api.proto"; | ||
| import "google/protobuf/descriptor.proto"; | ||
| import "google/protobuf/wrappers.proto"; | ||
| import "google/protobuf/source_context.proto"; | ||
| import "google/protobuf/empty.proto"; | ||
| import "google/protobuf/struct.proto"; | ||
| import "google/protobuf/compiler/plugin.proto"; | ||
| import "google/protobuf/duration.proto"; |
There was a problem hiding this comment.
So I learned that protoc bundles all of these includes so you don't actually need to have a protoc_include env var set with these nor do we need to bundle them into the .crate for prost-build.
Co-authored-by: Marcus Griep <marcus@griep.us>
This reverts commit ac57645.
|
Whatever happened to that blog post? :) |
Sorry, I ran out of time unfortunately :( |
…o 0.3.0 (tensorflow#5755) Note: This subsumes tensorflow#5738, which just attempted to update prost-types to 0.8.0. For Reviewers: The only file I modified by hand is Cargo.toml. Everything else is auto-generated from subsequent commands (`cargo update`, `cargo raze`, `bazel run //tensorboard/data/server:update_protos`) tensorflow#5738 attempted to update prost-types from 0.7.0 to 0.8.0. This did not work out-of-the-box for several reasons, the most important of which is that we could not successfully build and run the server with mixed versions of prost (0.7.0), prost-types (0.8.0), and prost-build (0.7.0). This PR attempts to upgrade all three of prost, prost-types, and prost-build to the same version at the same time. We also upgrade tonic, tonic-reflection, and tonic-build so that they, too, depend on the same prost version and we don't have multiple prost crates of different versions. * https://crates.io/crates/prost/0.9.0 * https://crates.io/crates/prost-types/0.9.0 * https://crates.io/crates/prost-build/0.9.0 * https://crates.io/crates/tonic/0.6.2 * https://crates.io/crates/tonic-build/0.6.2 * https://crates.io/crates/tonic-reflection/0.3.0 We choose to upgrade prost to 0.9.0 because, why not? We attempted to upgrade to 0.10.* (the latest version) but these versions of prost have a dependency on cmake, which seems to assume cmake is installed on your machine, which it typically is not within Google. There seems to be some work to further refine the dependency on cmake (tokio-rs/prost#620, tokio-rs/prost#657) so hopefully this gets resolved in a future release of prost and a future upgrade to, say, 0.11 will not have the cmake problems. The upgrade was not straightforward for me, a rust and rustboard newb. Several other edits had to be made and commands had to be run to get it to work. This was the incantation that finally worked for me: * Edit Cargo.toml to update versions of prost, prost-types, prost-build, tonic, tonic-reflection, tonic-build and to update any related raze metadata entries. * Run `cargo update -p prost -p prost-types -p prost-build -p tonic -p tonic-reflection -p tonic-build` * Inspect Cargo.lock and compare to raze metadata entries in Cargo.toml. If any of the versions changed (indexmap) then update the associated raze metadata entry. If any of the crates were removed (none were) then remove the associated raze metadata entry. Also, libc version didn't change but I updated its raze metadata anyway since its version seems to have changed in the past. * Run `cargo raze` * Run `bazel test //tensorboard/data/server:update_protos_test` (especially since this is updating proto-related libraries) and witness it FAIL and then run `bazel run //tensorboard/data/server:update_protos`. * Confirm server works with `bazel run -c opt //tensorboard/data/server -- --logdir <some logdir>`
…o 0.3.0 (tensorflow#5755) Note: This subsumes tensorflow#5738, which just attempted to update prost-types to 0.8.0. For Reviewers: The only file I modified by hand is Cargo.toml. Everything else is auto-generated from subsequent commands (`cargo update`, `cargo raze`, `bazel run //tensorboard/data/server:update_protos`) tensorflow#5738 attempted to update prost-types from 0.7.0 to 0.8.0. This did not work out-of-the-box for several reasons, the most important of which is that we could not successfully build and run the server with mixed versions of prost (0.7.0), prost-types (0.8.0), and prost-build (0.7.0). This PR attempts to upgrade all three of prost, prost-types, and prost-build to the same version at the same time. We also upgrade tonic, tonic-reflection, and tonic-build so that they, too, depend on the same prost version and we don't have multiple prost crates of different versions. * https://crates.io/crates/prost/0.9.0 * https://crates.io/crates/prost-types/0.9.0 * https://crates.io/crates/prost-build/0.9.0 * https://crates.io/crates/tonic/0.6.2 * https://crates.io/crates/tonic-build/0.6.2 * https://crates.io/crates/tonic-reflection/0.3.0 We choose to upgrade prost to 0.9.0 because, why not? We attempted to upgrade to 0.10.* (the latest version) but these versions of prost have a dependency on cmake, which seems to assume cmake is installed on your machine, which it typically is not within Google. There seems to be some work to further refine the dependency on cmake (tokio-rs/prost#620, tokio-rs/prost#657) so hopefully this gets resolved in a future release of prost and a future upgrade to, say, 0.11 will not have the cmake problems. The upgrade was not straightforward for me, a rust and rustboard newb. Several other edits had to be made and commands had to be run to get it to work. This was the incantation that finally worked for me: * Edit Cargo.toml to update versions of prost, prost-types, prost-build, tonic, tonic-reflection, tonic-build and to update any related raze metadata entries. * Run `cargo update -p prost -p prost-types -p prost-build -p tonic -p tonic-reflection -p tonic-build` * Inspect Cargo.lock and compare to raze metadata entries in Cargo.toml. If any of the versions changed (indexmap) then update the associated raze metadata entry. If any of the crates were removed (none were) then remove the associated raze metadata entry. Also, libc version didn't change but I updated its raze metadata anyway since its version seems to have changed in the past. * Run `cargo raze` * Run `bazel test //tensorboard/data/server:update_protos_test` (especially since this is updating proto-related libraries) and witness it FAIL and then run `bazel run //tensorboard/data/server:update_protos`. * Confirm server works with `bazel run -c opt //tensorboard/data/server -- --logdir <some logdir>`
This PR is a proposal for updating (again!) how
prostand more specificallyprost-buildgains access toprotoc.Problem
Prost does not implement a pure rust parser for protobuf file definitions. Instead, it uses
protocto parse the protobuf definitions. Version0.8and before originally bundled a copy of protoc for each platform to be used when the environment variables are no present andprotocdoesn't exist in thePATH. This (#575 and #562) provides an avenue for supply chain attacks and was removed in version0.9and replaced by bundling theprotobufsource code and usingcmaketo compile it. Whilecmakeworks (it is the recommended way to build for windows) it does have some issues (#653, #646, #650, #626, #645).Solution
This PR introduces even tighter requirements for sourcing
protoc. This change forces users to either haveprotocin their PATH or to supply it viaPROTOCenvironment variable. With this change as well the new recommendation for libraries is to commit their generated protobuf.To supplement this change, prost will take ownership of
protobuf-srcand potentially addprotobuf-src-ccvia the work in #620.In addition, a blog post will be written on the tokio.rs blog to explain how users should use prost-build with these changes.