From 00d325db0fbcf407f1d4f6ba2ff5e5ee34adf9b7 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 25 Jun 2018 18:00:04 -0700 Subject: [PATCH] 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 | 4 +- src/cargo/core/compiler/context/mod.rs | 21 +++- src/cargo/ops/cargo_compile.rs | 18 +--- src/cargo/ops/cargo_test.rs | 128 +++++++++++-------------- tests/testsuite/build_script.rs | 6 ++ 5 files changed, 90 insertions(+), 87 deletions(-) diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 1231de815ef..2d9b1676b50 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/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>, - pub to_doc_test: Vec, + /// 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>, diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index 0000b895057..8171c3928a8 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/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 diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 5e60111e8b6..dc16c9dbf04 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/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::>(); 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), } } diff --git a/src/cargo/ops/cargo_test.rs b/src/cargo/ops/cargo_test.rs index c65cc1970d4..06fb07c5938 100644 --- a/src/cargo/ops/cargo_test.rs +++ b/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::()?; - 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::()?; + errors.push(e); + if !options.no_fail_fast { + return Ok((Test::Doc, errors)); } } } diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs index a5e32f5023f..1754123364e 100644 --- a/tests/testsuite/build_script.rs +++ b/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]