Stricter need_dev_deps behaviour #5186
Conversation
This reverts commit d46db71.
- Also add a mode param to need_dev_deps to make clear the intent
- generate_{auto => default}_target since it matches on CompileFilter::Default
- CompileFilter::{matches => target_run} to make it clear it only affects `cargo run`
- Add a comment pointing to generate_target for other subcommands
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| @@ -62,7 +62,12 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult { | |||
| } | |||
| }; | |||
| let mut compile_opts = args.compile_options_for_single_package(config, mode)?; | |||
| compile_opts.target_rustc_args = Some(values(args, "args")); | |||
| let target_args = values(args, "args"); | |||
| compile_opts.target_rustc_args = if target_args.is_empty() { | |||
There was a problem hiding this comment.
Was this change needed for the correctness of the patch? I think it changes the behavior of cargo rustc by default (erroring today whereas after this patch it succeeds and builds multiple crates)
There was a problem hiding this comment.
Yes, this is needed for e.g. cargo rustc with no args to work in the first place.
Assuming "package" and "crate" are the same thing, the behaviour you're describing is tested for by fail_with_multiple_packages which still passes. There is also a fails_when_trying_to_build_main_and_lib_with_args test for failing when multiple targets are selected, and it is still passing as well.
There was a problem hiding this comment.
Yes, this is needed for e.g.
cargo rustcwith no args to work in the first place.
Sorry, this is not correct. The hunk here is needed for cargo rustc --all-targets with no further args to work, which is documented as part of its CLI. Furthermore it's also needed when passing cargo args like -v, as opposed to binary-target args that go after -- which is what fails_when_trying_to_build_main_and_lib_with_args tests for and this is still passing.
There was a problem hiding this comment.
Hm I think the documentation is wrong then? The cargo rustc command was only ever intended to work with only one target
There was a problem hiding this comment.
Ok but my commit here doesn't affect this (i.e. it was broken before and it's broken afterwards), it only fixes the -- related behaviour that was broken by #5152. If you run cargo rustc using cargo 0.25.0 (the current stable) then it will still work with a multi-target crate, e.g.
$ cat Cargo.toml
[package]
name = "foo"
version = "0.0.1"
authors = []
$ cat src/lib.rs
pub fn myfunc() {}
$ cat src/main.rs
fn main () {}
$ cargo --version
cargo 0.25.0
$ cargo rustc --verbose
Compiling foo v0.0.1 (file://$PWD)
Running `rustc --crate-name foo src/lib.rs --crate-type lib --emit=dep-info,link -C debuginfo=2 -C metadata=0fa9827c683f047b -C extra-filename=-0fa9827c683f047b --out-dir $PWD/target/debug/deps -L dependency=$PWD/target/debug/deps`
Running `rustc --crate-name foo src/main.rs --crate-type bin --emit=dep-info,link -C debuginfo=2 -C metadata=a9125b8469059127 -C extra-filename=-a9125b8469059127 --out-dir $PWD/target/debug/deps -L dependency=$PWD/target/debug/deps --extern foo=$PWD/target/debug/deps/libfoo-0fa9827c683f047b.rlib`
Finished dev [unoptimized + debuginfo] target(s) in 0.35 secs
There was a problem hiding this comment.
Ah I see... odd! Well in any case it looks like nothing's changing so seems good to land!
tests/testsuite/build.rs
Outdated
| [RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \ | ||
| -C debuginfo=2 --test [..]"), | ||
| ); | ||
| assert_that(p.cargo("clean"), execs().with_status(0)); |
There was a problem hiding this comment.
Unfortunately we've had pretty bad luck with commands like this which delete the target directory (intermittent failures on windows due to presumed mspdbsrv.exe instances), in light of that could these tests be extracted to individual tests?
|
@bors: r+ |
|
📌 Commit ce26ddf has been approved by |
|
@bors: r+ |
|
📌 Commit 0bc1155 has been approved by |
Stricter need_dev_deps behaviour The previous PR (#5012) contained an unnecessary work-around for behaviour of `--all-targets` that was misunderstood. This PR removes that work-around and adds some tests and comments to clarify the behaviour for future contributors, which may help to make easier a future fix for #5177 and #5178.
|
☀️ Test successful - status-appveyor, status-travis |
Applied-Upstream: rust-lang/cargo#5012 Applied-Upstream: rust-lang/cargo#5186 Gbp-Pq: Name 1001_PR5012.patch
Applied-Upstream: rust-lang/cargo#5012 Applied-Upstream: rust-lang/cargo#5186 Gbp-Pq: Name 1001_PR5012.patch
Applied-Upstream: rust-lang/cargo#5012 Applied-Upstream: rust-lang/cargo#5186 Gbp-Pq: Name 1001_PR5012.patch
The previous PR (#5012) contained an unnecessary work-around for behaviour of
--all-targetsthat was misunderstood. This PR removes that work-around and adds some tests and comments to clarify the behaviour for future contributors, which may help to make easier a future fix for #5177 and #5178.