corrosion icon indicating copy to clipboard operation
corrosion copied to clipboard

Support running tests via CTest

Open AndrewGaspar opened this issue 6 years ago • 11 comments

https://cmake.org/cmake/help/v3.0/command/add_test.html

AndrewGaspar avatar Mar 11 '18 09:03 AndrewGaspar

cargo test --no-run --message-format=json | ConvertFrom-Json | ? { $_.profile.test }

AndrewGaspar avatar Mar 15 '18 04:03 AndrewGaspar

See https://github.com/rust-lang/cargo/issues/1924

AndrewGaspar avatar Mar 15 '18 04:03 AndrewGaspar

Do we really need deterministic exe file names, or can we just run cargo test as a "test driver" for CTest's add_test COMMAND? What are the use cases where this would not be enough?

ratijas avatar Aug 09 '21 13:08 ratijas

I guess that depends what's the use case for this feature. In my case, i select specific sub-crates of a workspace. And so cago test would run the test for the whole workspace. But i wouldn't want to use this feature anyway because cargo test is used directly. In the use case where corrosion is used without workspace and that the developer don't want to use cargo at all, just running cargo test might be good enough. However one can't filter and run specific tests easily then. If each test is added separately with add_test, then this is more granular to use.

ogoffart avatar Aug 09 '21 14:08 ogoffart

I'm working on this now for a much more lightweight alternative to Corrosion that we're using in the ClamAV project.

I ran into this problem today when I realized my I was seeing build failures in test infra for my PR adding a "PRECOMPILE_TESTS" option for my add_rust_test cmake command.

I think you / I need deterministic names if you want to precompile the tests with cargo test --no-run.

My Twitter plea for help on the topic, before I found this issue discussing the same thing: https://twitter.com/_micahsnyder/status/1570278750230835200

Essentially I found that cmake will re-run cargo test --no-run when you do sudo make install if CMake doesn't know both the source DEPENDS and also the OUTPUT file(s). CMake's add_custom_command() would check if the output file exists and if the source files haven't changed to prevent recompiling, but without either, it can't know. (https://cmake.org/cmake/help/latest/command/add_custom_command.html)

micahsnyder avatar Sep 15 '22 05:09 micahsnyder

If you try to run sudo cargo test --no-run it will fail on most Linux systems with something like this: rustup could not choose a version of cargo to run, because one wasn't specified explicitly, and no default is configured.

Rust is installed/configured per user, so using sudo will cause issues if you are not aware of that. Instead of using the rustup proxy cargo (which is the one in $PATH) you could specify the actual cargo instead and you would avoid this specific problem.

Essentially I found that cmake will re-run cargo test --no-run when you do sudo make install if CMake doesn't know both the source DEPENDS and also the OUTPUT file(s).

cmake rerunning the test command is not a (huge) problem per-se, because cargo also determines if it needs to recompile. I'm assuming that in you case cargo determines it has to recompile?

I'm assuming you are trying this because of the mentioned cargo test --no-run --message-format=json in the Cargo issue. I'm not sure this is such a great fit for CMake, since compiling the test and parsing the JSON would have to be at configure time.

At the moment I'm leaning towards adding one ctest target per crate target, by just defining the ctest command as cargo test --package <package> [--lib|--bin bin] -- --nocapture etc. It's not as nice as just running the test executable itself, but currently I don't see a way without knowing the test executable name.

jschwe avatar Sep 15 '22 07:09 jschwe

Rust is installed/configured per user, so using sudo will cause issues if you are not aware of that. Instead of using the rustup proxy cargo (which is the one in $PATH) you could specify the actual cargo instead and you would avoid this specific problem.

@jschwe This is interesting! I had no idea. Do you know of a programmatic way to identify the real cargo for cargo stable or cargo nightly for any specific platform (e.g. stable-x86_64-unknown-linux-gnu), rather than using $HOME/.cargo/bin/cargo +stable? If I try to do $HOME/.cargo/bin/cargo +stable under sudo, I get error: toolchain 'stable-x86_64-unknown-linux-gnu' is not installed.

In my case, cargo test was failing because of the "rustup could not choose a version of cargo to run" issue before cargo every had a chance to determine it didn't need to rebuild. If Cargo actually ran, it shouldn't do anything for sudo make install, unless the user forgot to do make first.

I'm assuming you are trying this because of the mentioned cargo test --no-run --message-format=json in the Cargo issue. I'm not sure this is such a great fit for CMake, since compiling the test and parsing the JSON would have to be at configure time.

I'm also not interested in the option presented in cargo/issues#1924 to get the file path after it is already created. And parsing JSON in CMake-script doesn't sound fun anyways ;-). Being able to predetermine the filepath would be ideal. I liked te

