Cargo: Add cargo build --examples to build all examples

[jsanders]

Or maybe it could be called --all_examples for consistency with --all_features.

It would be convenient to build all examples and then run them with target/debug/examples/NAME instead of needing to list the examples directory and find specific names to pass to cargo run --example.

2548 would definitely help here, but I think both features would be nice to have.

Edit: Apologies if this has been discussed and rejected - I couldn't find an explicit "we don't want this" anywhere.

[alexcrichton]

Sounds reasonable to me, we could even extend to:

cargo build --bins
cargo build --tests
cargo test --tests
cargo test --bins

(etc)

[BenWiederhake]

I'd like to see this implemented.

My workaround: move them to tests/ (or use symlinks).

[alexcrichton]

@BenWiederhake would you be up for implementing a PR to do this? I wouldn't mind helping out with a PR if you'd like

[BenWiederhake]

At first glance, it looks like this would either need to (I) enumerate things on its own during cargo::bin::{bench, build, run, test}, or (II) changing cargo::cargo::ops::cargo_compile::CompileFilter to refine the granularity, so that a CompileFilter instance can express things like "only bin x, but all examples, and only test y, but all benches".

Am I even on the right track? I guess you prefer the (II) way?

I'm MikeMountain on irc.mozilla.org, in case a chat is more productive.

[alexcrichton]

Ah yeah I think this change will boil down to:

• Add a new variant to CompileFilter
• Get code compiling again (you'll find the cases that need updating)
• Add tests

and that should be it!

[BenWiederhake]

A new variant to CompileFilter? Hmm, I imagined something more like this:

// Current

pub enum CompileFilter<'a> {
    Everything,
    Only {
        lib: bool,
        bins: &'a [String],
        examples: &'a [String],
        tests: &'a [String],
        benches: &'a [String],
    }
}

// Proposed

pub enum FilterRule<'a> {
    Everything,
    Only (&'a [String]),
}

pub enum CompileFilter<'a> {
    lib: bool,
    bins: FilterRule<'a>,
    examples: FilterRule<'a>,
    tests: FilterRule<'a>,
    benches: FilterRule<'a>,
}

With this I want to express that:

• Choice between Everything/Only shall be possible for each "thing" independently.
• CompileFilter would stop being an enum.

Where these aren't sure yet:

• I haven't looked into the "default" value of lib in the case of Everything in the current implementation.
  It either stays a bool or becomes an enum of FORCE_LIB, FORCE_BIN, AUTO or something.
• I didn't even attempt to compile that, I just want to express the general approach.

I'll go ahead and try to implement it like this. Or do you prefer a different way?

EDIT: Oh Github Hotkeys, I love you. Sorry for the spurious initial version.

[alexcrichton]

@BenWiederhake yeah that sounds great to me! I've often wanted to add something like cargo test --bins or cargo test --tests and I think that'd fit the bill nicely.

[BenWiederhake]

bin/doc.rs:95:            filter: ops::CompileFilter::new(options.flag_lib,
bin/doc.rs-96-                                            &options.flag_bin,
bin/doc.rs-97-                                            &empty,
bin/doc.rs-98-                                            &empty,
bin/doc.rs-99-                                            &empty),

The current behavior goes like this: If the user specifies --lib, --bin NAME, or both, then no examples, benches, tests will be built. If the user doesn't specify anything, then all libs, bins, and also all examples, benches, and tests will be built.

With the new representation, it is much easier to write compile::generate_targets (which needs to be rewritten anyway) so that it behave like this:
If the user specifies --lib, --bin NAME, or both, then no examples, benches, tests will be built (no change). If the user doesn't specify anything, then all libs and bins are built (no change here), but none of the examples, benches, or tests (change!).

---

EDIT: Ah, no, obviously CompileMode restricts this, and only libs and bins end up documented, as one can see in core::manifest::Target::*.

Looks I need to read more first :)

[alexcrichton]

Ah yeah so to clarify the intention was that we'd support crazy invocations like:

cargo test --tests --lib --bin foo

or basically any mixture of all these flags, so I don't think we'd want any of them to be mutually exclusive.

[BenWiederhake]

Yup, I'm aware of this. I think I found a nice interpretation for the flags that stays backwards compatible.

