rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.55k stars 2.38k forks source link

Provide an option to `cargo check` everything except benchmarks #10958

Open RalfJung opened 2 years ago

RalfJung commented 2 years ago

IDE plugins like RA like to invoke cargo check with --all-targets to ensure that tests and examples are checked as well, not just the main crate sources. However, this often fails on stable since it will also check benchmarks, which essentially always need nightly features (Cc https://github.com/crossbeam-rs/crossbeam/issues/890).

So I thought I could work around this by telling RA to run "cargo" "check" "--lib" "--bins" "--tests" "--examples" "--workspace" "--message-format=json" instead -- however, this command sadly will fail when there is no lib target. Is there a way to say "please check everything except for the benchmarks"?

The documentation claims that --all-targets is equivalent to specifying --lib --bins --tests --benches --examples, but that is wrong: --lib --bins --tests --benches --examples will fail when there is no lib target, but --all-targets will still work.

RalfJung commented 2 years ago

E.g. there could be a --libs flag that selects all the libs targets that exist -- it's going to be either 0 or 1, but that still leaves two options, and cargo currently doesn't have a uniform way to deal with this situation.

I'd rather not have to manually configure RA for each and every package and then remember to change the configuration when a libs crate is added to a package.

weihanglo commented 2 years ago

Sorry for the subtlety between these flags. --tests implies lib target is included as well, so I believe cargo check --tests is effectively the same as cargo check --lib --tests except checking the existence of lib target. Does cargo check --bins --tests --examples work for you?

RalfJung commented 2 years ago

What if my Cargo.toml contains

[lib]
test = false

will cargo check --tests then still also check the lib? And is that a stable guarantee I can rely on? The documentation doesn't say anything like this (as far as I can see).

RalfJung commented 2 years ago

FWIW for now I am simply sticking to nightly rustc for crates where cargo check --all-targets does not work on stable. But at least the documentation issue in --all-targets should still be tracked.

weihanglo commented 2 years ago

What if my Cargo.toml contains

[lib]
test = false

will cargo check --tests then still also check the lib?

No. It won't. See https://doc.rust-lang.org/stable/cargo/commands/cargo-test.html#target-selection (or you can check the doc with the old new friend cargo help check 😆). cargo check --tests will skip those flagged as test = false.

And is that a stable guarantee I can rely on? The documentation doesn't say anything like this (as far as I can see).

I believe it's fairly stable. At least it's a documented behaviour and changes against that should be under meticulous reviews.

RalfJung commented 2 years ago

No. It won't.

That doesn't achieve my goal then, of checking everything except for the benchmarks.

epage commented 2 years ago

Personally, I would lean towards the following route until benches are stable

I would accept a patch to use criterion in benchmarks, but there is no motivation on our part to address this since this situation is so common.

weihanglo commented 2 years ago

Oh now I see. You want to run all Cargo targets including those disabled by default. I would suggest the following command:

cargo check --test '*' --bin '*' --example '*'

And I totally agree using criterion is a viable choice.

ehuss commented 2 years ago

Can you say more about why RA wants to check all targets? Like, in what situation does it do that? Would it be possible for it to run checks for a narrower subset?

RalfJung commented 2 years ago

Well usually I'd like my IDE to tell me when I do some change that breaks examples/tests/benchmarks. So it makes a lot of sense to check all targets I think?

In Miri I used to accidentally break benchmarks fairly regularly and only got caught on CI when I created a PR, which was rather annoying.

RalfJung commented 2 years ago

Note that ./x.py check library in rustc also checks tests and all the other targets, for the same reason.

weihanglo commented 2 years ago

will cargo check --tests then still also check the lib?

Oh wait. It should check the lib, but won't test it. Since test targets depends on the lib target, it will always get the lib target checked. Sorry for misreading your requirements.

RalfJung commented 2 years ago

cargo check --test '*' --bin '*' --example '*'

But that will still skip lib crates if they have no tests, I would assume? In the extreme case, this crate might have only a lib crate and no tests/bins/examples at all. Then I would assume cargo check --test '*' --bin '*' --example '*' does nothing?

I should probably just test this locally... ... yes my theory is confirmed:

$ cargo check --tests
warning: Target filter `tests` specified, but no targets matched. This is a no-op
    Finished dev [unoptimized + debuginfo] target(s) in 0.21s

also FWIW

$ cargo check --test '*' --bin '*' --example '*'
error: no bin target matches pattern `*`

So this won't work at all for lib-only crates.

RalfJung commented 2 years ago

You want to run all Cargo targets including those disabled by default.

Yes. I want to do exactly what cargo check --all-targets does except I want to exclude benchmarks. And I want it to be the same command for all crates so that I can just configure RA globally to use that command and have it work everywhere.

ryoqun commented 2 months ago

"cargo" "check" "--lib" "--bins" "--tests" "--examples" "--workspace" "--message-format=json" instead -- however, this command sadly will fail when there is no lib target.

hi, I stumbled on this problem as well and created a pr to fix this: https://github.com/rust-lang/cargo/pull/14163

weihanglo commented 2 months ago

14163 proposed a solution that demote to a warning if lib is missing when --lib is provided. I have mixed feeling with this.

https://github.com/rust-lang/cargo/issues/10958#issuecomment-1209510703 wants a way to build all targets but benchmarks. It seems like #14163 achieves that but want to hear back from @RalfJung.

Also, @ryoqun could you share your use case so we can know the problem space more?

RalfJung commented 2 months ago

Not sure what you want to hear back about from me? The key goal hasn't changed, and is described here: have a command that builds all parts of a crate that one can expect to build successfully on a stable toolchain.

Well the ideal version would actually be context-sensitive: build benchmarks on a nightly toolchain but not on stable. Basically, "cargo please build everything that should actually work on this toolchain" -- it almost never makes sense to even try building benchmarks on a stable toolchain. So maybe --all-targets should just exclude benchmarks on stable, but include them on nightly?

ryoqun commented 2 months ago

@weihanglo thanks for your quick reply.

Also, @ryoqun could you share your use case so we can know the problem space more?

The context is quite involved...

Firstly, I want to run cargo check individually over many workspace members, for the lib target and all of bin targets uniformly (i.e. with same cli args). The members could have only the lib or only the bins or both.

To recap, cargo check --lib --bins fails when encountered with only-bins member crates. And I can't use --all-targets --workspace on the root package described below...

To begin with, individual cargo check invocations is needed to avoid the unwanted effect of the feature unification. For that end, I'm using cargo hack actually (as I referenced back to this issue, there's relevant issue in the cargo-hack: https://github.com/taiki-e/cargo-hack/issues/189, cc: @taiki-e ).

The unwanted effect is referring to the well-known possibility of false-negatives of compilation errors of feature-gated code (again, one of primary motivations of cargo-hack): Even if subcrate B code needs to depend on Supercrate A with feature F enabled, it won't cause complication failures if some other random crate R happens to activate the feature F of the supercate A. That's why --workspace can't be used.

That's especially problematic for our special feature F, actually called dev-context-only-utils. In short, it's an ad-hoc approximation solution for (https://github.com/rust-lang/cargo/issues/8379, also see my comment there: https://github.com/rust-lang/cargo/issues/8379#issuecomment-1584575965).

The dev-context-only-utils should ever be enabled only for --tests. In other words, I want to make sure all of --lib and --bins targets will compile successfully, without accidentally depending on code gated under dev-context-only-utils. For that end, I need to suppress the feature unification across --lib --bins <=> --tests. Thus, that's why --all-targets (= contains --tests) can't be used, which activates dev-dependencies, in which dev-context-only-utils is enabled usually. fyi, the actual usage of dev-context-only-utils can be viewed at https://github.com/anza-xyz/agave.

Cargo only allows a single lib in a package. It makes sense that --lib bails out when no lib exists.

Hmm, can --lib (0-or-1 possibility) justify the inconsistency with --bins (0-or-N possibilities), regarding bail or warn? In other words, does it make sense that --bins warns instead of bailing out for a package with no bins, only because cargo allows multiple bins in a package by design? Why is there this warn-or-bail behavior difference? Maybe, I'm missing something?

Well the ideal version would actually be context-sensitive: build benchmarks on a nightly toolchain but not on stable. Basically, "cargo please build everything that should actually work on this toolchain" -- it almost never makes sense to even try building benchmarks on a stable toolchain. So maybe --all-targets should just exclude benchmarks on stable, but include them on nightly?

I think it's possible to write bench targets without relying on the unstable bench feature (i.e. https://github.com/rust-lang/rust/issues/66287), meaning there's a possibility of stable-compatible bench targets. So, I wonder this context-sensitivity could work? At least, it needs a bit smarter?

RalfJung commented 2 months ago

I think it's possible to write bench targets without relying on the unstable bench feature (i.e. https://github.com/rust-lang/rust/issues/66287),

That issue is about the unstable feature being unstable? What does it say about using it on stable?

ryoqun commented 2 months ago

I think it's possible to write bench targets without relying on the unstable bench feature (i.e. rust-lang/rust#66287),

That issue is about the unstable feature being unstable? What does it say about using it on stable?

I meant that it's possible to create --bench targets, which do compile with stable toolchains. So, it's not good for cargo to implicitly exclude --benchs from --all-targets only because it's running with the stable toolchain.

That's said. It's rather rare use case. Usually, --benches targets can only be built with a nightly toolchain as you said, because they often use that unstable feature.

epage commented 2 months ago

RE unstable benches, see https://github.com/rust-lang/cargo/issues/10958#issuecomment-1209427196

@ryoqun it sounds like you are wanting to run non-dev targets across each workspace member to avoid unification. For this, you want cargo check --bins --lib to always work, whether there is a lib or not. Why doesn't cargo check work for your use case? My naive assumption is that it does what you want out of cargo check --bins --lib.

ryoqun commented 2 months ago

Why doesn't cargo check work for your use case? My naive assumption is that it does what you want out of cargo check --bins --lib.

@epage Thanks for replying! cargo check won't work because I'm planning to run those target flags separately. I mean, run cargo check --bins and cargo check --libs in parallel to cut ci time (EDIT: using multiple machines).

Sorry to rehash this, but speaking of the behavior difference of cargo check and cargo check --bins --lib, seems this subtlety is well recognized, but is this really desired?

https://github.com/rust-lang/cargo/issues/10958#issuecomment-1209332109 cc: @weihanglo :

I believe cargo check --tests is effectively the same as cargo check --lib --tests except checking the existence of lib target.

As far as I read the docs, there's no explicit mention about this additional check only if --lib is supplied. The doc reads like cargo check is equivalent to cargo check --bins --lib indeed:

https://doc.rust-lang.org/stable/cargo/commands/cargo-check.html#target-selection

cargo check will check all binary and library targets of the selected packages. ... --lib Check the package’s library. ... --bins Check all binary targets.

For that consistency perspective as well, I'd still argue to demote the process-terminating error into just a warning one...

RalfJung commented 2 months ago

cargo check already does everything maximally in parallel, so it seems counterproductive to invoke cargo multiple times instead.

ryoqun commented 2 months ago

cargo check already does everything maximally in parallel, so it seems counterproductive to invoke cargo multiple times instead.

oh, thanks for quick reply. I meant using different machines. It's easy to run parallel jobs using multiple machines with CI providers, but it's not easy to request bigger machine with 2x cores...

epage commented 2 months ago

Depending on your project, there would be a shared overhead between --bins and --lib targets and only the final --bins and --libs would make a difference in parallel. If this is check, most of the "heavy" overhead, like linking, shouldn't be there. You also are losing out on the benefits as a project winds down and cores go unused.

If you have found this is faster (and either uses less compute minutes or you are fine with it using more which is more likely) and you are fine with taking up more of your max concurrent jobs for the duration of the shared overhead, this sounds fairly limited in who all would benefit from this and seems something not to design towards.