Clam 1619 cmake rust improvements #694
Merged
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
Update CMakeRust.cmake from github.com/micahsnyder/cmake-rust-demo Add the ability to build Rust-based executables from CMake.
Precompile the test executable during the main build, after libclamav has built. This will make it so the compile time does not count against the test time. It also, unfortunately, make the main build take way longer. Removed 3 duplicate variables from the test ENVIRONMENT variable.
shutton
suggested changes
Sep 7, 2022
libclamav_rust/build.rs
Outdated
| let bindings = builder.generate().unwrap(); | ||
| builder | ||
| .generate() | ||
| .expect("Unable to generate Rust bindings for C code") |
There was a problem hiding this comment.
As discussed, dropping the "Unable to" and phrasing it "Generating..." will make for better wording if this panics.
| builder | ||
| .generate() | ||
| .expect("Unable to generate Rust bindings for C code") | ||
| .write_to_file(BINDGEN_OUTPUT_FILE) |
There was a problem hiding this comment.
Ditto. Suggest: `Writing Rust bindings to output file". Bonus points given for naming the file.
There was a problem hiding this comment.
No bonus points for me today. I just need to merge this and move on to more pressing tasks.
Right now, each Rust-based target added in CMake is being built in its own directory under the build path. This causes Rust to build each module from scratch, meaning any dependencies they have in common are built twice. The solution in this commit is to specify the top level build directory as the target directory for every Rust build and test. Note this also changes where the `clamav_rust.h` file is generated. It is now also placed in the top-level build directory, instead of under the `build/libclamav_rust` directory. That's a bit of a side-effect, and could be rectified if needed, but it appears to have no ill-effects. It's the same location that we drop the clamav-types.h file, so I think it's fine, for now. Note that `clamav_rust.h` is not a public header, it's just so libclamav functions can call into libclamav_rust functions.
micahsnyder
force-pushed
the
CLAM-1619-cmake-rust-improvements
branch
from
18723b9
to
d36d764
Compare
|
This build is failing on windows because the logic I use to provide the environment variables for the libclamav_rust executable (see 'libclamav_rust_test' CMake target) is including the PATH, and the path has more than one item on Windows that is space separated... That doesn't work. It makes the I'll work on a solution. |
Precompiling the test executable causes `sudo make install` to fail. The issue is that because we don't know the OUTPUT file path for the test executable, CMake can't know if it doesn't need to rerun that command, so it will run it with any call to `make`. The problem is that cargo is will fail to run with `sudo` on many systems. This is generally okay, because we just fixed the issue where we had been compiling the dependencies for each module. Now that we only build the dependencies once, building the test executable is actually very fast. So compiling it during the test isn't so bad.
micahsnyder
force-pushed
the
CLAM-1619-cmake-rust-improvements
branch
from
0580c88
to
13d86ae
Compare
shutton
approved these changes
Sep 16, 2022
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.
This PR solves two issues:
This significantly reduces test time in this project, and the equivalent changes in my CMake-Rust Demo project found a more significant improvement in build time because it has more than one Rust target, with some common dependencies: micahsnyder/cmake-rust-demo#9