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 cmake #620

Closed
wants to merge 22 commits into from
Closed

Remove cmake #620

wants to merge 22 commits into from

Conversation

Jake-Shadle
Copy link

@Jake-Shadle Jake-Shadle commented Apr 7, 2022

We noticed due to hyperium/tonic#965 that cmake was now a dependency for tonic via prost. We've had cmake banned for quite a while since it is terrible, so I started working on a PR to make prost-build use cc to build protoc instead. However, upon investigation I quickly realized that building and shelling out to protoc is massive overkill for prost's usage, which essentially just boils down to getting a list of the input files and all of their imports. Protoc by contrast includes support for generating multiple different languages (none of which prost uses) and plugin support (again, something that prost doesn't use), so I instead just reimplemented the small part of the command line tool that prost actually depended on into a single C function that is now linked with prost-build, and when compile_protos is called, just invokes that C function to get the output.

Pros

  • No cmake
  • Only compiles the minimum amount of C++ code that is actually needed to write the file descriptor set, so the build is significantly faster than compiling the protoc binary
  • No shelling out to a separate program and using the filesystem to read/write the output

Cons

  • This does the one thing that "vanilla" prost-build does, so usage of additional protoc arguments that people may be depending on won't do anything. I consider this fine since in those more advances use cases, you can just use an installed protoc compiler.
  • Added C++ code. I felt bad while doing it.
  • Unsafe, of course, to talk with the C code.

Other options

  • There already exists a Rust native way to read generate FileDescriptorSets at https://github.com/stepancheg/rust-protobuf/tree/master/protobuf-parse, it states that it might not be fully feature complete compared to protoc, but I think it would be fairly trivial to fix any issues there and instead use pure Rust code, allowing prost to completely remove the protobuf code altogether.

@LucioFranco
Copy link
Member

I am curious if this supports https://github.com/tokio-rs/console/blob/main/console-api/build.rs#L17 this flag?

@Jake-Shadle
Copy link
Author

@LucioFranco
Copy link
Member

I must be allergic to c++ code thanks for getting that lmaooo

@@ -0,0 +1,11 @@
[licenses]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intent here to impose Embark Studio’s deny configuration onto Prost?

Copy link
Author

Choose a reason for hiding this comment

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

This isn't "our" deny configuration, this is just a way to ensure that the dependency this PR is removing doesn't return in the future.

@olix0r
Copy link
Member

olix0r commented Apr 8, 2022

It doesn't necessarily have to be done as a part of this PR, but I wonder if we could use a feature flag to control whether any of these dependencies are incurred... We'd prefer to only ever use PROTOC_NO_VENDOR, so there's no need for us to incur the cc (or cmake) dependency. It would be fine for the feature to be enabled by default. And we'd have to follow suit and re-expose the feature from tonic-build.

@LucioFranco
Copy link
Member

@olix0r I wonder if we should provide a subset of prost-build that contains no vendoring? prost-build-cc and prost-build-lite or something like that that gives you clear control. Though I wonder how this would affect down stream users of libraries.

@olix0r
Copy link
Member

olix0r commented Apr 8, 2022

@LucioFranco yeah, I'm not sure what the most usable approach is. The decision to use vs build a protoc is really up to the application that is building a library that includes protobuf. While, the library itself needs to depend on prost-build. The library shouldn't be opinionated on whether protoc is used or not, which is why I thought the feature-flag approach might be best.

@ricnewton
Copy link
Contributor

ricnewton commented Apr 30, 2022

