Conversation
c62bbc6 to
82f1fc4
Compare
|
Ok, I think this is ready. Sorry most of it is in one giant commit. I'd created a lot of smaller commits as I went, but ended up going back and redoing and fixing things a lot. There's a lot of interaction between different parts, s.t. using fixup commits to try to create a clean history of small commits would have been a fair bit of work. I ended up just breaking off a few small independent commits instead and putting the rest in one big one. |
|
Some ad-hoc benchmarks on my machine to validate things didn't get worse: Clean release build: 55s -> 54s Mostly ok; incremental builds get a bit slower. I think this is because the C code is now only incremental at the crate level. When any file inside a crate is touched, we always rebuild all the C files in that crate and relink (though I think the compiled rust files within the affected crate should still be reused) |
2120802 to
bcc0a36
Compare
|
In addition to now always recompiling the C code, we unfortunately lose cmake's generation of compile_commands.json, which vscode and other tools use for analysis. I've verified that it can be generated by running the whole build under bear, but haven't worked out how to get vscode to use the resulting file yet. I poked around some more to see if we could solve the main problem without this drastic overhaul. I thought maybe using Ultimately I still think this PR is the best way forward. |
This will be used by the build.rs scripts to build C and C++ code, using consistent flags and settings. Some flags specified in the cmake files are explicitly specified here, but we currently also accept some flags via env variables so that we don't need to implement all of the conditional logic yet.
4d98c1c to
993cf28
Compare
The previous state of invoking Cargo multiple times meant that dependencies could be built multiple times, potentially with different compiler flags. Our coverage build had broken as a result of a dependency getting compiled and linked multiple times with conflicting symbols (the link needed mangled symbols from both instances, but they had duplicate non-mangled symbols). Invoking Cargo only once avoids this issue - everything is compiled with the same Cargo environment (including e.g. CFLAGS, etc). Previously this wasn't possible since crates that need to link with shadow C libraries need the respective link flags passed through the Cargo environment, and there isn't a way to specify such flags for individual crates from the command-line. We solve this by wrapping the C libraries in Rust crates, and building those with Cargo as well. We use Rust build scripts to build the C and C++ code. This way Cargo can manage the dependencies and corresponding link flags since everything is a Rust crate. While not strictly necessary as part of this change, we also move binding generation into the build scripts instead of using the command line tools via cmake. As before this is disabled by default; it is now enabled via a Cargo feature *bindings*. This lets us move more logic out of cmake, partly deduplicate the bindgen and cbindgen configs by adding utility libraries (instead of having toml files for each project), and lets Cargo and our Cargo.lock file manage what version of these tools is used. While debugging, I added some additional binding-generation for some of the wrapped C crates, thinking it was needed to get the C static libraries to link properly. It turned out not to be needed for that purpose, but they'll be useful as we start to transition some of those libraries into Rust.
Generating the bindings via cmake ensures we use the same compiler flags as when compiling the rest of the code. In particular this works around an issue where newer versions of clang warn about deprecated atomic APIs unless we've specified the C standard version.
The new glib compilation test seems to have triggered a warning in cmake suggesting that we explicitly opt into either the old or new behavior
993cf28 to
fcfaf88
Compare
Accidentally dropped running any rust unit tests in shadow#2404 We were also previously not running doc tests, and I think not running tests for any crates for which we don't build a `staticlib` for C code to link to. (I'm not sure whether there were any such tests before, but there increasingly will be) For simplicity and maintainability I just invoke `cargo test` now, instead of trying to directly run the test executables. While the cargo behavior of always rebuilding (if needed) may be slightly surprising, this approach should be more robust and maintainable.
Accidentally dropped running any rust unit tests in shadow#2404 We were also previously not running doc tests, and I think not running tests for any crates for which we don't build a `staticlib` for C code to link to. (I'm not sure whether there were any such tests before, but there increasingly will be) For simplicity and maintainability I just invoke `cargo test` now, instead of trying to directly run the test executables. While the cargo behavior of always rebuilding (if needed) may be slightly surprising, this approach should be more robust and maintainable.
Accidentally dropped running any rust unit tests in shadow#2404 We were also previously not running doc tests, and I think not running tests for any crates for which we don't build a `staticlib` for C code to link to. (I'm not sure whether there were any such tests before, but there increasingly will be) For simplicity and maintainability I just invoke `cargo test` now, instead of trying to directly run the test executables. While the cargo behavior of always rebuilding (if needed) may be slightly surprising, this approach should be more robust and maintainable.
Accidentally dropped running any rust unit tests in shadow#2404 We were also previously not running doc tests, and I think not running tests for any crates for which we don't build a `staticlib` for C code to link to. (I'm not sure whether there were any such tests before, but there increasingly will be) For simplicity and maintainability I just invoke `cargo test` now, instead of trying to directly run the test executables. While the cargo behavior of always rebuilding (if needed) may be slightly surprising, this approach should be more robust and maintainable.
Accidentally dropped running any rust unit tests in shadow#2404 We were also previously not running doc tests, and I think not running tests for any crates for which we don't build a `staticlib` for C code to link to. (I'm not sure whether there were any such tests before, but there increasingly will be) For simplicity and maintainability I just invoke `cargo test` now, instead of trying to directly run the test executables. While the cargo behavior of always rebuilding (if needed) may be slightly surprising, this approach should be more robust and maintainable.
Accidentally dropped running any rust unit tests in shadow#2404 We were also previously not running doc tests, and I think not running tests for any crates for which we don't build a `staticlib` for C code to link to. (I'm not sure whether there were any such tests before, but there increasingly will be) For simplicity and maintainability I just invoke `cargo test` now, instead of trying to directly run the test executables. While the cargo behavior of always rebuilding (if needed) may be slightly surprising, this approach should be more robust and maintainable.
Accidentally dropped running any rust unit tests in #2404 We were also previously not running doc tests, and I think not running tests for any crates for which we don't build a `staticlib` for C code to link to. (I'm not sure whether there were any such tests before, but there increasingly will be) For simplicity and maintainability I just invoke `cargo test` now, instead of trying to directly run the test executables. While the cargo behavior of always rebuilding (if needed) may be slightly surprising, this approach should be more robust and maintainable.
These were inadvertently dropped in shadow#2404.
These were inadvertently dropped in shadow#2404.
I was thinking some more about the coverage breakage - I think it actually highlights a bigger problem that our hybrid cargo/cmake build ends up building some rust code multiple times. It usually isn't a big deal and works out ok in the end, but could end up causing subtle problems.
targettree. The generated artifacts have mangled names that, IIUC, incorporate the version of the compiler, the version of the dependency, what features are enabled, and extra flags passed through env variables, etc.-L, and-lshouldn't actually change the final static libraries)I think this problem all goes away if we build all of our Rust code in a single cargo invocation. In that case, everything is built with the same cargo env variables, getting rid of some of the false duplication.
For that to work, we need to move the knowledge of what linker flags to use for which crates from the cargo invocations in cmake, into cargo build scripts for each crate.
Right now that would be tricky, since cmake is responsible for building some of the C libraries. It's hard to "hard code" the right linker flags in the Rust build scripts.
We can fix that by wrapping each C library in a Rust crate. That moves the responsibility for managing and propagating linker flags from cmake to cargo.
So that's what I'm doing here. This PR currently:
shadoworlibshadow-shim.sointo a Rust crate. Currently these are just ~empty Rust libraries, with a build script describing how to compile the C files. This also has the benefit of putting us in a position where we could incrementally start moving code from C to Rust within each of these libraries.cargo buildinvocation to build all of the relevant Rust code. In addition to avoiding false duplication due to different env variables, this allows the whole Rust build to use a singletargetdirectory, which should allow it do better caching/reuse.I'm still doing some cleanup. In particular:
If we do both of those, pretty much the whole shadow build will be managed by Cargo, and we'll just be using cmake to invoke cargo and compile and run the tests.
I started thinking more about whether we could do 1 big cargo build instead of using it to build i was curious about how hard it is to have Cargo compile C code for you; it turns out it's actually pretty nice. it ended up spiraling into moving most of the shadow build from cmake to cargo