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

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

Open
briansmith opened this issue Oct 6, 2023 · 5 comments

Comments

@briansmith
Copy link
Owner

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 rust-lang/cargo#1581 (comment), @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:

  • Create a ring-c-test crate analogous to "cpp-linker" above.
  • Have it's Cargo.toml point to the same build.rs that the ring crate uses; i.e. they share a single build.rs.
  • In build.rs, use the target crate name to determine which static library to build.
  • Add ring-c-test as a dev-dependency of ring.
@briansmith
Copy link
Owner Author

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

NobodyXu commented Oct 7, 2023

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
Copy link
Owner Author

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
Copy link
Owner Author

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

@briansmith
Copy link
Owner Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants