From 00d325db0fbcf407f1d4f6ba2ff5e5ee34adf9b7 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 25 Jun 2018 18:00:04 -0700 Subject: [PATCH 1/5] 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] From 33a5f7cc1727e209108dc2ec4aac2f6cb0cabe3c Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 26 Jun 2018 18:10:05 -0700 Subject: [PATCH 2/5] Don't pollute the deps_map. In rare cases, the profile_for is set incorrectly for this temporary unit used to compute dependencies. However, it's not needed since the RunCustomBuild Unit's parent is already in the map. --- .../core/compiler/context/unit_dependencies.rs | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/cargo/core/compiler/context/unit_dependencies.rs b/src/cargo/core/compiler/context/unit_dependencies.rs index 8c87d0a53e5..f522f3839a9 100644 --- a/src/cargo/core/compiler/context/unit_dependencies.rs +++ b/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() { From 849220e8281bbba03a4b99967d8fead2fcbfb955 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 26 Jun 2018 18:10:49 -0700 Subject: [PATCH 3/5] Avoid randomly printing "Fresh" for the doc test unit. --- src/cargo/core/compiler/job_queue.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index de633e7af37..807b925630e 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -410,6 +410,8 @@ impl<'a> JobQueue<'a> { ) -> CargoResult<()> { if (self.compiled.contains(key.pkg) && !key.mode.is_doc()) || (self.documented.contains(key.pkg) && key.mode.is_doc()) + // Skip doctest, it is a dummy entry that is always fresh. + || key.mode == CompileMode::Doctest { return Ok(()); } @@ -419,11 +421,8 @@ impl<'a> JobQueue<'a> { // being a compiled package Dirty => { if key.mode.is_doc() { - // Skip Doctest - if !key.mode.is_any_test() { - self.documented.insert(key.pkg); - config.shell().status("Documenting", key.pkg)?; - } + self.documented.insert(key.pkg); + config.shell().status("Documenting", key.pkg)?; } else { self.compiled.insert(key.pkg); if key.mode.is_check() { From 812fba62ff3b173ec129cf9a25420820c0c7971d Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 26 Jun 2018 19:29:08 -0700 Subject: [PATCH 4/5] Doctest cleanup. - Consolidate logic for determining doctest dependencies. - Use a struct for the doctest info in `Compilation`. --- src/cargo/core/compiler/compilation.rs | 14 ++++++++++++-- src/cargo/core/compiler/context/mod.rs | 26 +++++++++++++++++++++----- src/cargo/core/compiler/mod.rs | 2 +- src/cargo/ops/cargo_test.rs | 24 ++++++++---------------- 4 files changed, 42 insertions(+), 24 deletions(-) diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 2d9b1676b50..28b2a6b17f2 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/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>, /// An array of all tests created during this compilation. @@ -51,8 +62,7 @@ pub struct Compilation<'cfg> { pub extra_env: HashMap>, /// Libraries to test with rustdoc. - /// The third value is the list of dependencies. - pub to_doc_test: Vec<(Package, Target, Vec<(Target, PathBuf)>)>, + pub to_doc_test: Vec, /// 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 8171c3928a8..0b5c77d2277 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/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}; @@ -228,6 +230,16 @@ 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 { @@ -235,15 +247,19 @@ impl<'a, 'cfg> Context<'a, 'cfg> { 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(( - unit.pkg.clone(), - unit.target.clone(), - doctest_deps, - )); + 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()); diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 2dff328de0b..2e148ef22f2 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/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; diff --git a/src/cargo/ops/cargo_test.rs b/src/cargo/ops/cargo_test.rs index 06fb07c5938..1ed480fc0b4 100644 --- a/src/cargo/ops/cargo_test.rs +++ b/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,7 +154,12 @@ fn run_doc_tests( return Ok((Test::Doc, errors)); } - for (package, target, deps) in &compilation.to_doc_test { + for doctest_info in &compilation.to_doc_test { + let Doctest { + package, + target, + deps, + } = doctest_info; config.shell().status("Doc-tests", target.name())?; let mut p = compilation.rustdoc_process(package)?; p.arg("--test") @@ -189,19 +194,6 @@ fn run_doc_tests( } 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); From 478d1ce29fd8ffaf0e6e623e758cfdfc8ee33a8a Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 27 Jun 2018 17:38:14 -0700 Subject: [PATCH 5/5] Fix "Fresh" display with doctest units. --- src/cargo/core/compiler/job_queue.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index 807b925630e..f7612e3cedf 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -410,8 +410,6 @@ impl<'a> JobQueue<'a> { ) -> CargoResult<()> { if (self.compiled.contains(key.pkg) && !key.mode.is_doc()) || (self.documented.contains(key.pkg) && key.mode.is_doc()) - // Skip doctest, it is a dummy entry that is always fresh. - || key.mode == CompileMode::Doctest { return Ok(()); } @@ -421,8 +419,11 @@ impl<'a> JobQueue<'a> { // being a compiled package Dirty => { if key.mode.is_doc() { - self.documented.insert(key.pkg); - config.shell().status("Documenting", key.pkg)?; + // Skip Doctest + if !key.mode.is_any_test() { + self.documented.insert(key.pkg); + config.shell().status("Documenting", key.pkg)?; + } } else { self.compiled.insert(key.pkg); if key.mode.is_check() { @@ -432,11 +433,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(()) }