micahsnyder avatar Sep 15 '22 18:09 micahsnyder

Do you know of a programmatic way to identify the real cargo for cargo stable or cargo nightly for any specific platform (e.g. stable-x86_64-unknown-linux-gnu), rather than using $HOME/.cargo/bin/cargo +stable?

You can have a look at how corrosion does it in FindRust.cmake. For rustup managed toolchains the process is basically: Run rustup toolchain list --verbose, match against the line containing the toolchain you are interested in (e.g. stable) and extract the path from that line.

And parsing JSON in CMake-script doesn't sound fun anyways ;-)

CMake >= 3.19 actually does support parsing JSON and corrosion uses it to parse the cargo metadata output and automatically add targets for crates defined in a workspace.

Being able to predetermine the filepath would be ideal.

I agree, but given how old the cargo issue is I don't expect it to be possible anytime soon.

jschwe avatar Sep 15 '22 19:09 jschwe

You can have a look at how corrosion does it in FindRust.cmake. For rustup managed toolchains the process is basically: Run rustup toolchain list --verbose, match against the line containing the toolchain you are interested in (e.g. stable) and extract the path from that line.

Oooh it's under ~/.rustup. I foolishly assumed something was hidden in ~/.cargo. 👍 Excellent pointers. Thank you.

CMake >= 3.19 actually does support parsing JSON and corrosion uses it to parse the cargo metadata output and automatically add targets for crates defined in a workspace.

Neat! I missed that release note. That's really cool.

I agree, but given how old the cargo issue is I don't expect it to be possible anytime soon.

Likewise :(.

micahsnyder avatar Sep 15 '22 20:09 micahsnyder

I was able to implement your suggestions and it worked great for identifying the non-shimmed/real cargo bin directory.

However, one of our library dependencies is the jpeg-decoder crate, and that crate has a rust-toolchain file that forces the toolchain back to version 1.56.

My default toolchain expects rustc v1.63.0, which apparently means that if I invoke cargo directly, without the shim, it passes parameters that it thinks rustc v1.63.0 can handle. Sadly, that includes a --json option future-incompat which was added in rustc v1.59.0. So the build fails with:

   Compiling jpeg-decoder v0.2.6
error: unknown `--json` option `future-incompat`

I'm stumped once more. But I think we (my project) can live without pre-compiling our test executable. Compiling the test executable takes ~15.8 seconds in our CI (running the test itself only takes ~0.15 seconds).

Unless you have any other pointers for how to resolve the rustc parameter compatibility issue, I'll probably give up on test precompiling.

micahsnyder avatar Sep 15 '22 22:09 micahsnyder

However, one of our library dependencies is the jpeg-decoder crate, and that crate has a rust-toolchain file that forces the toolchain back to version 1.56.

The rust-toolchain file is only parsed if cargo/rustc is invoked via the rustup proxy. Corrosion also sets CARGO_BUILD_RUSTC=/path/to/real/rustc when invoking cargo build to force cargo to use a specific rustc. Probably you would need to set this too.

Since this topic has strayed rather far from the original issue, I'd ask you to continue this in the discussions section if you have any further questions.

jschwe avatar Sep 16 '22 06:09 jschwe