Support JSON with rustdoc. by ehuss · Pull Request #5878 · rust-lang/cargo

ehuss · 2018-08-09T06:54:02Z

This allows cargo doc --message-format=json to actually work.

Note that this explicitly does not attempt to support it for doctests for
several reasons:

rustdoc --test --error-format=json does not work for some reason.
Since the lib is usually compiled before running rustdoc, warnings/errors
will be emitted correctly by rustc.
I'm unaware of any errors/warnings rustdoc --test is capable of producing
assuming the code passed rustc.
The compilation of the tests themselves do not support JSON.
libtest does not output json, so it's utility is limited.

rust-highfive · 2018-08-09T06:54:05Z

(rust_highfive has picked a reviewer for you, use r? to override)

This allows `cargo doc --message-format=json` to actually work. Note that this explicitly does not attempt to support it for doctests for several reasons: - `rustdoc --test --error-format=json` does not work for some reason. - Since the lib is usually compiled before running rustdoc, warnings/errors will be emitted correctly by rustc. - I'm unaware of any errors/warnings `rustdoc --test` is capable of producing assuming the code passed `rustc`. - The compilation of the tests themselves do not support JSON. - libtest does not output json, so it's utility is limited.

ehuss · 2018-08-09T07:00:27Z

cc @GuillaumeGomez I assume this is OK now that --error-format is stabilized?

GuillaumeGomez · 2018-08-09T08:01:03Z

Indeed!

matklad

❤️

matklad · 2018-08-09T21:36:01Z

src/cargo/core/compiler/mod.rs

+                    &mut json_stdout,
+                    &mut |line| json_stderr(line, &package_id, &target),
+                    false,
+                ).map(drop)


map(drop) probably should not be here, it's just ignore the error. I wonder if the one on the next line should be removed as well?

Yea, it's a little weird. The intent is so that all branches are CargoResult<()> so they all have the same type so that a common error can be attached. I can't think of a more elegant way to do that.

Oh, I've misunderstood the intention of the code completely...

Yep, the only better way to write this would probably be a catch block, which is currently unstable.

You could go the other way and change rustdoc.exec() to rustdoc.exec_with_output(), so all branches type-check to CargoResult<Output>.

That'll unfortunately hide rustdoc's output from the user, b/c it would be captured.

matklad · 2018-08-09T21:37:36Z

src/cargo/core/compiler/mod.rs

    }
 }
+
+fn json_stdout(line: &str) -> CargoResult<()> {


Let's rename this to assert_is_empty? EDIT: or smthing similar, so it's clear on the call site what actually happens with json on stdout :)

Also, are we sure that rustdoc's stdout is clear?

I'll investigate if it is possible for it to print to stdout.

matklad · 2018-08-09T21:39:32Z

src/cargo/core/compiler/mod.rs

    }

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


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.

I don't understand the question. This is the code that translates cargo's --message-format flag to rustdoc's --error-format flag.

--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.

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.

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.

matklad · 2018-08-09T21:41:09Z

tests/testsuite/doc.rs

+        // This can be removed once 1.30 is stable (rustdoc --error-format stabilized).
+        return;
+    }
+    let p = project().file("src/lib.rs", "asdf").build();


Hm, let's use a code that is valid, but has docstring errors? That way, we'll be more sure that we are checking rustdoc output specifically.

Sure, I was on the fence with that. My concern is that the rustdoc messages might be a little unstable, but there is another test already doing that.

matklad · 2018-08-09T21:43:16Z

src/cargo/core/compiler/mod.rs

+}
+
+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


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

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.

matklad · 2018-08-09T22:49:08Z

LGTM!

Let's also see what @alexcrichton thinks about this, because formally this extend "public API" of Cargo.

alexcrichton · 2018-08-10T03:14:30Z

@bors: r+

Thanks for this @ehuss!

I agree that we do not need to do feature detection here, while Cargo supports mismatched rustc it's like how Rustc supports other versions of LLVM. It sort of works but not if you want everything to work.

bors · 2018-08-10T03:14:31Z

📌 Commit c28823f has been approved by alexcrichton

bors · 2018-08-10T03:14:38Z

⌛ Testing commit c28823f with merge 578e253...

Support JSON with rustdoc. This allows `cargo doc --message-format=json` to actually work. Note that this explicitly does not attempt to support it for doctests for several reasons: - `rustdoc --test --error-format=json` does not work for some reason. - Since the lib is usually compiled before running rustdoc, warnings/errors will be emitted correctly by rustc. - I'm unaware of any errors/warnings `rustdoc --test` is capable of producing assuming the code passed `rustc`. - The compilation of the tests themselves do not support JSON. - libtest does not output json, so it's utility is limited.

bors · 2018-08-10T03:54:49Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 578e253 to master...

Update cargo - Update transitioning url (rust-lang/cargo#5889) - Resolve some clippy lint warnings (rust-lang/cargo#5884) - Don't kill child processes on normal exit on Windows (rust-lang/cargo#5887) - fix a bunch of clippy warnings (rust-lang/cargo#5876) - Add support for rustc's --error-format short (rust-lang/cargo#5879) - Support JSON with rustdoc. (rust-lang/cargo#5878) - Fix rustfmt instructions in CONTRIBUTING.md (rust-lang/cargo#5880) - Allow `cargo run` in workspaces. (rust-lang/cargo#5877) - Change target filters in workspaces. (rust-lang/cargo#5873) - Improve verbose console and log for finding git repo in package check (rust-lang/cargo#5858) - Meta rename (rust-lang/cargo#5871) - fetch: skip target tests when cross_compile is disabled (rust-lang/cargo#5870) - Fully capture rustc and rustdoc output when -Zcompile-progress is passed (rust-lang/cargo#5862) - Fix test --example docs. (rust-lang/cargo#5867) - Add a feature to build a vendored OpenSSL (rust-lang/cargo#5865)

rust-highfive assigned matklad Aug 9, 2018

ehuss force-pushed the rustdoc-json branch from f267cf6 to f3faa97 Compare August 9, 2018 06:59

matklad reviewed Aug 9, 2018

View reviewed changes

Update for review comments.

c28823f

bors merged commit c28823f into rust-lang:master Aug 10, 2018

ehuss mentioned this pull request Aug 14, 2018

Update cargo rust-lang/rust#53367

Merged

ehuss added this to the 1.30.0 milestone Feb 6, 2022

Conversation

ehuss commented Aug 9, 2018

Uh oh!

rust-highfive commented Aug 9, 2018

Uh oh!

ehuss commented Aug 9, 2018

Uh oh!

GuillaumeGomez commented Aug 9, 2018

Uh oh!

matklad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matklad Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matklad commented Aug 9, 2018

Uh oh!

alexcrichton commented Aug 10, 2018

Uh oh!

bors commented Aug 10, 2018

Uh oh!

bors commented Aug 10, 2018

Uh oh!

bors commented Aug 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

matklad Aug 9, 2018 •

edited

Loading