Remove build.rs #657

LucioFranco · 2022-05-24T22:21:26Z

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 · 2022-05-27T07:17:28Z

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 · 2022-05-27T11:41:30Z

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 · 2022-05-30T14:40:44Z

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 · 2022-06-09T14:56:06Z

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.

…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>`

neoeinstein

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.

neoeinstein · 2022-06-21T20:48:47Z

prost-build/src/lib.rs

@@ -1289,6 +1345,7 @@ mod tests {
        let _ = env_logger::try_init();
        Config::new()
            .service_generator(Box::new(ServiceTraitGenerator))
+            .out_dir(std::env::temp_dir())


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.

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

LucioFranco · 2022-06-27T18:41:54Z

tests/src/well_known_types.proto

+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";


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

Co-authored-by: Marcus Griep <[email protected]>

This reverts commit ac57645.

notmgsk · 2022-10-13T12:25:20Z

Whatever happened to that blog post? :)

LucioFranco · 2022-11-16T18:02:53Z

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>`

LucioFranco added 12 commits May 24, 2022 18:17

Proposal modular protoc support

6be9577

Add vendored binaries for CI

8d75b8f

fix protoc path

09ad2e2

better paths for protoc

a741b7a

use protoc action

16a35e9

remove locally vendored items

6e45665

fix more tests

4129a33

Merge remote-tracking branch 'origin/master' into lucio/remove-build-rs

af30d81

Remove vendored feature

43a2e2f

remove feature flag usage

c29e157

remove edition2021 for vendored test

9456594

bump msrv to 1.56

087cd98

LucioFranco mentioned this pull request May 25, 2022

chore: bump autotools to 0.2.5 MaterializeInc/rust-protobuf-native#3

Merged

more docs

274a6c9

bmd3k mentioned this pull request Jun 10, 2022

build(deps): bump prost to 0.9.0, tonic to 0.6.2, tonic-reflection to 0.3.0 tensorflow/tensorboard#5755

Merged

Merge remote-tracking branch 'origin/master' into lucio/remove-build-rs

5bb5cd2

neoeinstein approved these changes Jun 21, 2022

View reviewed changes

LucioFranco added 5 commits June 24, 2022 13:59

Remove requirement for PROTOC_INCLUDE

e89f2fd

update docs

f097c62

fix vendored ci

eb92120

Remove vendored

d5bfb37

Remove build.rs

7c61078

LucioFranco marked this pull request as ready for review June 27, 2022 16:57

LucioFranco requested review from neoeinstein and danburkert June 27, 2022 17:26

LucioFranco commented Jun 27, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into lucio/remove-build-rs

b2b0733

neoeinstein approved these changes Jun 28, 2022

View reviewed changes

prost-build/src/lib.rs Outdated Show resolved Hide resolved

Update prost-build/src/lib.rs

2f5dc52

Co-authored-by: Marcus Griep <[email protected]>

LucioFranco changed the title ~~Proposal modular protoc support~~ Remove build.rs Jun 28, 2022

LucioFranco merged commit ac57645 into master Jun 28, 2022

LucioFranco deleted the lucio/remove-build-rs branch June 28, 2022 20:47

This was referenced Jul 29, 2022

prost-build occasionally fails to compile protobuf C++ #653

Closed

[prost-build] avoid cmake dependency #646

Closed

Update tonic docs on new protoc requirements hyperium/tonic#1049

Closed

Compile fails with tonic 0.8 hyperium/tonic#1047

Closed

mdrach added a commit to sisudata/prost that referenced this pull request Aug 4, 2022

Revert "Remove build.rs (tokio-rs#657)"

1869f5d

This reverts commit ac57645.

benesch mentioned this pull request Aug 29, 2022

chore: update dependencies for prost, nix, and protobuf tikv/pprof-rs#158

Closed

jxs mentioned this pull request Sep 4, 2022

Update changelogs for prost dep update libp2p/rust-libp2p#2851

Merged

4 tasks

ldm0 mentioned this pull request Sep 10, 2022

build-std compatible profile(code coverage) support rust-lang/rust#101392

Closed

tustvold mentioned this pull request Sep 12, 2022

Vendor protoc influxdata/pbjson#62

Open

nazar-pc mentioned this pull request Sep 19, 2022

Add initial api for SDK autonomys/subspace-pulsar-sdk#2

Merged

hdevalence mentioned this pull request Sep 24, 2022

Restore bundled protoc binary #725

Closed

leviathanbeak mentioned this pull request Oct 3, 2022

update libp2p to v0.48 FuelLabs/fuel-core#661

Merged

BusyJay mentioned this pull request Nov 14, 2022

Q: Could not find protoc installation when using grpcio=0.12.0 with prost-build=0.11 tikv/grpc-rs#599

Closed

billy1624 mentioned this pull request Jul 25, 2023

update example's dependency versions SeaQL/sea-orm#1759

Merged

4 tasks

mariocynicys mentioned this pull request Mar 14, 2024

fix(tests): fix failing tests KomodoPlatform/komodo-defi-framework#2085

Merged

5 tasks

This was referenced Apr 3, 2024

Commit code generated by prost-build oll3/bita#41

Closed

Commit generated protobuf code, only regenerate on-demand oll3/bita#42

Merged

tokio-rs deleted a comment from Kh0088 Oct 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove build.rs #657

Remove build.rs #657

LucioFranco commented May 24, 2022 •

edited

Loading

LucioFranco commented May 27, 2022

benesch commented May 27, 2022

Jake-Shadle commented May 30, 2022

LucioFranco commented Jun 9, 2022

neoeinstein left a comment

neoeinstein Jun 21, 2022

LucioFranco Jun 27, 2022

LucioFranco Jun 27, 2022

notmgsk commented Oct 13, 2022

LucioFranco commented Nov 16, 2022

Remove build.rs #657

Remove build.rs #657

Conversation

LucioFranco commented May 24, 2022 • edited Loading

Problem

Solution

LucioFranco commented May 27, 2022

benesch commented May 27, 2022

Jake-Shadle commented May 30, 2022

LucioFranco commented Jun 9, 2022

neoeinstein left a comment

Choose a reason for hiding this comment

neoeinstein Jun 21, 2022

Choose a reason for hiding this comment

LucioFranco Jun 27, 2022

Choose a reason for hiding this comment

LucioFranco Jun 27, 2022

Choose a reason for hiding this comment

notmgsk commented Oct 13, 2022

LucioFranco commented Nov 16, 2022

LucioFranco commented May 24, 2022 •

edited

Loading