Standardize on cargo-c for building rustls-ffi, CMake for building test programs #493
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Commits the result of running gersemi[0] on our CMake files: ``` gersemi -i CMakeLists.txt tests/CMakeLists.txt ``` Based on a short survey it appears Gersemi is the most active/well supported CMake formatter. The formatting might not be quite as nice as doing it by hand, but it is consistent and enforceable in CI. [0]: https://github.com/blankspruce/gersemi
Previously we used the `set` cmake command[0] to populate cache string variables for two boolean options: `CERT_COMPRESSION` and `FIPS`. The `option` cmake command[1] is exactly for this purpose: > Provide a boolean option that the user can optionally select. Use it instead so we get better handling of unknown values for free, removing the more brittle str comparison to "true". [0]: https://cmake.org/cmake/help/latest/command/set.html#command:set [1]: https://cmake.org/cmake/help/latest/command/option.html#option
The top level CMakeLists.txt was getting pretty messy. Let's make it easier to digest by splitting it up into: * `cmake/options.cmake` - for build settings related to options. * `cmake/rust.cmake` - for the external project that builds the rustls-ffi Rust library.
cpu
force-pushed
the
cpu-cmake-it-go
branch
2 times, most recently
from
88c4689 to
25d055d
Compare
|
@ctz I'm curious if you have any strong feelings about this before I try and get Jsha's eyes on the proposal. WDYT? |
ctz
reviewed
Nov 27, 2024
cpu
commented
Nov 27, 2024
|
@divergentdave is there any chance you'd be interested in taking a look at this diff as the author of the original |
|
Sure! I can take a look |
I was able to reproduce the issue I'm seeing in a small standalone repo and asked the cargo-c folks to weigh in. There's a strong chance I'm putting square pegs in round holes, win32 is greek to me. |
cpu
force-pushed
the
cpu-cmake-it-go
branch
2 times, most recently
from
f8eb7ea to
68acdfc
Compare
All fixed w/ lu-zero/cargo-c#428. We now have working CI for building the example binaries using static and dynamic linking for all of Linux, MacOS and Windows 🎉 |
divergentdave
approved these changes
Dec 4, 2024
|
|
||
| add_custom_target( | ||
| cbindgen | ||
| # TODO(@cpu): I suspect this won't work on Windows :P |
There was a problem hiding this comment.
FWIW, this does indeed fail for me on Windows, but thus far it's just due to missing dependencies. cbindgen invokes cargo rustc -Zunpretty=expanded, and that fails when trying to build aws-lc-fips-sys, since I don't have nasm, Go, and Ninja installed.
There was a problem hiding this comment.
Good data point, thank you. I wasn't sure the stdout redirect would work on Windows but it sounds like your attempt to repro might have bailed before that point 🤔
There was a problem hiding this comment.
I think stdout redirection with > should be safe, it's supported by cmd.exe.
This is largely in preparation for supporting MacOS and Linux CMake builds where the default for the C/C++ tooling is annoying to deal with. It is a no-op for Windows/IDE based CMake builds that use a "Multi config" setup. See the diff's comment for more detail on why this is trickier than it has any right to be.
The client and server test binary targets are almost the exact same. To avoid a lot of duplication we can use a function to add the targets and invoke it twice, once for `client` and once for `server`. This also requires telling Gersemi to not warn about unknown commands as any functions defined by ourselves will be unknown to the formatter for some complicated CMake reasons[0] I don't pretend to understand. Along the way, change to using the `target_sources` command[1] to specify the target's source code separate from `add_executable`. I found this easier to work with when later trying to format the C source code from a cmake target. [0]: https://github.com/blankspruce/gersemi?tab=readme-ov-file#lets-make-a-deal [1]: https://cmake.org/cmake/help/latest/command/target_sources.html#target-sources
The `PUBLIC` scope for the `target_include_directories`[0] command is to populate `INTERFACE_INCLUDE_DIRECTORIES` and is for libraries to list their include directory requirements. We don't intend for the client/server targets to be used as libraries so use `PRIVATE` scope. [0]: https://cmake.org/cmake/help/latest/command/target_include_directories.html
Previously we used the `add_compile_options`[0] command to conditionally add the sanitizer settings to have debug builds use ASAN. This commit lifts that configuration to the target level in the `test_binary` function using `target_compile_options`[1]. Soon we will want to apply the sanitizer options differently based on whether we're building for Windows or UNIX. It also seems like a generally useful practice to try and scope as much config to specific targets as possible. [0]: https://cmake.org/cmake/help/latest/command/add_compile_options.html [1]: https://cmake.org/cmake/help/latest/command/target_compile_options.html
This is relatively straightforward compared to Windows. Some care is required to set the right linker options for MacOS, and for cert compression we want `-lm` when the option is enabled. Compiler options are matched to what `Makefile` was setting `CFLAGS` to. The ASAN settings are similarly handled like in `Makefile`.
When building a debug release w/ clang we can enable a few more sanitizers for the test code: * undefined * unsigned-integer-overflow * local-bounds * implicit-conversion This is the "undefined" check group plus a few of the ones that it doesn't include by default. See https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html for more information. You can test the build with the following on a UNIX system: ``` CC=clang CXX=clang cmake -B build -S . -DCMAKE_BUILD_TYPE=Debug cmake --build build ```
We have several `.PHONY` targets in the GNU `Makefile` that handle code formatting. This commit extends the CMake build system to be able to do the same. We add `-fix` and `-check` targets for: * `rust-format-fix` and `rust-format-check` for running/checking `cargo fmt` for rust code. * `cmake-format-fix` and `cmake-format-check` for running/checking Gersemi for CMake files. * `c-format-fix` and `c-format-check` for running/checking `clang-format` on the test .c/.h files. * `format-fix` and `format-check` for handling all of the above. You can use these from a UNIX system like: ``` cmake -B build -S . cmake --build build --target format-fix cmake --build build --target help ```
This ports the `src/rustls.h` GNU `Makefile` target to the CMake build system. For now I'm assuming the development related targets for formatting, generating headers, etc, won't be run on Windows (since that's the status quo). In the future it would be nicer for these targets to be platform independent.
This ports the `.PHONY` targets for running a platform verifier connection test and the integration tests. Both are ignored by default during normal `cargo test` invocations because they depend on the client/server binaries being built (and may make network requests in the case of the connect-test target). Run these like usual on a UNIX system: ``` cmake -S . -B build cmake --build build --target connect-test cmake --build build --target integration-test ```
Use one job for Windows with a matrix of inputs for: * crypto (aws-lc-rs or ring) * config (Debug or Release) * cert compression (on or off) We don't extensively test all possibilities to save CI time. Notably we only test cert compression w/ aws-lc-rs and in Release mode. The integration test handling is now simplified by the addition of the `integration-test` target.
It was getting cramped in here.
* Avoid the GNU Makefile for building C test code, performing helper tasks like running formatters. Instead, use `cmake`. * Collapse the separate cert compression job into the Build+Test matrix.
If we can't run the client/server binary, share more about why.
|
@divergentdave Thank you for the thorough review. I really appreciate it. |
@ctz Would you be willing to turn this comment into a +1 review? I emailed Jsha Nov 27th to ask for his review and will give him a bit more time before I merge but in the meantime it'd be helpful to have your approval recorded. |
ctz
approved these changes
Dec 9, 2024
jsha
approved these changes
Dec 12, 2024
* Emphasize that a C compiler and build tools (cmake, etc) aren't needed to build rustls-ffi: just the example client/server binaries. * Prefer describing building the rustls-ffi library (static and dynamic) using `cargo capi`. It's `install` action takes over for the `Makefile`'s manually curated version (and also works on Windows). * Prefer using `cmake` to describe how to build the client/server tests. It's easier to use once you're familiar with it, and it's a cross-platform solution that works on Windows, MacOS and Linux. * Some additional coverage for optional features (cert compression, FIPS).
This is _much_ easier, provides a real `install` command, and can provide dynamic linking support that integrates well with `pkg-config`.
This commit replaces the Makefile.pkg-config script that was testing using our client/server with a dynamically linked librustls on macOS and Linux. Now, the CMake based build script can be used for this purpose by activating `-DDYN_LINK=on` when configuring the cmake build. It supports MacOS, Linux and Windows.
Helpful for Visual Studio usage.
This requires an upstream bug-fix from cargo-c. To support dynamic linking on windows ensure you are using cargo-c 0.10.7+. With this in place, we can run integration tests against client/server binaries that dynamically link librustls.dll \o/
As pointed out by Divergentdave we can avoid using `--no-warn-about-unknown-commands` by adding a simple decl file with our two custom functions.
Rather than handle the difference in output binaries per-platform on our own, rely on the target artifact generator expression[0] cmake already provides. [0]: https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html#target-artifacts
Admin merging to bypass the stale rules (which I will fix afterwards). |
Done. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I recommend reviewing this branch commit-by-commit. There are two main goals:
rustls-ffilibrary to use as a consumer.client/serverexamples, and for driving dev. tools (formatting/running integration tests/etc).Standardizing on cargo-c
We introduced support for building a static or dynamic
rustls-ffilibrary using cargo-c back in #274. Since then it's been used successfully by several of our downstream packagers (archlinux, homebrew, gentoo, nixpkgs, etc). We're also seeing downstream projects like curl working around missing .pc files for theMakefilebuild.I propose we standardize on
cargo cas the preferred way to build and install the library both in our docs and CI. It does an excellent job of providing a convenient way to both build and install the library for both static and dynamic linking, with pkg-config consideration, on all of our supported OSes, without needing to get into the nitty gritty ofrustc. We benefit from being able to focus onrustls-ffiinstead of the minutiae of building native libraries in a variety of tricky contexts & downstream distributors and users can benefit from a consistent approach that matches other Rust software usingcargo-c.Specifically,
cargo-c:--crate-type=cdylibor to remember to install the header file manually.Makefileinstallation process that didn't support Windows.Makefileonly supported the latter.cargo, or downloading pre-built binaries as we do in CI).Notably standardizing on
cargo cdoes not require it. Adventurous users are free to find the right incantations to build the library with standardrusttooling. The only caveat is that they're on their own for determining the right arguments for their platform/linking type, installing the header file, generating .pc files, etc etc, but the library will build withoutcargo c.Standardizing on CMake
Previously the GNU
Makefilehad three roles for Linux/MacOS systems:rustls-ffi.astatic library, and installing it and therustls.hheader file into the correct location (proposed to be replaced bycargo-c, per above).clientandserverexample C applications.In parallel we maintained a CMake based system in order to provide a better integration story for Windows and MSVC toolchains, but did not offer the development task helpers.
In addition to the duplication of work, maintaining the
Makefilefeels like it's starting to hit a complexity wall. We're shimming in new feature flags as environment variables but there's no good way for a consumer to list the existing options or intuit their values without reading the makefile code. It's also difficult to make complex logical expressions; instead you must nest each conditional. In practice I've also found IDE integration poor: CLion does very well with CMake/Rust projects and not so well with GNU Makefile projects. We could reach for something like GNU autotools, but this is the path to madness and we're still stuck with CMake for Windows.I propose we bite the bullet and standardize on
CMake. It's a mildly unpleasant build system with a bit of a learning curve but we're stuck with it for Windows and so we might as well make the most of its cross-platform abilities. The extra power is beneficial for expressing our build options and more complex logic like ("FIPS and dynamic linking is ok, but only on UNIX"). It's also helpful having a standardized way to list existing options/descriptions for a better user experience.Notably
cmakeis only required for building the example binaries, where you also need a C compiler. In this context it feels very reasonable to requestcmakeas a prerequisite. Users only interested in therustls-ffilibrary only requirecargo, andcargo-c.Resolves #390 where there's some additional context/discussion.