crate-ci / azure-pipelines

Easy continuous integration for Rust projects with Azure Pipelines
MIT License
88 stars 24 forks source link

Cargo check should use `--all-targets` #24

Closed ghost closed 5 years ago

ghost commented 5 years ago

Currently the cargo check script uses --bins --examples --tests https://github.com/crate-ci/azure-pipelines/blob/0471851e53018eade2a78543e080de9345c67a26/azure/cargo-check.yml#L16 These options are not complete. Benches are missing for example.

--all-targets is the better option (https://doc.rust-lang.org/cargo/commands/cargo-check.html).

jonhoo commented 5 years ago

It does not use --all-targets because it implies --benches, which will generally not compile on stable/beta since #[bench] is nightly-only :)

I would love to be able to switch to --all-targets, though I think that issue would have to be solved first.

epage commented 5 years ago

iirc benches is stable, the harness is not (allowing you to use criterion).

However, we are then split on whether to focus on custom harness users or the built-in harness users.

jonhoo commented 5 years ago

Sorry, what I mean is that if a binary or library has benchmarks that use the built-in harness, then running cargo check --all-targets will fail when using stable. This would probably be very unexpected from the author's perspective. I'm not sure what the best way around this is though, because as you say we would like to check things like criterion benchmarks. May be worth filing a cargo issue for this?

epage commented 5 years ago

I was just clarifying the situation, that for some people it can work on stable. As long as we have something step that is ensuring benches compile, I have no problem directly calling out each type of target rather than using --all-targets.

jonhoo commented 5 years ago

There's this: https://github.com/crate-ci/azure-pipelines/blob/0471851e53018eade2a78543e080de9345c67a26/azure/cargo-check.yml#L23

Though it's not currently settable from stages, and not called out in the parameters list at the top of the file, so that should probably be fixed.

ghost commented 5 years ago

I wasn't aware of the issues with nightly. I just remembered that the option --all-targets exists and was wondering why it was not used. Thanks for the discussion, I learnt stuff.

jonhoo commented 5 years ago

I opened #26 to track the benches issue.