How should we handle combinations like --all-tests --test foobar? I see two ways to interprete that:

1. It's contradictory and says: "Consider all compatible tests, but only foobar."
2. It's perfectly fine and means: "Consider all compatible tests, and make sure foobar is among them."

I'll go and try to implement 1, as it is slightly easier. Or do you think that anyone needs "2"?

[alexcrichton]

Oh I'd consider flags on the command line to always be "unioned" together in the sense that they're always composable. That way --tests --test foo just means "all tests"

[BenWiederhake]

So now there's at least three potential interpretations. Do you really think it's a good idea to specify the behavior of something for which there are multiple reasonable interpretations?

[BenWiederhake]

--tests --test foo just means "all tests"

I went with this interpretations since I didn't want to introduce a new CliError value.

[BenWiederhake]

So I implemented it, but running cargo test is inconclusive as even master fails a lot of tests. Huh.

How should I proceed? I don't want to open a PR yet, I haven't checked the style guidelines yet.

[alexcrichton]

It looks like those test failures may be related to -C target-cpu=native, do you know how that flag is getting inserted?

[BenWiederhake]

Argh, my fault, I didn't see that this is why the tests are failing. Yes, it's a flag in a local .cargo/config. I'll test everything again.

[alexcrichton]

Ah ok! That's covered by https://github.com/rust-lang/cargo/issues/3863

[BenWiederhake]

Looks like it's going well.

I guess I should also provide tests which actually make use of --all-bins and such. For example, cargo run --all-bins should work if and only if the number of bins is 1. That would go into tests/run.rs, right? I really like the test infrastructure, it's quite well readable!

[alexcrichton]

Yeah I think we'll definitely want tests for a number of the various cases here, and yeah --all-bins is ok to work w/ cargo run if there's only one binary, I see these as filters and then restrictions like "you must run a specific binary" are orthogonal to the filters.

[BenWiederhake]

This'll take longer than expected, apparently I broke a lot of implicit assumptions.

[BenWiederhake]

Fascinating. Apparently it wasn't me, it's just that the implicit assumption is just … wrong.

ops::cargo_run.rs::run() does it's own test with "blackjack and hookers" to determine how many binaries there are, independently of cargo_compile. The former does not respect features, the latter does. Here's how to crash the current Cargo, not yet touched by my code:

// BEGIN Cargo.toml
[package]
name = "foo"
version = "0.1.0"
authors = []

[features]
default = []
phantom = []

[[bin]]
name = "foobar"
path = "src/main.rs"
required-features = ["phantom"]
// END Cargo.toml
// BEGIN src/lib.rs
// END src/lib.rs
// BEGIN src/main.rs
fn main() {
    println!("hello");
}
// END src/main.rs

Now invoke cargo run. Obviously, that's a stupid thing to attempt, but it should still print a proper error instead.

This is a reminder to myself that I want to add a test case for this, after fixing it. (The logic that determines how many binaries there are needs to be adapted heavily anyway.)

EDIT: Just to make sure: I'm fully aware what the bug is and where it lives, I just wanted to point out that this bug becomes more obvious due to the new --all-${KINDS} flags.

[alexcrichton]

Oh dear sounds bad! If you end up fixing that in a drive-by commit it'd be more than welcome :)

[BenWiederhake]

Uhh, so I can only come up with these ways to handle the additional issue:

• Just compile first, and do any checks (e.g. "there is exactly one binary") afterwards. Doesn't work, as this would be a performance bug (unnecessarily slow) and many tests check for stdout, which would change (due to compilation).
• Refactor cargo_compile.rs so that the outside modules can inspect the target list. I might do that one day, but not now.
• Improve cargo_run's emulation of cargo_compile. This would be possible, but it smells. I don't want my name on crap like that, so I won't implement it.
• Leave the bug in there. The least work, and apparently nobody cares anyway.

I'll go ahead and implement the latter, leaving #3867 for later.

[BenWiederhake]

So I hunted down some bugs in my code, and there seems to be one where I don't know how to go about debugging it. This particular test:

cargo test --test test -- test_many_targets

Fails in a strange way. One can easily reproduce it by manually invoking it:

$ cd path/to/prepared/folder
$ my/cargo/target/debug/cargo test --bin a --bin b
error: Invalid arguments.

