Auto merge of #5651 - ehuss:panic-rustdoc, r=matklad

Fix doctests linking too many libs.

Fixes #5650.  cc #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

@@ -9,10 +9,21 @@ use core::{Feature, Package, PackageId, Target, TargetKind};
 use util::{self, join_paths, process, CargoResult, Config, ProcessBuilder};
 use super::BuildContext;
 
+pub struct Doctest {
+    /// The package being doctested.
+    pub package: Package,
+    /// The target being tested (currently always the package's lib).
+    pub target: Target,
+    /// Extern dependencies needed by `rustdoc`. The path is the location of
+    /// the compiled lib.
+    pub deps: Vec<(Target, PathBuf)>,
+}
+
 /// A structure returning the result of a compilation.
 pub struct Compilation<'cfg> {
     /// A mapping from a package to the list of libraries that need to be
     /// linked when working with that package.
+    // TODO: deprecated, remove
     pub libraries: HashMap<PackageId, HashSet<(Target, PathBuf)>>,
 
     /// An array of all tests created during this compilation.
@@ -50,7 +61,8 @@ 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.
+    pub to_doc_test: Vec<Doctest>,
 
     /// Features per package enabled during this compilation.
     pub cfgs: HashMap<PackageId, HashSet<String>>,

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

@@ -1,5 +1,6 @@
 #![allow(deprecated)]
 use std::collections::{HashMap, HashSet};
+use std::ffi::OsStr;
 use std::fmt::Write;
 use std::path::PathBuf;
 use std::sync::Arc;
@@ -8,6 +9,7 @@ use std::cmp::Ordering;
 use jobserver::Client;
 
 use core::{Package, PackageId, Resolve, Target};
+use core::compiler::compilation;
 use core::profiles::Profile;
 use util::errors::{CargoResult, CargoResultExt};
 use util::{internal, profile, Config, short_hash};
@@ -175,7 +177,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 +229,39 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                     );
             }
 
+            if unit.mode == CompileMode::Doctest {
+                // 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.
+                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()
+                                .filter(|output| {
+                                    output.path.extension() == Some(OsStr::new("rlib"))
+                                        || dep.target.for_host()
+                                })
+                                .map(|output| (dep.target.clone(), output.path.clone())),
+                        );
+                    }
+                }
+                self.compilation.to_doc_test.push(compilation::Doctest {
+                    package: unit.pkg.clone(),
+                    target: unit.target.clone(),
+                    deps: doctest_deps,
+                });
+            }
+
             let feats = self.bcx.resolve.features(unit.pkg.package_id());
             if !feats.is_empty() {
                 self.compilation

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

@@ -199,19 +199,11 @@ fn compute_deps_custom_build<'a, 'cfg>(
     // 1. Compiling the build script itself
     // 2. For each immediate dependency of our package which has a `links`
     //    key, the execution of that build script.
-    let not_custom_build = unit.pkg
-        .targets()
+    let deps = deps
         .iter()
-        .find(|t| !t.is_custom_build())
-        .unwrap();
-    let tmp = Unit {
-        pkg: unit.pkg,
-        target: not_custom_build,
-        profile: unit.profile,
-        kind: unit.kind,
-        mode: CompileMode::Build,
-    };
-    let deps = deps_of(&tmp, bcx, deps, ProfileFor::Any)?;
+        .find(|(key, _deps)| key.pkg == unit.pkg && !key.target.is_custom_build())
+        .expect("can't find package deps")
+        .1;
     Ok(deps.iter()
         .filter_map(|unit| {
             if !unit.target.linkable() || unit.pkg.manifest().links().is_none() {

src/cargo/core/compiler/job_queue.rs

@@ -453,11 +453,15 @@ impl<'a> JobQueue<'a> {
                     }
                 }
             }
-            Fresh if self.counts[key.pkg] == 0 => {
-                self.compiled.insert(key.pkg);
-                config.shell().verbose(|c| c.status("Fresh", key.pkg))?;
+            Fresh => {
+                // If doctest is last, only print "Fresh" if nothing has been printed.
+                if self.counts[key.pkg] == 0
+                    && !(key.mode == CompileMode::Doctest && self.compiled.contains(key.pkg))
+                {
+                    self.compiled.insert(key.pkg);
+                    config.shell().verbose(|c| c.status("Fresh", key.pkg))?;
+                }
             }
-            Fresh => {}
         }
         Ok(())
     }

src/cargo/core/compiler/mod.rs

@@ -24,7 +24,7 @@ use self::output_depinfo::output_depinfo;
 
 pub use self::build_context::{BuildContext, FileFlavor, TargetConfig, TargetInfo};
 pub use self::build_config::{BuildConfig, CompileMode, MessageFormat};
-pub use self::compilation::Compilation;
+pub use self::compilation::{Compilation, Doctest};
 pub use self::context::{Context, Unit};
 pub use self::custom_build::{BuildMap, BuildOutput, BuildScripts};
 pub use self::layout::is_bad_artifact_name;

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

@@ -1,7 +1,7 @@
-use std::ffi::{OsStr, OsString};
+use std::ffi::OsString;
 
 use ops;
-use core::compiler::Compilation;
+use core::compiler::{Compilation, Doctest};
 use util::{self, CargoTestError, ProcessError, Test};
 use util::errors::CargoResult;
 use core::Workspace;
@@ -154,86 +154,64 @@ fn run_doc_tests(
         return Ok((Test::Doc, errors));
     }
 
-    let libs = compilation.to_doc_test.iter().map(|package| {
-        (
+    for doctest_info in &compilation.to_doc_test {
+        let Doctest {
             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);
-            }
+            target,
+            deps,
+        } = doctest_info;
+        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() {
+            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]