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

Remove build.rs #657

Merged
merged 21 commits into from
Jun 28, 2022
Merged

Remove build.rs #657

merged 21 commits into from
Jun 28, 2022

Conversation

LucioFranco
Copy link
Member

@LucioFranco LucioFranco commented May 24, 2022

This PR is a proposal for updating (again!) how prost and more specifically prost-build gains access to protoc.

Problem

Prost does not implement a pure rust parser for protobuf file definitions. Instead, it uses protoc to parse the protobuf definitions. Version 0.8 and before originally bundled a copy of protoc for each platform to be used when the environment variables are no present and protoc doesn't exist in the PATH. This (#575 and #562) provides an avenue for supply chain attacks and was removed in version 0.9 and replaced by bundling the protobuf source code and using cmake to compile it. While cmake works (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 have protoc in their PATH or to supply it via PROTOC environment 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-src and potentially add protobuf-src-cc via 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.

@LucioFranco
Copy link
Member Author

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.

@benesch
Copy link

benesch commented May 27, 2022

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?)

@Jake-Shadle
Copy link

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 PATH. If it could become standard to include the generated sources in library crates that would avoid this situation and the need to run protobuf generation at build time at all, but that seems like a big change, and could I imagine be problematic for some scenarios that I'm not thinking of? Either way this seems like this will be another big ecosystem change...?

@LucioFranco
Copy link
Member Author

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?

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.

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 PATH.

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.

bmd3k added a commit to tensorflow/tensorboard that referenced this pull request Jun 13, 2022
…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>`
Copy link
Contributor

@neoeinstein neoeinstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we can fix this in another PR but prob a good idea.

@LucioFranco LucioFranco marked this pull request as ready for review June 27, 2022 16:57
Comment on lines +4 to +14
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";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

prost-build/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Marcus Griep <marcus@griep.us>
@notmgsk
Copy link

notmgsk commented Oct 13, 2022

Whatever happened to that blog post? :)

@LucioFranco
Copy link
Member Author

Whatever happened to that blog post? :)

Sorry, I ran out of time unfortunately :(

yatbear pushed a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
…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>`
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
…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>`
@tokio-rs tokio-rs deleted a comment from Kh0088 Oct 2, 2024
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

Successfully merging this pull request may close these issues.

5 participants