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

Add --message-format flag. #3000

Merged
merged 6 commits into from Oct 6, 2016
Merged

Add --message-format flag. #3000

merged 6 commits into from Oct 6, 2016

Conversation

@matklad
Copy link
Member

matklad commented Aug 16, 2016

Closes #2982

This adds a --message-format flag with values human or json-v1 to commands that may trigger compilation.

After the discussion in the issue I am no longer sure that this is a way to go:

  • Looks like it buys nothing compared to RUST_FLAGS approach: a flag is more useful on the command line, but from the tool point of view there should be no significant differences between a flag and an environmental variable.
  • Looks like we really want to wrap compiler's messages into our own json to tie them to particular compilation.
@rust-highfive
Copy link

rust-highfive commented Aug 16, 2016

r? @alexcrichton

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

@matklad matklad force-pushed the matklad:error-format branch from 35245a9 to dc6ee7a Aug 16, 2016
@matklad
Copy link
Member Author

matklad commented Aug 17, 2016

Not sure why appveyor fails: looks like it uses an older nightly, which requires -Z unstable-options.

@alexcrichton
Copy link
Member

alexcrichton commented Aug 17, 2016

Thanks for the PR @matklad! This looks pretty good to me in terms of plumbing.

I think this is important over RUSTFLAGS because Cargo has intrusive knowledge that this flag doesn't change the output artifact (e.g. for using previously built artifacts). Other than that though yeah it looks like it's not buying a huge amount.

cc @rust-lang/tools, what would your expectations here be? If we're emitting JSON, should Cargo emit everything as JSON (e.g. status messages as well). Additionally, should Cargo always wrap the compiler's error messages with its own JSON documents? That way we could attribute what crate's being compiled, what version, etc.

@matklad
Copy link
Member Author

matklad commented Aug 17, 2016

I think this is important over RUSTFLAGS because Cargo has intrusive knowledge that this flag doesn't change the output artifact (e.g. for using previously built artifacts)

Yes, I totally missed this effect, this is a huge argument in favor of custom flag.

If we're emitting JSON, should Cargo emit everything as JSON (e.g. status messages as well). Additionally, should Cargo always wrap the compiler's error messages with its own JSON documents?

Are there any tools which already require this info?

@nrc
Copy link
Member

nrc commented Aug 17, 2016

If we're emitting JSON, should Cargo emit everything as JSON (e.g. status messages as well).

