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.

Sign up for GitHub

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

Add support for documenting all members of the workspace with "doc --all" #3515

Merged
merged 4 commits into from
Jan 9, 2017
Merged

Add support for documenting all members of the workspace with "doc --all" #3515

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
067eb28
Add support for documenting all members of the workspace with "doc --…
sdroege Jan 6, 2017
01e930f
Add tests for "doc --all"
sdroege Jan 8, 2017
9576b31
Check if the "doc --all" tests exit with exit code 0
sdroege Jan 9, 2017
b850c25
Collect library/binary names when documenting even if a spec was given
sdroege Jan 9, 2017
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
13 changes: 12 additions & 1 deletion src/bin/doc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ pub struct Options {
flag_bin: Vec<String>,
flag_frozen: bool,
flag_locked: bool,
flag_all: bool,
}

pub const USAGE: &'static str = "
Expand All @@ -35,6 +36,7 @@ Options:
-h, --help Print this message
--open Opens the docs in a browser after the operation
-p SPEC, --package SPEC ... Package to document
--all Document all packages in the workspace
--no-deps Don't build documentation for dependencies
-j N, --jobs N Number of parallel jobs, defaults to # of CPUs
--lib Document only this package's library
Expand All @@ -55,6 +57,9 @@ Options:
By default the documentation for the local package and all dependencies is
built. The output is all placed in `target/doc` in rustdoc's usual format.

All packages in the workspace are documented if the `--all` flag is supplied. The
`--all` flag may be supplied in the presence of a virtual manifest.

