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

Support JSON with rustdoc. #5878

Merged
merged 2 commits into from
Aug 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
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
80 changes: 49 additions & 31 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,36 +283,10 @@ fn rustc<'a, 'cfg>(
&package_id,
&target,
mode,
&mut |line| {
if !line.is_empty() {
Err(internal(&format!(
"compiler stdout is not empty: `{}`",
line
)))
} else {
Ok(())
}
},
&mut |line| {
// stderr from rustc can have a mix of JSON and non-JSON output
if line.starts_with('{') {
// Handle JSON lines
let compiler_message = serde_json::from_str(line).map_err(|_| {
internal(&format!("compiler produced invalid json: `{}`", line))
})?;

machine_message::emit(&machine_message::FromCompiler {
package_id: &package_id,
target: &target,
message: compiler_message,
});
} else {
// Forward non-JSON to stderr
writeln!(io::stderr(), "{}", line)?;
}
Ok(())
},
).map_err(Internal::new).chain_err(|| format!("Could not compile `{}`.", name))?;
&mut assert_is_empty,
&mut |line| json_stderr(line, &package_id, &target),
).map_err(Internal::new)
.chain_err(|| format!("Could not compile `{}`.", name))?;
} else if build_plan {
state.build_plan(buildkey, rustc.clone(), outputs.clone());
} else {
Expand Down Expand Up @@ -631,6 +605,10 @@ fn rustdoc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult
rustdoc.arg("--cfg").arg(&format!("feature=\"{}\"", feat));
}

if bcx.build_config.json_messages() {
rustdoc.arg("--error-format").arg("json");
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to feature-detect support for json here, right? The user requests --message-format=json explicitly, so they'll see an error and that is expected behavior.

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 don't understand the question. This is the code that translates cargo's --message-format flag to rustdoc's --error-format flag.

Copy link
Member

Choose a reason for hiding this comment

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

--error-format is a recent addition to rustdoc, so, in theory, using new cargo and old rustdoc could result in a

error: the `-Z unstable-options` flag must also be passed to enable the flag `error-format`

message. However, that should be fine, because the user has to ask for cargo doc --message-format=json explicitly. That is, it's not a flag that we add automatically for existing workflows, so we don't have to feature-detect support for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I understand now. Yea, my understanding is that the version of cargo is tightly wedded to rustc and rustdoc. I think some distributions have attempted to split them, but afaik cargo doesn't really support that use case, right? At least in this case, it wouldn't break the normal usage.

Copy link
Member

Choose a reason for hiding this comment

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

It is at least somewhat supported: you can override compiler & rustdoc with env vars, and we try account for different compilers in Rustc cache. I think the current approach is "don't break things without necessity", and I don't remember any actual CLI changes that could have caused breakage.

}

if let Some(ref args) = bcx.extra_args_for(unit) {
rustdoc.args(args);
}
Expand All @@ -642,6 +620,9 @@ fn rustdoc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult
let name = unit.pkg.name().to_string();
let build_state = cx.build_state.clone();
let key = (unit.pkg.package_id().clone(), unit.kind);
let json_messages = bcx.build_config.json_messages();
let package_id = unit.pkg.package_id().clone();
let target = unit.target.clone();

let should_capture_output = cx.bcx.config.cli_unstable().compile_progress;

Expand All @@ -656,7 +637,14 @@ fn rustdoc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult
}
state.running(&rustdoc);

let exec_result = if should_capture_output {
let exec_result = if json_messages {
rustdoc
.exec_with_streaming(
&mut assert_is_empty,
&mut |line| json_stderr(line, &package_id, &target),
false,
).map(drop)
} else if should_capture_output {
state.capture_output(rustdoc, false).map(drop)
} else {
rustdoc.exec()
Expand Down Expand Up @@ -999,3 +987,33 @@ impl Kind {
}
}
}

fn assert_is_empty(line: &str) -> CargoResult<()> {
if !line.is_empty() {
Err(internal(&format!(
"compiler stdout is not empty: `{}`",
line
)))
} else {
Ok(())
}
}

fn json_stderr(line: &str, package_id: &PackageId, target: &Target) -> CargoResult<()> {
// stderr from rustc/rustdoc can have a mix of JSON and non-JSON output
Copy link
Member

Choose a reason for hiding this comment

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

Hm, does rustdoc put JSON on stderr as well? It's a shame that Cargo and Rustc use different streams for JSON =/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is an odd inconsistency. There's an open issue for that (rust-lang/rust#48301). I imagine since the vast majority of people use rustc behind cargo it doesn't really come up often.

if line.starts_with('{') {
// Handle JSON lines
let compiler_message = serde_json::from_str(line)
.map_err(|_| internal(&format!("compiler produced invalid json: `{}`", line)))?;

machine_message::emit(&machine_message::FromCompiler {
package_id: package_id,
target: target,
message: compiler_message,
});
} else {
// Forward non-JSON to stderr
writeln!(io::stderr(), "{}", line)?;
}
Ok(())
}
47 changes: 38 additions & 9 deletions tests/testsuite/doc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1359,22 +1359,22 @@ fn doc_private_items() {
assert_that(&foo.root().join("target/doc/foo/private/index.html"), existing_file());
}

const BAD_INTRA_LINK_LIB: &'static str = r#"
#![deny(intra_doc_link_resolution_failure)]

/// [bad_link]
pub fn foo() {}
"#;

#[test]
fn doc_cap_lints() {
if !is_nightly() {
// This can be removed once 1.29 is stable (rustdoc --cap-lints).
return;
}
let a = git::new("a", |p| {
p.file("Cargo.toml", &basic_lib_manifest("a")).file(
"src/lib.rs",
"
#![deny(intra_doc_link_resolution_failure)]

/// [bad_link]
pub fn foo() {}
",
)
p.file("Cargo.toml", &basic_lib_manifest("a"))
.file("src/lib.rs", BAD_INTRA_LINK_LIB)
}).unwrap();

let p = project()
Expand Down Expand Up @@ -1419,5 +1419,34 @@ fn doc_cap_lints() {
",
),
);
}

#[test]
fn doc_message_format() {
if !is_nightly() {
// This can be removed once 1.30 is stable (rustdoc --error-format stabilized).
return;
}
let p = project().file("src/lib.rs", BAD_INTRA_LINK_LIB).build();

assert_that(
p.cargo("doc --message-format=json"),
execs().with_status(101).with_json(
r#"
{
"message": {
"children": "{...}",
"code": "{...}",
"level": "error",
"message": "[..]",
"rendered": "[..]",
"spans": "{...}"
},
"package_id": "foo [..]",
"reason": "compiler-message",
"target": "{...}"
}
"#,
),
);
}