taiki-e / cargo-llvm-cov

Cargo subcommand to easily use LLVM source-based code coverage (-C instrument-coverage).
Apache License 2.0
933 stars 57 forks source link

Logic to detect rustc version is incomplete #82

Closed taiki-e closed 3 years ago

taiki-e commented 3 years ago

The current way does not handle it correctly when the version is specified in a cargo or rustup specific way (e.g., cargo +nightly-..., rustup run +nightly-...).

https://github.com/taiki-e/cargo-llvm-cov/blob/eb7c3f350bb787d6a91fcbb1ee28af4f55c678ce/src/rustc.rs#L16-L22

Currently, the following heuristic is the only one that uses the retrieved rustc information, so it does not affect the coverage results, but it may accidentally remove dependency's build artifacts.

https://github.com/taiki-e/cargo-llvm-cov/blob/eb7c3f350bb787d6a91fcbb1ee28af4f55c678ce/src/main.rs#L100

related(?): https://github.com/rust-lang/cargo/issues/2833

taiki-e commented 3 years ago

In a build script, the RUSTC environment variable can be used for this purpose, but in a subcommand, the RUSTC environment variable has a different meaning.

If we need the major-minor-patch version of the toolchain (e.g., cargo-hack's --version-range), cargo --version --verbose is fine, but if we need the exact toolchain date of nightly rustc, cargo --version --verbose is not enough.

So I think the correct action here is just to remove this heuristic. (IICU, the way introduced in #79 should work well without rustc version-based heuristics.)

taiki-e commented 3 years ago

So I think the correct action here is just to remove this heuristic. (IICU, the way introduced in #79 should work well without rustc version-based heuristics.)

In fact, it relies on information from rustc here as well, but that can be avoided.

https://github.com/taiki-e/cargo-llvm-cov/blob/eb7c3f350bb787d6a91fcbb1ee28af4f55c678ce/src/context.rs#L100

taiki-e commented 3 years ago

Oh, actually I can find the rustc binary based on the sysroot.