Auto merge of #5186 - infinity0:stricter-need-dev-deps, r=alexcrichton

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.

src/bin/commands/rustc.rs

@@ -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() {
+        None
+    } else {
+        Some(target_args)
+    };
     ops::compile(&ws, &compile_opts)?;
     Ok(())
 }

src/bin/commands/rustdoc.rs

@@ -49,7 +49,12 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     let ws = args.workspace(config)?;
     let mut compile_opts =
         args.compile_options_for_single_package(config, CompileMode::Doc { deps: false })?;
-    compile_opts.target_rustdoc_args = Some(values(args, "args"));
+    let target_args = values(args, "args");
+    compile_opts.target_rustdoc_args = if target_args.is_empty() {
+        None
+    } else {
+        Some(target_args)
+    };
     let doc_opts = DocOptions {
         open_result: args.is_present("open"),
         compile_opts,

src/cargo/ops/cargo_compile.rs

@@ -246,7 +246,7 @@ pub fn compile_ws<'a>(
     let specs = spec.into_package_id_specs(ws)?;
     let features = Method::split_features(features);
     let method = Method::Required {
-        dev_deps: ws.require_optional_deps() || filter.need_dev_deps(),
+        dev_deps: ws.require_optional_deps() || filter.need_dev_deps(mode),
         features: &features,
         all_features,
         uses_default_features: !no_default_features,
@@ -442,19 +442,25 @@ impl CompileFilter {
         }
     }
 
-    pub fn need_dev_deps(&self) -> bool {
+    pub fn need_dev_deps(&self, mode: CompileMode) -> bool {
-        match *self {
+        match mode {
-            CompileFilter::Default { .. } => true,
+            CompileMode::Test | CompileMode::Doctest | CompileMode::Bench => true,
+            CompileMode::Build | CompileMode::Doc { .. } | CompileMode::Check { .. } => match *self
+            {
+                CompileFilter::Default { .. } => false,
                 CompileFilter::Only {
                     ref examples,
                     ref tests,
                     ref benches,
                     ..
                 } => examples.is_specific() || tests.is_specific() || benches.is_specific(),
+            },
         }
     }
 
-    pub fn matches(&self, target: &Target) -> bool {
+    // this selects targets for "cargo run". for logic to select targets for
+    // other subcommands, see generate_targets and generate_default_targets
+    pub fn target_run(&self, target: &Target) -> bool {
         match *self {
             CompileFilter::Default { .. } => true,
             CompileFilter::Only {
@@ -493,7 +499,7 @@ struct BuildProposal<'a> {
     required: bool,
 }
 
-fn generate_auto_targets<'a>(
+fn generate_default_targets<'a>(
     mode: CompileMode,
     targets: &'a [Target],
     profile: &'a Profile,
@@ -715,7 +721,7 @@ fn generate_targets<'a>(
             } else {
                 &profiles.test_deps
             };
-            generate_auto_targets(
+            generate_default_targets(
                 mode,
                 pkg.targets(),
                 profile,

src/cargo/ops/cargo_run.rs

@@ -33,7 +33,7 @@ pub fn run(
             !a.is_lib() && !a.is_custom_build() && if !options.filter.is_specific() {
                 a.is_bin()
             } else {
-                options.filter.matches(a)
+                options.filter.target_run(a)
             }
         })
         .map(|bin| bin.name())

tests/testsuite/build.rs

@@ -5381,6 +5381,70 @@ fn build_filter_infer_profile() {
     );
 }
 
+#[test]
+fn targets_selected_default() {
+    let p = project("foo")
+        .file(
+            "Cargo.toml",
+            r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+            authors = []
+        "#,
+        )
+        .file("src/main.rs", "fn main() {}")
+        .build();
+    assert_that(
+        p.cargo("build").arg("-v"),
+        execs().with_status(0)
+        // bin
+        .with_stderr_contains("\
+            [RUNNING] `rustc --crate-name foo src[/]main.rs --crate-type bin \
+            --emit=dep-info,link[..]")
+        // bench
+        .with_stderr_does_not_contain("\
+            [RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
+            -C opt-level=3 --test [..]")
+        // unit test
+        .with_stderr_does_not_contain("\
+            [RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
+            -C debuginfo=2 --test [..]"),
+    );
+}
+
+#[test]
+fn targets_selected_all() {
+    let p = project("foo")
+        .file(
+            "Cargo.toml",
+            r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+            authors = []
+        "#,
+        )
+        .file("src/main.rs", "fn main() {}")
+        .build();
+    assert_that(
+        p.cargo("build").arg("-v").arg("--all-targets"),
+        execs().with_status(0)
+        // bin
+        .with_stderr_contains("\
+            [RUNNING] `rustc --crate-name foo src[/]main.rs --crate-type bin \
+            --emit=dep-info,link[..]")
+        // bench
+        .with_stderr_contains("\
+            [RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
+            -C opt-level=3 --test [..]")
+        // unit test
+        .with_stderr_contains("\
+            [RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
+            -C debuginfo=2 --test [..]"),
+    );
+}
+
 #[test]
 fn all_targets_no_lib() {
     let p = project("foo")
@@ -5471,11 +5535,9 @@ fn avoid_dev_deps() {
         .file("src/main.rs", "fn main() {}")
         .build();
 
-    // --bins is needed because of #5134
+    assert_that(p.cargo("build"), execs().with_status(101));
-    assert_that(p.cargo("build").arg("--bins"), execs().with_status(101));
     assert_that(
         p.cargo("build")
-            .arg("--bins")
             .masquerade_as_nightly_cargo()
             .arg("-Zavoid-dev-deps"),
         execs().with_status(0),

tests/testsuite/check.rs

@@ -606,7 +606,30 @@ fn check_virtual_all_implied() {
 }
 
 #[test]
-fn check_all_targets() {
+fn targets_selected_default() {
+    let foo = project("foo")
+        .file("Cargo.toml", SIMPLE_MANIFEST)
+        .file("src/main.rs", "fn main() {}")
+        .file("src/lib.rs", "pub fn smth() {}")
+        .file("examples/example1.rs", "fn main() {}")
+        .file("tests/test2.rs", "#[test] fn t() {}")
+        .file("benches/bench3.rs", "")
+        .build();
+
+    assert_that(
+        foo.cargo("check").arg("-v"),
+        execs()
+            .with_status(0)
+            .with_stderr_contains("[..] --crate-name foo src[/]lib.rs [..]")
+            .with_stderr_contains("[..] --crate-name foo src[/]main.rs [..]")
+            .with_stderr_does_not_contain("[..] --crate-name example1 examples[/]example1.rs [..]")
+            .with_stderr_does_not_contain("[..] --crate-name test2 tests[/]test2.rs [..]")
+            .with_stderr_does_not_contain("[..] --crate-name bench3 benches[/]bench3.rs [..]"),
+    );
+}
+
+#[test]
+fn targets_selected_all() {
     let foo = project("foo")
         .file("Cargo.toml", SIMPLE_MANIFEST)
         .file("src/main.rs", "fn main() {}")

tests/testsuite/install.rs

@@ -1222,9 +1222,8 @@ fn dev_dependencies_no_check() {
         .file("src/main.rs", "fn main() {}")
         .build();
 
-    // --bins is needed because of #5134
+    assert_that(p.cargo("build"), execs().with_status(101));
-    assert_that(p.cargo("build").arg("--bins"), execs().with_status(101));
+    assert_that(p.cargo("install"), execs().with_status(0));
-    assert_that(p.cargo("install").arg("--bins"), execs().with_status(0));
 }
 
 #[test]
@@ -1256,10 +1255,9 @@ fn dev_dependencies_lock_file_untouched() {
         .file("a/src/lib.rs", "")
         .build();
 
-    // --bins is needed because of #5134
+    assert_that(p.cargo("build"), execs().with_status(0));
-    assert_that(p.cargo("build").arg("--bins"), execs().with_status(0));
     let lock = p.read_lockfile();
-    assert_that(p.cargo("install").arg("--bins"), execs().with_status(0));
+    assert_that(p.cargo("install"), execs().with_status(0));
     let lock2 = p.read_lockfile();
     assert!(lock == lock2, "different lockfiles");
 }

tests/testsuite/rustc.rs

@@ -413,6 +413,70 @@ fn build_only_bar_dependency() {
     );
 }
 
+#[test]
+fn targets_selected_default() {
+    let p = project("foo")
+        .file(
+            "Cargo.toml",
+            r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+            authors = []
+        "#,
+        )
+        .file("src/main.rs", "fn main() {}")
+        .build();
+    assert_that(
+        p.cargo("rustc").arg("-v"),
+        execs().with_status(0)
+        // bin
+        .with_stderr_contains("\
+            [RUNNING] `rustc --crate-name foo src[/]main.rs --crate-type bin \
+            --emit=dep-info,link[..]")
+        // bench
+        .with_stderr_does_not_contain("\
+            [RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
+            -C opt-level=3 --test [..]")
+        // unit test
+        .with_stderr_does_not_contain("\
+            [RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
+            -C debuginfo=2 --test [..]"),
+    );
+}
+
+#[test]
+fn targets_selected_all() {
+    let p = project("foo")
+        .file(
+            "Cargo.toml",
+            r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+            authors = []
+        "#,
+        )
+        .file("src/main.rs", "fn main() {}")
+        .build();
+    assert_that(
+        p.cargo("rustc").arg("-v").arg("--all-targets"),
+        execs().with_status(0)
+        // bin
+        .with_stderr_contains("\
+            [RUNNING] `rustc --crate-name foo src[/]main.rs --crate-type bin \
+            --emit=dep-info,link[..]")
+        // bench
+        .with_stderr_contains("\
+            [RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
+            -C opt-level=3 --test [..]")
+        // unit test
+        .with_stderr_contains("\
+            [RUNNING] `rustc --crate-name foo src[/]main.rs --emit=dep-info,link \
+            -C debuginfo=2 --test [..]"),
+    );
+}
+
 #[test]
 fn fail_with_multiple_packages() {
     let foo = project("foo")