Fix doctests linking too many libs.

Fixes rust-lang#5650.  cc rust-lang#5435

As part of my recent work on profiles, I introduced some situations where a
library can be compiled multiple times with different settings.  Doctests were
greedily grabbing all dependencies for a package, regardless of which target
is was for.  This can cause doctests to fail if it links multiple copies of
the same library.

One way to trigger this is `cargo test --release` if you have dependencies, a
build script, and `panic="abort"`.  There are other (more obscure) ways to
trigger it with profile overrides.

src/cargo/core/compiler/compilation.rs

@@ -50,7 +50,9 @@ pub struct Compilation<'cfg> {
     /// be passed to future invocations of programs.
     pub extra_env: HashMap<PackageId, Vec<(String, String)>>,
 
-    pub to_doc_test: Vec<Package>,
+    /// Libraries to test with rustdoc.
+    /// The third value is the list of dependencies.
+    pub to_doc_test: Vec<(Package, Target, Vec<(Target, PathBuf)>)>,
 
     /// Features per package enabled during this compilation.
     pub cfgs: HashMap<PackageId, HashSet<String>>,

src/cargo/core/compiler/context/mod.rs

@@ -175,7 +175,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                     None => &output.path,
                 };
 
-                if unit.mode.is_any_test() && !unit.mode.is_check() {
+                if unit.mode == CompileMode::Test {
                     self.compilation.tests.push((
                         unit.pkg.clone(),
                         unit.target.kind().clone(),
@@ -227,6 +227,25 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                     );
             }
 
+            if unit.mode == CompileMode::Doctest {
+                let mut doctest_deps = Vec::new();
+                for dep in self.dep_targets(unit) {
+                    if dep.target.is_lib() && dep.mode == CompileMode::Build {
+                        let outputs = self.outputs(&dep)?;
+                        doctest_deps.extend(
+                            outputs
+                                .iter()
+                                .map(|output| (dep.target.clone(), output.path.clone())),
+                        );
+                    }
+                }
+                self.compilation.to_doc_test.push((
+                    unit.pkg.clone(),
+                    unit.target.clone(),
+                    doctest_deps,
+                ));
+            }
+
             let feats = self.bcx.resolve.features(unit.pkg.package_id());
             if !feats.is_empty() {
                 self.compilation

src/cargo/ops/cargo_compile.rs

@@ -280,7 +280,7 @@ pub fn compile_ws<'a>(
         extra_compiler_args = Some((units[0], args));
     }
 
-    let mut ret = {
+    let ret = {
         let _p = profile::start("compiling");
         let bcx = BuildContext::new(
             ws,
@@ -295,8 +295,6 @@ pub fn compile_ws<'a>(
         cx.compile(&units, export_dir.clone(), &exec)?
     };
 
-    ret.to_doc_test = to_builds.into_iter().cloned().collect();
-
     return Ok(ret);
 }
 
@@ -541,9 +539,9 @@ fn generate_targets<'a>(
                     .collect::<Vec<_>>();
                 proposals.extend(default_units);
                 if build_config.mode == CompileMode::Test {
-                    // Include the lib as it will be required for doctests.
+                    // Include doctest for lib.
                     if let Some(t) = pkg.targets().iter().find(|t| t.is_lib() && t.doctested()) {
-                        proposals.push((new_unit(pkg, t, CompileMode::Build), false));
+                        proposals.push((new_unit(pkg, t, CompileMode::Doctest), false));
                     }
                 }
             }
@@ -681,15 +679,7 @@ fn generate_default_targets(targets: &[Target], mode: CompileMode) -> Vec<&Targe
                 })
                 .collect()
         }
-        CompileMode::Doctest => {
-            // `test --doc``
-            targets
-                .iter()
-                .find(|t| t.is_lib() && t.doctested())
-                .into_iter()
-                .collect()
-        }
-        CompileMode::RunCustomBuild => panic!("Invalid mode"),
+        CompileMode::Doctest | CompileMode::RunCustomBuild => panic!("Invalid mode {:?}", mode),
     }
 }
 

src/cargo/ops/cargo_test.rs

@@ -154,86 +154,72 @@ fn run_doc_tests(
         return Ok((Test::Doc, errors));
     }
 
