Add cargo run --example and cargo run --bin
#702
Conversation
|
Hm, the Travis failures seem to be unrelated to this change and I've seen them it other PRs as well. What's the protocol here? @alexcrichton you suggested this in #538, r? |
|
Ah yes sorry, I've been meaning to get around to this! I think it's safe to ignore the travis failures. This is quite similar to #685 (cc @LexxFedoroff) in its goal of "run just one specific target", and I'd like to consider both together to make sure that we've got a consistent story across all the cargo commands. This PR, for example, only modifies I think I might pefer the So all in all, @LexxFedoroff, how do you feel about moving from cc @wycats, I'm curious if you have an opinion here. |
|
Sorry, I thought that pull request was about the target triples. Yeah, being able to build/run a specific command, test or benchmark would be great. I prefer specifying the binary type (example, bin, test), too. At least in Maybe we could use something like |
|
I was chatting with @wycats about this, and I think your intuitions is right @tomassedovic, the common case should be the same. We ended up realizing that the "odd one out" are examples, so how about:
We may want to avoid Does that sound ok? |
|
Sounds good to me! Not quite sure about having @LexxFedoroff, I don't want to step on your toes (your patch came first, I should have noticed). If this makes sense to you, do you want to pick it up? |
|
@tomassedovic I like your approach, I do not mind if you do this. |
|
Allright, will do! |
|
@tomassedovic please add also |
|
@vhbit where do you want to add it? If I understand it correctly, the current plan is this:
I'm not sure I understand what should |
|
@tomassedovic I though of |
|
sorry I wasn't clear enough, I see this PR as a way to get a "focused" output out of all possibilities and in my use case getting just lib without binaries at all perfectly fits it. |
|
Ah, I see. That would be better as a separate change, I think. This PR is more about being able to run stuff (where stuff = test, example and "normal" binaries). Adding |
|
@vhbit could you open an issue on that as well to track it? Seems like something good to have! |
|
@alexcrichton here it is #724 |
|
Thanks! |
tomassedovic
force-pushed
the
run-examples
branch
from
af51323 to
386eb37
Compare
|
Oops, I meant to squash the fixup commit up. Will fix that in a later revision. Anyway, this is pretty much what we agreed on. There seems to be an issue when we have a bin and an example with the same name, but everything else seems to be working. I rebased the |
| }).collect::<Vec<&Target>>(); | ||
| let targets = to_build.get_targets().iter() | ||
| .filter(|target| { | ||
| let name_eq = name.is_none() || (name.unwrap() == target.get_name()); |
There was a problem hiding this comment.
This could avoid an unwrap with name.is_none() || name == Some(target.get_name())
There was a problem hiding this comment.
Some(target.get_name()) results in a lifetime error in a seemingly unrelated bit of code above ("packages does not live long enough" in line 121).
map_or works fine though, so I haven't bothered to investigate it:
let name_eq = name.map_or(true, |name| name == target.get_name());
(FWIW, I don't mind switching to a full match, if map_or is too cryptic)
|
This looks fantastic, thanks so much @tomassedovic! The test failure on travis looks somewhat worrying, can you reproduce it locally? |
|
Oh interesting. I've seen the same error Travis reports once but then it went away. Removing the |
|
Hm, this is bizarre. So with your suggestions applied, the last test that was failing for me is fixed and I can no longer reproduce the Travis failure with However, when I created a cargo project with identically-named bin and example executables, I'm hitting a multitude of issues. The problems only seems to happen on a clean checkout and only if we run the example first. A race condition of some kind seems to be involved since I'm getting four different behaviours:
However, when I try to run (and compile) the Although as the warning suggests, we seem to be compiling the bin again in a test environment. This also suggests a possibility that the reason I'll upload the code I have now and will return to this later. Though if anything jumps out at you, please point it out. I've no idea what's going on here. |
tomassedovic
force-pushed
the
run-examples
branch
from
386eb37 to
6bdde43
Compare
|
That definitely looks like a race, and I think it has to do with two targets having the same output filename, and you're right in that it looks like I'll take a look at the patch again to see if I can tell what's going on. |
| All of the trailing arguments are passed as to the binary to run. | ||
| "; | ||
|
|
||
| pub fn execute(options: Options, shell: &mut MultiShell) -> CliResult<Option<()>> { | ||
| shell.set_verbose(options.flag_verbose); | ||
| let root = try!(find_root_manifest_for_cwd(options.flag_manifest_path)); | ||
| let env = if options.flag_example.is_some() { | ||
| "test" |
There was a problem hiding this comment.
This may be one point of failure for the race. If you're filtering for all targets with the name "foo" with the "test" environment you'll get the example "foo" as well as the test case for "foo" itself. To help weed this out, could you add -v to your test cases to make sure the compiler is invoked only once when you request just one example? That should help ensure that only one target is actually compiled.
There was a problem hiding this comment.
Ah yes and the other case is happening because there's a Target for each binary with test env because the examples and/or tests could actually rely on the binaries themselves.
So what should happen here is:
- When I run
cargo run --name foo, it should only compile the binary target namedfoo. - When I run
cargo run --example foo, it should compile all binaries, and then compile the example named foo, but none of the test binaries should be built.
Does that make sense? I do think that the usage of many many Targets is somewhat confusing, sorry about that!
There was a problem hiding this comment.
Yeah. Also, I've just noticed that cargo run --name or --example fails if the file contains extern crate project_name.
Looks like the modification to cargo_compile.rs in the first commit is not behaving as desired. I'll give this some more thought.
There was a problem hiding this comment.
Ah I think that would indeed make sense! You may want to alter cargo_compile to have a bit of a two-phase filtering strategy. The first pass would be the same as today (filtering based on the environment), and the second pass would be filtering based off the name requested. If --name was requested with cargo run then the only thing that should be filtered out is other binaries (and the same for tests), but libraries should stick around.
Could you add tests for these cases as well? Thanks for being so thorough with this!
|
Good points, thanks. Looks like Cargo isn't currently geared towards different things (tests, bins, examples) having the same name.
I've opened issue #751 to track that and I'll update this PR based on how it turns out. |
|
Now that #758 has landed, I think this just needs a rebase and its good to go! |
tomassedovic
force-pushed
the
run-examples
branch
from
6bdde43 to
f246b5e
Compare
|
Okay, this seems to survive everything I throw at it. It's a bit different from the previous versions: I dropped the modifications to cargo_compile. When the user passes I did that because the logic for picking the right target is a bit different for |
| "#) | ||
| .file("src/bin/b.rs", r#" | ||
| fn main() { println!("hello b.rs"); } | ||
| "#); |
There was a problem hiding this comment.
Could you add a couple of extern crate foo in here to ensure that the lib is built for the bins?
There was a problem hiding this comment.
Good idea, done.
|
Are the semantics here that if you have 100 bins that |
|
|
||
| assert_that(prj.cargo_process("test").arg("--name").arg("bin2"), | ||
| execs().with_status(0).with_stdout(expected_stdout.as_slice())); | ||
| }) |
There was a problem hiding this comment.
Could you also add a test for tests/a.rs and tests/b.rs and run cargo test --name a? It may also be worth adding src/bin/a.rs and tests/a.rs and make sure that --name doesn't generate an error, running both is ok to me.
tomassedovic
force-pushed
the
run-examples
branch
from
f246b5e to
47e1861
Compare
|
You're right that this compiles all the targets, not just the one being run. I was originally thinking of this as: But yeah, if you have a bunch of targets, this can take longer than necessary. On the other hand, can we merge this in the meantime (if you're happy with the code) and add the optimization as a separate change? It'll probably take me some more time to figure out what needs to be changed, add the right tests, etc. |
Sure! Thanks again for this, this is great! |
This lets us compile and run examples using `cargo run --example NAME`. Selecting the other binary targets is now done using the `--name` flag. `cargo run` falls back to the old behaviour (running the only bin target in the project, failing if there are more) in neither `--name` nor `--example` are present. Closes rust-lang#538
|
|
||
| assert_that(prj.cargo_process("test").arg("--name").arg("b"), | ||
| execs().with_status(0).with_stdout(expected_stdout.as_slice())); | ||
| }) |
There was a problem hiding this comment.
It looks like this tests is failing because either the bin or test is running first (nondeterministically). I'm also curious, shouldn't this have a {running} target[..]b-[..] for the second command that's being run?
There was a problem hiding this comment.
I think the nondeterminism is caused by the suffix (e.g. b-5ceb27d70f3864a2 vs. b-f1d1a5ae2be55cf5) that gets added to the file name. The paths are being sorted:
I'm not sure where the suffix comes from -- is that something we can control so the order is consistent? Good point about the missing {running} section. When I just tried it manually, it's there, but the tests didn't complain.
There was a problem hiding this comment.
Hm weird, maybe our matching algorithm for the tests isn't quite up to par... I suppose the metadata (the extra stem of the filename) could be different across platforms, so it could probably run out of order yeah. Could you modify it so that both the test and the bin b have one test of the same name so the output is the same? that way we don't have to worry about sorting
There was a problem hiding this comment.
Ha, that's neat. Done.
You can now run a single test/bench file by passing its name to `cargo test` or `cargo bench`.
tomassedovic
force-pushed
the
run-examples
branch
from
47e1861 to
f93506a
Compare
This lets users run any executable from the `examples` or `bin` directories by passing its name with `--example` or `--bin` flag. If neither is specified, we fall back to the old behaviour (running the only bin target in the project, failing if there are more). Closes #538
This lets users run any executable from the
examplesorbindirectories bypassing its name with
--exampleor--binflag.If neither is specified, we fall back to the old behaviour (running the only bin
target in the project, failing if there are more).
Closes #538