I would like this as a long-term goal, but in the short-term, tools should handle the mix (to be defensive, they'll have to handle a mix in any case).

Additionally, should Cargo always wrap the compiler's error messages with its own JSON documents?

I would like this. Not sure it has to block, but it should certainly happen at some point.

@alexcrichton
Copy link
Member

alexcrichton commented Aug 18, 2016

@matklad

Are there any tools which already require this info?

I don't think so, but it provides us a lot of flexibility where we could just send "status reports" every so often from Cargo for when something is happening.

@nrc

I would like this. Not sure it has to block, but it should certainly happen at some point.

I kind of agree, and I feel like it's best to get it out of the way sooner rather than later. @matklad would you be open to exploring this?

The final product would look similar to this where you'd basically read the output in a streaming fashion line-by-line. Each line of JSON would then be wrapped in our own, and otherwise all other output would be forwarded to the console.

@matklad
Copy link
Member Author

matklad commented Aug 19, 2016

@matklad would you be open to exploring this?

I would prefer to do #3003 first. Doing wrapping also requires some design work: just what exactly is the message format and how do we identify particular compilation?

@alexcrichton
Copy link
Member

alexcrichton commented Aug 19, 2016

I would think we could start out with a simple schema like:

{
    "crate": "crate-name",
    "version": "crate-version",
    "source": "url",
    "message": { ... },
    "reason": "rustc-message"<
}

We could expand that over time, but we should always have all those pieces of information at least

@brson
Copy link
Contributor

brson commented Aug 23, 2016

I agree that it would be best for cargo to wrap rustc's json. But if it's too onerous to do that now it could be designed in a way that tools can detect whether they are seeing rustc-json or cargo-json.

cc @jonathandturner another json error format being proposed.

@bors
Copy link
Contributor

bors commented Aug 31, 2016

The latest upstream changes (presumably #3038) made this pull request unmergeable. Please resolve the merge conflicts.

@matklad matklad force-pushed the matklad:error-format branch from dc6ee7a to c16f8b1 Sep 14, 2016
@matklad
Copy link
Member Author

matklad commented Sep 14, 2016

We could expand that over time, but we should always have all those pieces of information at least

@alexcrichton what do you think about including only PackageId and Target in the output? Source, version, crate name, etc can then be learned by finding the package in the cargo metadata output.

@alexcrichton
Copy link
Member

alexcrichton commented Sep 14, 2016

@matklad sounds good to me!

@matklad
Copy link
Member Author

matklad commented Sep 16, 2016

And one more question: what stream should be used for JSON?

I think that it should be stdout, because it is intended for machines (see also comments by @kamalmarhubi and @kevincox on this issue).

An immediate practical benefit is that the consumers wouldn't need to filter JSON from diagnostic messages.

But rustc outputs JSON to stderr,.. perhaps this is wrong?

@kevincox
Copy link

kevincox commented Sep 16, 2016

I would argue that stdout is the right place. Because this becomes the output and it's intended for machines. Then you "could" put human readable warnings and errors on stderr without breaking the JSON. However in this case that's probably not very useful because the JSON output specifically includes errors and warnings.

@White-Oak
Copy link

White-Oak commented Sep 16, 2016

@matklad I'm pretty sure rustc and cargo output to stderr, and I believe this is semantically right. Also, to 'machines' there is really no difference between reading stderr and stdout.

I am really excited for this PR, but it's a a bit sad it won't be included in the same stable version as new and JSON errors are.

@matklad
Copy link
Member Author

matklad commented Sep 16, 2016

However in this case that's probably not very useful because the JSON output specifically includes errors and warnings.

There are diagnostic messages from the Cargo itself, which are not JSON at the moment.

but it's a a bit sad it won't be included in the same stable version as new and JSON errors are.

Yep, Cargo naturally lags behind rustc for some features because they have to be implemented in the compiler first. However you could use nightly Cargo with stable rustc!

@matklad matklad force-pushed the matklad:error-format branch 2 times, most recently from 74af75d to 4537d66 Sep 17, 2016
bors added a commit that referenced this pull request Sep 26, 2016
Move stream_output to ProcessBuilder

Make `stream_output` method more reusable (I intend to use it in #3000).

Unrelated question: what is that `ExecEngine` thing? Looks like it does nothing at the moment and can be removed.
Copy link
Member

alexcrichton left a comment

Thanks @matklad! Could you also add some documentation on this feature as well?

Also cc @rust-lang/tools and @jonathandturner, JSON errors plumbed through Cargo along with future extensibility so we can add more JSON to the output stream of Cargo.

Right now this is exposed through cargo build --message-format json-v1. The v1 is in theory so we can change the JSON format later, but the compiler doesn't have this, so maybe it's just wishful thinking?

&mut |line| assert!(line.is_empty()),
&mut |line| {
let rustc_message = json::Json::from_str(line).unwrap_or_else(|_| {
panic!("Compiler produced invalid json: `{}`", line)

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Sep 26, 2016

Member

This should probably get translated to a normal error instead of a panic

Json
}

impl MessageFormat {

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Sep 26, 2016

Member

You can probably get away from directly implementing rustc_serialize::Decodable here as that'll thread all the way through docopt I think.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Sep 29, 2016

Member

thoughts on this?

This comment has been minimized.

Copy link
@matklad

matklad Sep 29, 2016

Author Member

Yep, I'll do this when I have time (soonish). I think I'll leave json instead of json-v1. v1 was intended for "let's output compiler messages as is, and wrap them later", but we are already do the streaming.

message: json::Json,
}
process_builder.exec_with_streaming(
&mut |line| assert!(line.is_empty()),

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Sep 26, 2016

Member

(same with below, this'll want to be a normal error not a panic)

let process_builder = rustc.into_process_builder();
try!(if json_errors {
#[derive(RustcEncodable)]
struct Message<'a> {

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Sep 26, 2016

Member

Perhaps this could be defined in a more central location? This'd in theory I guess actually be some sort of enum for the messages that get emitted, and all of them have a reason.

@matklad
Copy link
Member Author

matklad commented Sep 27, 2016

@alexcrichton

What do you think about

what stream should be used for JSON?

@alexcrichton
Copy link
Member

alexcrichton commented Sep 27, 2016

I personally feel like stdout is the best option here, but I don't want to rail up against the compiler and we should follow suit there.

@@ -110,13 +110,13 @@ pub struct ProcessError {
pub desc: String,
pub exit: Option<ExitStatus>,
pub output: Option<Output>,
cause: Option<io::Error>,
cause: Option<Box<Error + Send>>,

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Sep 29, 2016

Member

Perhaps Box<CargoError>?

@matklad
Copy link
Member Author

matklad commented Oct 5, 2016

Yep, will switch to stdout later today.

@matklad matklad force-pushed the matklad:error-format branch from 625d16c to b9ec2b1 Oct 5, 2016

pub fn emit(self) {
let json = json::encode(&self).unwrap();
println!("{}", json);

This comment has been minimized.

Copy link
@matklad

matklad Oct 5, 2016

Author Member

@alexcrichton is this the right way to print JSON to stdout? There is no error reporting, and it bypasses config.shell altogether, but I think both of these are OK: we don't use colors, so we don't need shell, println does necessary synchronization, and it is probably OK to panic if we fail to write to stdout.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Oct 5, 2016

Member

Yeah this is fine!

@alexcrichton
Copy link
Member

alexcrichton commented Oct 6, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 6, 2016

📌 Commit b9ec2b1 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Oct 6, 2016

Testing commit b9ec2b1 with merge 964afba...

bors added a commit that referenced this pull request Oct 6, 2016
Add --message-format flag.

Closes #2982

This adds a `--message-format` flag with values `human` or `json-v1` to commands that may trigger compilation.

After the discussion in the issue I am no longer sure that this is a way to go:

* Looks like it buys nothing compared to `RUST_FLAGS` approach: a flag is more useful on the command line, but from the tool point of view there should be no significant differences between a flag and an environmental variable.

* Looks like we really want to wrap compiler's messages into our own json to tie them to particular compilation.
@alexcrichton
Copy link
Member

alexcrichton commented Oct 6, 2016

@bors: retry force clean

@bors
Copy link
Contributor

bors commented Oct 6, 2016

Testing commit b9ec2b1 with merge c9c8a6e...

bors added a commit that referenced this pull request Oct 6, 2016
Add --message-format flag.

Closes #2982

This adds a `--message-format` flag with values `human` or `json-v1` to commands that may trigger compilation.

After the discussion in the issue I am no longer sure that this is a way to go:

* Looks like it buys nothing compared to `RUST_FLAGS` approach: a flag is more useful on the command line, but from the tool point of view there should be no significant differences between a flag and an environmental variable.

* Looks like we really want to wrap compiler's messages into our own json to tie them to particular compilation.
@alexcrichton
Copy link
Member

alexcrichton commented Oct 6, 2016

@bors: retry force clean

@bors
Copy link
Contributor

bors commented Oct 6, 2016

Testing commit b9ec2b1 with merge 0873893...

bors added a commit that referenced this pull request Oct 6, 2016
Add --message-format flag.

Closes #2982

This adds a `--message-format` flag with values `human` or `json-v1` to commands that may trigger compilation.

After the discussion in the issue I am no longer sure that this is a way to go:

* Looks like it buys nothing compared to `RUST_FLAGS` approach: a flag is more useful on the command line, but from the tool point of view there should be no significant differences between a flag and an environmental variable.

* Looks like we really want to wrap compiler's messages into our own json to tie them to particular compilation.
@bors
Copy link
Contributor

bors commented Oct 6, 2016

@bors bors merged commit b9ec2b1 into rust-lang:master Oct 6, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@jplatte
Copy link
Contributor

jplatte commented Oct 10, 2016

Is there an ETA for the next release that will include this feature?

@alexcrichton
Copy link
Member

alexcrichton commented Oct 10, 2016

@jplatte this'll be released with Rust 1.14 on 2016-12-22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.