-    let libs = compilation.to_doc_test.iter().map(|package| {
-        (
-            package,
-            package
-                .targets()
-                .iter()
-                .filter(|t| t.doctested())
-                .map(|t| (t.src_path(), t.name(), t.crate_name())),
-        )
-    });
-
-    for (package, tests) in libs {
-        for (lib, name, crate_name) in tests {
-            config.shell().status("Doc-tests", name)?;
-            let mut p = compilation.rustdoc_process(package)?;
-            p.arg("--test")
-                .arg(lib)
-                .arg("--crate-name")
-                .arg(&crate_name);
-
-            for &rust_dep in &[&compilation.deps_output] {
-                let mut arg = OsString::from("dependency=");
-                arg.push(rust_dep);
-                p.arg("-L").arg(arg);
-            }
+    for (package, target, deps) in &compilation.to_doc_test {
+        config.shell().status("Doc-tests", target.name())?;
+        let mut p = compilation.rustdoc_process(package)?;
+        p.arg("--test")
+            .arg(target.src_path())
+            .arg("--crate-name")
+            .arg(&target.crate_name());
+
+        for &rust_dep in &[&compilation.deps_output] {
+            let mut arg = OsString::from("dependency=");
+            arg.push(rust_dep);
+            p.arg("-L").arg(arg);
+        }
 
-            for native_dep in compilation.native_dirs.iter() {
-                p.arg("-L").arg(native_dep);
-            }
+        for native_dep in compilation.native_dirs.iter() {
+            p.arg("-L").arg(native_dep);
+        }
 
-            for &host_rust_dep in &[&compilation.host_deps_output] {
-                let mut arg = OsString::from("dependency=");
-                arg.push(host_rust_dep);
-                p.arg("-L").arg(arg);
-            }
+        for &host_rust_dep in &[&compilation.host_deps_output] {
+            let mut arg = OsString::from("dependency=");
+            arg.push(host_rust_dep);
+            p.arg("-L").arg(arg);
+        }
 
-            for arg in test_args {
-                p.arg("--test-args").arg(arg);
-            }
+        for arg in test_args {
+            p.arg("--test-args").arg(arg);
+        }
 
-            if let Some(cfgs) = compilation.cfgs.get(package.package_id()) {
-                for cfg in cfgs.iter() {
-                    p.arg("--cfg").arg(cfg);
-                }
+        if let Some(cfgs) = compilation.cfgs.get(package.package_id()) {
+            for cfg in cfgs.iter() {
+                p.arg("--cfg").arg(cfg);
             }
+        }
 
-            let libs = &compilation.libraries[package.package_id()];
-            for &(ref target, ref lib) in libs.iter() {
-                // Note that we can *only* doctest rlib outputs here.  A
-                // staticlib output cannot be linked by the compiler (it just
-                // doesn't do that). A dylib output, however, can be linked by
-                // the compiler, but will always fail. Currently all dylibs are
-                // built as "static dylibs" where the standard library is
-                // statically linked into the dylib. The doc tests fail,
-                // however, for now as they try to link the standard library
-                // dynamically as well, causing problems. As a result we only
-                // pass `--extern` for rlib deps and skip out on all other
-                // artifacts.
-                if lib.extension() != Some(OsStr::new("rlib")) && !target.for_host() {
-                    continue;
-                }
-                let mut arg = OsString::from(target.crate_name());
-                arg.push("=");
-                arg.push(lib);
-                p.arg("--extern").arg(&arg);
+        for &(ref target, ref lib) in deps.iter() {
+            // Note that we can *only* doctest rlib outputs here.  A
+            // staticlib output cannot be linked by the compiler (it just
+            // doesn't do that). A dylib output, however, can be linked by
+            // the compiler, but will always fail. Currently all dylibs are
+            // built as "static dylibs" where the standard library is
+            // statically linked into the dylib. The doc tests fail,
+            // however, for now as they try to link the standard library
+            // dynamically as well, causing problems. As a result we only
+            // pass `--extern` for rlib deps and skip out on all other
+            // artifacts.
+            if lib.extension() != Some(OsStr::new("rlib")) && !target.for_host() {
+                continue;
             }
+            let mut arg = OsString::from(target.crate_name());
+            arg.push("=");
+            arg.push(lib);
+            p.arg("--extern").arg(&arg);
+        }
 
-            if let Some(flags) = compilation.rustdocflags.get(package.package_id()) {
-                p.args(flags);
-            }
+        if let Some(flags) = compilation.rustdocflags.get(package.package_id()) {
+            p.args(flags);
+        }
 
-            config
-                .shell()
-                .verbose(|shell| shell.status("Running", p.to_string()))?;
-            if let Err(e) = p.exec() {
-                let e = e.downcast::<ProcessError>()?;
-                errors.push(e);
-                if !options.no_fail_fast {
-                    return Ok((Test::Doc, errors));
-                }
+        config
+            .shell()
+            .verbose(|shell| shell.status("Running", p.to_string()))?;
+        if let Err(e) = p.exec() {
+            let e = e.downcast::<ProcessError>()?;
+            errors.push(e);
+            if !options.no_fail_fast {
+                return Ok((Test::Doc, errors));
             }
         }
     }

tests/testsuite/build_script.rs

@@ -3044,6 +3044,7 @@ fn panic_abort_with_build_scripts() {
             "src/lib.rs",
             "#[allow(unused_extern_crates)] extern crate a;",
         )
+        .file("build.rs","fn main() {}")
         .file(
             "a/Cargo.toml",
             r#"
@@ -3078,6 +3079,11 @@ fn panic_abort_with_build_scripts() {
         p.cargo("build").arg("-v").arg("--release"),
         execs().with_status(0),
     );
+
+    assert_that(
+        p.cargo("test --release"),
+        execs().with_status(0)
+    );
 }
 
 #[test]