briansmith / ring

Safe, fast, small crypto using Rust
Other
3.75k stars 704 forks source link

Build speedup: Don't build "test" static library in build.rs unless we're going to build/run tests #1705

Open briansmith opened 1 year ago

briansmith commented 1 year ago

Right now we build two static libraries in build.rs: one that ring actually needs, and one that only ring's tests need. And that second static library is really just for "crypto/constant_time_test.c" and probably soon "crypto/compiler_test.c" (to be adapted from BoringSSL's "compiler_test.cc").

Ideally we'd only build these source files and run the archiver to create the library when we're actually building tests. For most users of ring, they will not be building ring's tests, so they should be able to skip building that library completely.

In https://github.com/rust-lang/cargo/issues/1581#issuecomment-1216924878, @dcsommer had a very good hack for creating what is effectively a "test build script" that achieves this effect, which I am quoting here for archival purposes:

Separate crate cpp-linker that has

  • An empty lib.rs
  • build.rs links the C++ code using println!("cargo:rustc-link-lib=static:-bundle={}", lib_name); (I also use the [+-]whole-archive linking modifier, but that's not relevant to this). -bundle was stabilized in 1.63 and allows an rlib crate to defer the linking of C++ symbols until final test binary linking. Library rlib crate mylib
  • Has [dev-dependency] on cpp-linker
  • Include extern crate cpp_linker; inside of mod tests to bring in the C symbols

To use that idea:

briansmith commented 1 year ago

OTOH, we are also trying to remove dev-dependencies from the ring crate so that people can do cargo test -p ring to run the tests...so maybe we need a different solution.

NobodyXu commented 1 year ago

Maybe you can use Path::new(".git").exists() to detect whether they are in development or are using a tarball from https://crates.io ?

This solution isn't perfect since user could still use patch."craets.io" and use a specific git rev, but it can at least help in cases where ring is downloaded from https://crates.io

briansmith commented 1 year ago

For what we're doing right now, we actually wouldn't need the ring-c-test crate to be a dev-dependency of the ring crate; as long as they use the same build.rs they will compile crypto/internal.h the same way. It could just be another crate in the workspace. I will be doing similar for the benchmarks.

briansmith commented 1 year ago

...and, since there would be no dependency relationship between the two crates, they should build perfectly in parallel.

briansmith commented 1 year ago

PR #1710 creates a workspace. Presumably ring-c-test would become a default member of the workspace (unlike ring-bench).