Is there any more progress on this PR? So for our code now since the prebuilt protoc has been removed our build time is now dominated by prost-build, especially on low spec machines like the lower end github codespaces machines (I know we can use a preinstalled protoc but we'd rather not go that route).

For a build of one of our crates which is mainly dependent on prost-build (and tonic-build), on the lowest spec codespace machine (2 cores, admittedly pretty much worst case), the time it takes to build using normal prost-build was around 11 mins 40s, and with this branch it was around 2 mins 10s.

If the changes here are too much, we also tried just disabling building the protobuf tests: ricnewton@d423ca1, this built in around 4 mins 40s so significantly better than master but not as good as this branch, I can raise a PR if that is acceptable and this branch isn't, but this branch is faster to build. Any reason we would need to build the protobuf tests?

@Jake-Shadle
Copy link
Author

I'm unable to repro the CI failure locally, but I've submitted a change that should fix it.

@Jake-Shadle Jake-Shadle force-pushed the master branch 2 times, most recently from 11fa110 to bcfaff2 Compare May 5, 2022 06:04
@Jake-Shadle
Copy link
Author

Is there some way to mark this branch/me so that CI can run when I do pushes? Due to the time difference I basically have to wait like 8 hours to see if the changes I make pass CI since apparently my local environment is different enough that I don't catch all of the same things that CI does, so the turnaround time is making this PR last waaaaaaay longer than it should.

@LucioFranco
Copy link
Member

@Jake-Shadle changed some settings should work now when you push.

@Jake-Shadle
Copy link
Author

Thanks!

@Jake-Shadle
Copy link
Author

Nice, seems to work, now that I can iterate on this and test platforms that are tedious for me, I should have all this green and passing tomorrow early.

@Jake-Shadle
Copy link
Author

It also seems that prost 0.10.2 and 0.10.3 were released without bumping the actual version in the Cargo.toml file.

@LucioFranco
Copy link
Member

@Jake-Shadle those patch releases are on the v0.10 branch, master contains some breaking changes.

@Jasperav
Copy link

@Jake-Shadle is this already merged or? My CI is failing:

running: "cmake" "/home/runner/.cargo/git/checkouts/prost-0f318a28dfb453a6/2c9d9c1/prost-build/third-party/protobuf/cmake" "-DCMAKE_INSTALL_PREFIX=/home/runner/work/xx/xx/server/target/debug/build/prost-build-744ea9bdfe2d1c67/out" "-DCMAKE_C_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_C_COMPILER=/usr/bin/cc" "-DCMAKE_CXX_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_CXX_COMPILER=/usr/bin/c++" "-DCMAKE_ASM_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_ASM_COMPILER=/usr/bin/cc" "-DCMAKE_BUILD_TYPE=Debug"

--- stderr
CMake Error: The source directory "/home/runner/.cargo/git/checkouts/prost-0f318a28dfb453a6/2c9d9c1/prost-build/third-party/protobuf/cmake" does not exist.

@Jake-Shadle
Copy link
Author

Nope, don't think so

kvcache added a commit to momentohq/momento-cli that referenced this pull request May 21, 2022
This change establishes a common pattern for interacting with the
service. Rather than copy/pasting matches, MomentoError -> CliError
mapping and json dump code around, this names those functionalities
as functions:
* interact_with_momento: Completes a momento future and maps the
  result to a Result< HappyCaseType, CliError >
* print_whatever_this_is_as_json: prints whatever you feed it as json
* drop windows lint, linux lint is enough
* also choco install protoc in a windows build near you. Relates to tokio-rs/prost#620

This change was precipitated by the change in the SDK to simplify
and coalesce client creation behind the SimpleCacheClientBuilder and
triggered by the SDK upgrade.

There are small changes to debug and error messages, but this change
should cause no functional changes.

tired of copypasta.
```
pub async fn delete_cache(cache_name: String, auth_token: String) -> Result<(), CliError> {
    debug!("deleting cache...");
    let mut momento = get_momento_client(auth_token).await?;
    match momento.delete_cache(&cache_name).await {
        Ok(_) => (),
        Err(e) => return Err(CliError { msg: e.to_string() }),
    };
    Ok(())
}
```
becomes
```
pub async fn delete_cache(cache_name: String, auth_token: String) -> Result<(), CliError> {
    let mut client = get_momento_client(auth_token).await?;
    interact_with_momento("deleting cache...", client.delete_cache(&cache_name)).await
}
```
@ricnewton
Copy link
Contributor

Hi, could I ask what more it would take to get this PR merged? It looks like it's in good shape, is there something more that needs t be done or is there an issue with the approach being taken? Build times are massive with the current prost-build.

@LucioFranco
Copy link
Member

@ricnewton still looking to merge this, I just have not had enough time to full review this yet.

@ricnewton
Copy link
Contributor

Great, thanks. If there are any issues or anything or it will take a long time I can submit the one line change that at least disables running the protocol buffer tests if that helps, that saves loads of time as well.

@benesch
Copy link

benesch commented May 24, 2022

Just dropping by from @MaterializeInc to say that this is unfortunately a nonstarter for us. Appreciate the work you've done here, @Jake-Shadle, and I'm sorry it was our supply chain concerns that kicked off this whole mess, but if prost-build decides to essentially reimplement the protobuf build system we'll have to switch back to a fork that uses a stock vendored protoc. (I'm not a prost maintainer, just an opinionated community member, so please take this as just a data point.)

To provide some more context: I've been burned repeatedly by projects that reimplement upstream build systems. Build systems, especially C++ build systems, tend to accrete important platform-specific knowledge, built up slowly over years of bug reports because there are never any tests. All that knowledge gets lost when you fork the build system. CockroachDB maintained custom build systems for its C++ dependencies for a while (e.g., RocksDB), and it was a persistent source of bugs due to compiler flags not being set correctly [0][1].

There is also a permanent maintainability to forking a build system. Someone will need to update the list of source files here for every protobuf release. Someone will need to look through upstream's autotools/CMake build system changes with every release and ensure that any feature detection that results in new compiler flags or defines gets carried through here. Downstream maintainers are generally not well positioned to do that sort of work, since they are relatively unfamiliar with the upstream codebase.

We've had cmake banned for quite a while since it is terrible...

I think this is an unfair characterization! CMake, like most tools, has its pros and cons. I've found CMake to be by far the most robust of the C/C++ build systems.

I think it would be fairly trivial to fix any issues there and instead use pure Rust code, allowing prost to completely remove the protobuf code altogether.

Last I looked into this it is far from trivial, I'm afraid. prost relies on the --include_source_info flag to protoc, which is not supported by protobuf-parse. The source info is quite detailed and seemed to require a substantial overhaul of protobuf-parse in order to put the right bookkeeping into place.


