Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide an option to cargo check everything except benchmarks #10958

Open
RalfJung opened this issue Aug 9, 2022 · 21 comments · May be fixed by #14163
Open

Provide an option to cargo check everything except benchmarks #10958

RalfJung opened this issue Aug 9, 2022 · 21 comments · May be fixed by #14163
Labels
A-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request.

Comments

@RalfJung
Copy link
Member

RalfJung commented Aug 9, 2022

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 crossbeam-rs/crossbeam#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
Copy link
Member Author

RalfJung commented Aug 9, 2022

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
Copy link
Member

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
Copy link
Member Author

RalfJung commented Aug 9, 2022

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
Copy link
Member Author

RalfJung commented Aug 9, 2022

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
Copy link
Member

weihanglo commented Aug 9, 2022

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
Copy link
Member Author

RalfJung commented Aug 9, 2022

No. It won't.

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

@epage
Copy link
Contributor

epage commented Aug 9, 2022

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
Copy link
Member

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
Copy link
Contributor

ehuss commented Aug 9, 2022

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
Copy link
Member Author

RalfJung commented Aug 9, 2022

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
Copy link
Member Author

RalfJung commented Aug 9, 2022

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

@weihanglo
Copy link
Member

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
Copy link
Member Author

RalfJung commented Aug 9, 2022

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
Copy link
Member Author

RalfJung commented Aug 9, 2022

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.

@ehuss ehuss added the A-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) label Aug 13, 2022
@ryoqun
Copy link

ryoqun commented Jun 28, 2024

"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: #14163

@weihanglo
Copy link
Member

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

  • Cargo only allows a single lib in a package. It makes sense that --lib bails out when no lib exists.
  • Unlike --bin or --bench or --example, --lib doesn't require a target name. It acts more like --bins/--benches/--examples, which infers Cargo targets for users.

#10958 (comment) 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?

@weihanglo weihanglo added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. and removed S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. labels Jul 1, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Jul 1, 2024

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
Copy link

ryoqun commented Jul 2, 2024

@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: taiki-e/cargo-hack#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 (#8379, also see my comment there: #8379 (comment)).

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. rust-lang/rust#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
Copy link
Member Author

RalfJung commented Jul 2, 2024

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?

@ryoqun
Copy link

ryoqun commented Jul 3, 2024

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
Copy link
Contributor

epage commented Jul 5, 2024

RE unstable benches, see #10958 (comment)

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants