Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix doctests linking too many libs. #5651

Merged
merged 5 commits into from
Jul 5, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/cargo/core/compiler/compilation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)>)>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about changing this triple to a struct, so as to make comment not-necessary? It might also be worthwhile to move the

if lib.extension() != Some(OsStr::new("rlib")) && !target.for_host() {

logic to the place where we generate these dependencies?


/// Features per package enabled during this compilation.
pub cfgs: HashMap<PackageId, HashSet<String>>,
Expand Down
21 changes: 20 additions & 1 deletion src/cargo/core/compiler/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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
Expand Down
18 changes: 4 additions & 14 deletions src/cargo/ops/cargo_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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);
}

Expand Down Expand Up @@ -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));
}
}
}
Expand Down Expand Up @@ -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),
}
}

Expand Down
128 changes: 57 additions & 71 deletions src/cargo/ops/cargo_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions tests/testsuite/build_script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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#"
Expand Down Expand Up @@ -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]
Expand Down