If the changes here are too much, we also tried just disabling building the protobuf tests: ricnewton@d423ca1, this built in around 4 mins 40s so significantly better than master but not as good as this branch, I can raise a PR if that is acceptable and this branch isn't, but this branch is faster to build. Any reason we would need to build the protobuf tests?

This seems like a slam dunk to me, FWIW.


Is there a reason that you can't apt install protobuf-compiler or download the precompiled release from upstream in your CI? That'd be even faster than the approach in this PR. Coupled with #648 you'd even be able to remove the cmake crate from your transitive dependency graph.

In any case, my preference would be to figure out how to modularize prost-build! Then the code here could be in a separate crate/feature for the folks who want it.

@Jake-Shadle
Copy link
Author

I get where you're coming from, but in this case protobuf is a fairly small project, especially relative to the likes of rocksdb, and prost-build only uses a small fraction of it. In addition, it's not doing really doing anything complicated nor architecture specific, which is the typical reason for special flags and the like, but I could if course be wrong thinking that this would be trivial to maintain since I have never used protoc directly so have no familiarity with their release cadence or the like.

I think this is an unfair characterization! CMake, like most tools, has its pros and cons. I've found CMake to be by far the most robust of the C/C++ build systems.

Agree to disagree I guess, CMake may the best in C/C++ land but that's really not saying much. It only brought problems for us, easier to purge it completely so that the Rust ecosystem doesn't have to rely on it.

Last I looked into this it is far from trivial, I'm afraid. prost relies on the --include_source_info flag to protoc, which is not supported by protobuf-parse. The source info is quite detailed and seemed to require a substantial overhaul of protobuf-parse in order to put the right bookkeeping into place.

That's unfortunate, IMO using pure Rust would be the ideal solution since it would mean getting rid of the C++ code altogether and making prost-build and the crates that depend on it much smaller and faster to compile. Bad assumption on my part.

Is there a reason that you can't apt install protobuf-compiler or download the precompiled release from upstream in your CI? That'd be even faster than the approach in this PR. Coupled with #648 you'd even be able to remove the cmake crate from your transitive dependency graph.

Requiring an additional installation dependency to build a single crate dependency seems pretty terrible IMO. If we had other cmake dependencies it might be acceptable, but we purged them years ago and they are never coming back, requiring cmake for CI and users who build our code would be a major step back.

This PR still supports PROTOC_NO_VENDOR I don't understand why this solution can't coexist with those like yourself who want to use the stock protoc.

@neoeinstein
Copy link
Contributor

I could if course be wrong thinking that this would be trivial to maintain since I have never used protoc directly so have no familiarity with their release cadence or the like.

This is the main thing that I fear. While a little C++ doesn't scare me, it's not something I'm keen to sign-off on to maintain in perpetuity, particularly since it would explicitly bring us out of sync with the upstream we represent we're vendoring. (I know that many of the core files in the compilation are unadulterated, but the piecing it out and creating our own compilation unit makes it more difficult to trust that our behavior will be the same as the upstream behavior.)

I have no significant opinion about cmake, but as the vendored upstream requires it for building, I find it hard to argue that we can strip it and replace it without incurring similar potential problems in ensuring that what we provide is the same as the upstream.

I would love to see an all Rust protobuf compiler that is on par with protoc as an option, presuming that was tested to ensure equivalent output with the reference protoc. Much of the protobuf ecosystem now-a-days, though is moving toward the model of using protoc compiler plugins (the reason I created protoc-gen-prost). In polyglot environments having inconsistencies between parser implementations and FileDescriptorSet output could be unexpectedly painful. buf went this route in reimplementing a conforming parser in Go, but as mentioned, protobuf-parse is not yet suitable for us to use.

Requiring an additional installation dependency to build a single crate dependency seems pretty terrible IMO.

In my case, a vendored prost would be the only dependency that brings in any native compilation dependency. In such cases, swapping cmake or cc for a protoc is a swap rather than a new dependency. In your case, you want to avoid adding a cmake dependency, and the best way to do that may be to pull in a pre-built protoc, swapping one requirement for the other.

I like the approach in #648, and I think that it only needs a few minor adjustments. With that change, dependents can avoid bringing in both cmake and the upstream source repository by default, but we'd still provide the option for building protoc from vendored source if desired.

I'd be interested in taking a look at that one-line change that could avoid running all of the protobuf tests, as that could make the vendored flow quite a bit faster without significantly diminishing the reliability of the upstream.

@Jake-Shadle
Copy link
Author

Hmm, so should I just close this then? This PR has been open coming up on 2 months now and it feels like it's getting less likely to merge given this recent feedback.

I'd of course rather not maintain our own fork and patch it in to resolve something that also seems to be problematic for other people who use prost, but this code is fairly isolated from the bulk of what prost-build does so should be easy to keep in sync with new releases, barring big changes such as the original one to vendor sources instead of using the bundled protoc.

@LucioFranco
Copy link
Member

hi! I wrote a proposal that removes cmake and can also support this work. Let me know what you think in there! Thanks for putting this together jake <3

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

None yet

7 participants