If the --package argument is given, then SPEC is a package id specification
which indicates which package should be documented. If it is not given, then the
current package is documented. For more information on SPEC and its format, see
Expand All @@ -70,6 +75,12 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<()>> {

let root = find_root_manifest_for_wd(options.flag_manifest_path, config.cwd())?;

let spec = if options.flag_all {
Packages::All
} else {
Packages::Packages(&options.flag_package)
};

let empty = Vec::new();
let doc_opts = ops::DocOptions {
open_result: options.flag_open,
Expand All @@ -80,7 +91,7 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<()>> {
features: &options.flag_features,
all_features: options.flag_all_features,
no_default_features: options.flag_no_default_features,
spec: Packages::Packages(&options.flag_package),
spec: spec,
filter: ops::CompileFilter::new(options.flag_lib,
&options.flag_bin,
&empty,
Expand Down
60 changes: 34 additions & 26 deletions src/cargo/ops/cargo_doc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::fs;
use std::path::Path;
use std::process::Command;

use core::{PackageIdSpec, Workspace};
use core::Workspace;
use ops;
use util::CargoResult;

Expand All @@ -13,45 +13,53 @@ pub struct DocOptions<'a> {
}

pub fn doc(ws: &Workspace, options: &DocOptions) -> CargoResult<()> {
let package = ws.current()?;
let specs = options.compile_opts.spec.into_package_id_specs(ws)?;
let resolve = ops::resolve_ws_precisely(ws,
None,
options.compile_opts.features,
options.compile_opts.all_features,
options.compile_opts.no_default_features,
&specs)?;
let (packages, resolve_with_overrides) = resolve;

let spec = match options.compile_opts.spec {
ops::Packages::Packages(packages) => packages,
_ => {
// This should not happen, because the `doc` binary is hard-coded to pass
// the `Packages::Packages` variant.
bail!("`cargo doc` does not support the `--all` flag")
},
let mut pkgs = Vec::new();
if specs.len() > 0 {
for p in specs.iter() {
pkgs.push(packages.get(p.query(resolve_with_overrides.iter())?)?);
}
} else {
let root_package = ws.current()?;
pkgs.push(root_package);
};

let mut lib_names = HashSet::new();
let mut bin_names = HashSet::new();
if spec.is_empty() {
for target in package.targets().iter().filter(|t| t.documented()) {
if target.is_lib() {
assert!(lib_names.insert(target.crate_name()));
} else {
assert!(bin_names.insert(target.crate_name()));
if specs.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

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

I think this may actually work best if we remove this condition. I don't quite recall why it exists, but I think it's best if we execute the below logic for all packages, not just the root package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I only kept it because I thought someone probably had a reason for having done that and I just don't understand the reason :)

for package in &pkgs {
for target in package.targets().iter().filter(|t| t.documented()) {
if target.is_lib() {
assert!(lib_names.insert(target.crate_name()));
} else {
assert!(bin_names.insert(target.crate_name()));
}
}
}
for bin in bin_names.iter() {
if lib_names.contains(bin) {
bail!("cannot document a package where a library and a binary \
have the same name. Consider renaming one or marking \
the target as `doc = false`")
for bin in bin_names.iter() {
if lib_names.contains(bin) {
bail!("cannot document a package where a library and a binary \
have the same name. Consider renaming one or marking \
the target as `doc = false`")
}
}
}
}

ops::compile(ws, &options.compile_opts)?;

if options.open_result {
let name = if spec.len() > 1 {
let name = if pkgs.len() > 1 {
bail!("Passing multiple packages and `open` is not supported")
} else if spec.len() == 1 {
PackageIdSpec::parse(&spec[0])?
.name()
.replace("-", "_")
} else if pkgs.len() == 1 {
pkgs[0].name().replace("-", "_")
} else {
match lib_names.iter().chain(bin_names.iter()).nth(0) {
Some(s) => s.to_string(),
Expand Down
95 changes: 95 additions & 0 deletions tests/doc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::fs;

use cargotest::{is_nightly, rustc_host};
use cargotest::support::{project, execs, path2url};
use cargotest::support::registry::Package;
use hamcrest::{assert_that, existing_file, existing_dir, is_not};

#[test]
Expand Down Expand Up @@ -612,3 +613,97 @@ fn plugins_no_use_target() {
.arg("-v"),
execs().with_status(0));
}

#[test]
fn doc_all_workspace() {
let p = project("foo")
.file("Cargo.toml", r#"
[project]
name = "foo"
version = "0.1.0"

[dependencies]
bar = { path = "bar" }

[workspace]
"#)
.file("src/main.rs", r#"
fn main() {}
"#)
.file("bar/Cargo.toml", r#"
[project]
name = "bar"
version = "0.1.0"
"#)
.file("bar/src/lib.rs", r#"
pub fn bar() {}
"#);
p.build();

// The order in which bar is compiled or documented is not deterministic
assert_that(p.cargo_process("doc")
.arg("--all"),
execs().with_stderr_contains("[..] Documenting bar v0.1.0 ([..])")
.with_stderr_contains("[..] Compiling bar v0.1.0 ([..])")
.with_stderr_contains("[..] Documenting foo v0.1.0 ([..])"));
Copy link
Member

Choose a reason for hiding this comment

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

Could you add .with_status(0) assertions to these as well to ensure we test that the process ran successfully?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but shouldn't this also be added to the "build --all" test? I'll send another pull request for the change of the other test then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it would be "nice" but not necessary, as for building the test checks for the "finished successfully" line in the output

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah it's true I should have caught it on that PR! Want to send another PR for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will send something for that tomorrow :)

}

#[test]
fn doc_all_virtual_manifest() {
let p = project("workspace")
.file("Cargo.toml", r#"
[workspace]
members = ["foo", "bar"]
"#)
.file("foo/Cargo.toml", r#"
[project]
name = "foo"
version = "0.1.0"
"#)
.file("foo/src/lib.rs", r#"
pub fn foo() {}
"#)
.file("bar/Cargo.toml", r#"
[project]
name = "bar"
version = "0.1.0"
"#)
.file("bar/src/lib.rs", r#"
pub fn bar() {}
"#);
p.build();

// The order in which foo and bar are documented is not guaranteed
assert_that(p.cargo_process("doc")
.arg("--all"),
execs().with_stderr_contains("[..] Documenting bar v0.1.0 ([..])")
.with_stderr_contains("[..] Documenting foo v0.1.0 ([..])"));
}

#[test]
fn doc_all_member_dependency_same_name() {
let p = project("workspace")
.file("Cargo.toml", r#"
[workspace]
members = ["a"]
"#)
.file("a/Cargo.toml", r#"
[project]
name = "a"
version = "0.1.0"

[dependencies]
a = "0.1.0"
"#)
.file("a/src/lib.rs", r#"
pub fn a() {}
"#);
p.build();

Package::new("a", "0.1.0").publish();

assert_that(p.cargo_process("doc")
.arg("--all"),
execs().with_stderr_contains("[..] Updating registry `[..]`")
.with_stderr_contains("[..] Documenting a v0.1.0 ([..])"));
}