Usage:
    cargo test [options] [--] [<args>...]
$ echo $?
1
$

It appears to just fail to parse the arguments, but why? Did I forget to change something in the Options struct? (buildable version)

I'm aware that only_test_docs is also failing in that tree, but there I think I already know where to look.

[alexcrichton]

Ah yeah leaving #3867 is fine for now, thanks for opening an issue!

Oh for that it's a docopt thing whether --bin is accepted multiple times, you'll want to add ... to the doc string (I think it's there for -p currently)

[BenWiederhake]

You mean the pub const USAGE: &'static str = " doc string? That's quite non-obvious, but … it works, thanks!

But there's still something I don't understand: Why is it not needed for so many other things then? As far as I can see, bench build check install rustc didn't use ..., and test certainly didn't. Not sure about doc rustdoc, but they don't use it either.
tl;dr: bin/test.rs didn't use to contain .... Why is it now necessary? Should all other "binaries" contain it?

Also, I don't quite understand the failure of only_test_docs. It appears to truly call twice into cargo_compile, as far as some sprinkled-in debug!(…); messages show. I'm not sure why. Any hints?

[alexcrichton]

Oh I forget the precise intricacies of how docopt works unfortunately, you may have to consult that documentation.

Also hm that sounds quite odd coming into cargo_compile twice, I think that's likely unexpected and maybe a bug!

[BenWiederhake]

I give up for now, since I have no idea why this change breaks cargo test --doc, i.e., the only_test_docs test. I might rebase it once #3879 lands and check whether it is magically repaired by that, but I don't quite believe in magic.

EDIT: I should mention that RUSTLOG=debug path/to/cargo test --doc is not insightful. The generated target list by cargo_compile stays the same (one element), and not much changes (only timings). The second "cycle" seems to appear out of nowhere.

[BenWiederhake]

Erroneously closed, please re-open

[alexcrichton]

@BenWiederhake want to gist some of the logs you've been seeing? If you want to make a WIP PR I can also try to help out there

[BenWiederhake]

Here is a diff between the outputs. The command cargo test --doc was run in an environment that reproduces the regression introduced by my changes. The versions compared are:

• master from 24th March, plus most of the debug! messages sprinkled in as:
• this WIP runnable version, which also reports the set of targets being built.

As you can see, the debug messages don't indicate any difference (except in timing) until "suddenly" the second run of the Doc-tests foo begin.

As the list of enumerated targets is printed (and proven to be of length 1, so cargo_compile definitely doesn't do duplicate work), I can only guess that I broke an assumption somewhere in the "runner", or wherever the loop that says for testunit in units_to_be_built is located.

[alexcrichton]

Maybe some target is being added twice to the final list by accident or something like that? It may help to debug where the relevant arrays are constructed to see why they contain too many elements.

[BenWiederhake]

Turns out that the bug is somewhere else entirely:

-        filter = ops::CompileFilter::new(true, &empty, &empty, &empty, &empty);
+        filter = ops::CompileFilter::Everything;

Back when I did that change, I assumed that libonly=true (the first argument) is the standard, non-specific value. That's obviously wrong, but I forgot to go back and fix it when I learned that. This change violated the implicit assumption options.only_doc => filter is specific. After adding an assert for this assumption (in case someone breaks it again in the future) and fixing my code, it seems to work again.

All existing tests are now passing. I'm working on adding additional tests and rebasing it on top of current master (including #3879), then I can think about creating a PR.

[alexcrichton]

Oh awesome, thanks for tracking that down!

[behnam]

Since I haven't seen it being discussed, is there already a shortcut for --lib --bins --examples --tests --benches, so we don't have to type all of them?

If not, maybe we can add one, like --everything or --all-crates?

And then maybe --all can have a new name, like --all-packages, to prevent confusions.

IMHO, --all-crates and --all-packages are easier to remember than --all and some other new keyword, like --everything.

[alexcrichton]

@behnam I think that's https://github.com/rust-lang/cargo/issues/3431 (or https://github.com/rust-lang/cargo/issues/2495) with a PR at https://github.com/rust-lang/cargo/pull/4157

[liuchong]

Very nice feature! :smile_cat: