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 support for --error-format now that it has been stablized #2982

Closed
trixnz opened this issue Aug 10, 2016 · 16 comments
Closed

Add support for --error-format now that it has been stablized #2982

trixnz opened this issue Aug 10, 2016 · 16 comments

Comments

@trixnz
Copy link

trixnz commented Aug 10, 2016

It would be great if Cargo could add support for --error-format now that it has been stabilized and merged into rust-master.

This would be a big win for IDEs as we could move away from using RUSTFLAGS.

@alexcrichton
Copy link
Member

cc @jonathandturner

@alexcrichton
Copy link
Member

cc @matklad

@matklad
Copy link
Member

matklad commented Aug 11, 2016

One question is how to untangle Cargo output from the compiler's output. That is, if we simply add --error-format to the flags, we'll get an stderr like

   Compiling crossbeam v0.1.6
   Compiling rand v0.3.14
   Compiling memchr v0.1.10
   Compiling kernel32-sys v0.2.1
{"message":"transmute called with differently sized types: [usize; 32] (2048 bits) to [std::sync::atomic::AtomicBool; 32] (256 bits)","code":{"code":"E0512","explanation":"\nTransmute with two differently sized types was attempted. Erroneous code\nexample:\n\n```compile_fail,E0512\nfn takes_u8(_: u8) {}\n\nfn main() {\n    unsafe { takes_u8(::std::mem::transmute(0u16)); }\n    // error: transmute called with differently sized types\n}\n```\n\nPlease use types with same size or use the expected type directly. Example:\n\n```\nfn takes_u8(_: u8) {}\n\nfn main() {\n    unsafe { takes_u8(::std::mem::transmute(0i8)); } // ok!\n    // or:\n    unsafe { takes_u8(0u8); } // ok!\n}\n```\n"},"level":"error","spans":[{"file_name":"/home/matklad/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/crossbeam-0.1.6/src/sync/seg_queue.rs","byte_start":45628,"byte_end":45642,"line_start":34,"line_end":34,"column_start":29,"column_end":43,"is_primary":true,"text":[{"text":"            ready: unsafe { mem::transmute([0usize; SEG_SIZE]) },","highlight_start":29,"highlight_end":43}],"label":null,"suggested_replacement":null,"expansion":null}],"children":[],"rendered":null}
{"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":null}
Build failed, waiting for other jobs to finish...
error: Could not compile `crossbeam`.

To learn more, run the command again with --verbose.

I see three options:

  1. Output Cargo messages in the JSON per line format as well and get rid of human oriented parts.
  2. Don't do anything special and let the tool to decide on line by line basis if it is a compiler's JSON or a Cargo's status message.
  3. Silence all Cargo output.

I'd suggest to go with the option 2: it can be evolved into the 1 later and, even if 1 seems to be the correct approach, it may turn out that we don't actually need it.

@trixnz
Copy link
Author

trixnz commented Aug 11, 2016

Option 2 is how we do it in RustyCode right now. If the line doesn't begin with { then it is ignored as a JSON candidate. Ideally option 1 is best, but that would require a significant investment I imagine.

👍 for option 2

@matklad
Copy link
Member

matklad commented Aug 11, 2016

In the case of cargo run there may be some output from the program itself. Should --error-format be used with cargo run at all?

@alexcrichton
Copy link
Member

I think that 2 is a fine choice to take for now, it's certainly the most conservative. That being said, though, I'd also like to make Cargo as amenable to external tools as possible, so I wouldn't mind Cargo also printing out json status updates for itself.

I'd personally expect --error-format to be available on all cargo subcommands that could trigger compilations (e.g. those with -j as well). We may also want to consider hooking it up to a more general flag like --output-format which controls all of Cargo's output, not just the errors of the compiler itself.

cc @rust-lang/tools

@nrc
Copy link
Member

nrc commented Aug 11, 2016

rustw also basically does option 2, but I'd love for Cargo to output JSON as well as the compiler

@brson
Copy link
Contributor

brson commented Aug 12, 2016

It looks to me like there is no way to distinguish which crate the JSON corresponds to. Is that true?

@brson
Copy link
Contributor

brson commented Aug 12, 2016

I might expect Cargo itself to have a JSON output format that is a superset of the rustc JSON output. (Edit: I see @nrc said the same thing).

@nrc
Copy link
Member

nrc commented Aug 12, 2016

Yes, I think Cargo would need its own JSON format that wraps Rustc's.

@alexcrichton
Copy link
Member

Note that Cargo would have a very difficult time capturing the output of the compiler and then adding its own json. It could, but it wouldn't be an easy thing to do.

@nrc
Copy link
Member

nrc commented Aug 12, 2016

@alexcrichton why? Is it not just redirecting stdout?

@matklad
Copy link
Member

matklad commented Aug 12, 2016

It looks to me like there is no way to distinguish which crate the JSON corresponds to. Is that true?

Yes, looks like it is indeed impossible to trace error messages back to target and package. You can get an approximate answer by inspecting the path to the file, but this will break at least when the same file is used by multiple targets.

This is actually a very compelling reason to wrap compiler's output. But such wrapping would be incompatible with option 2 😦

In the ideal world each compilation may have a unique identity such that it is possible to connect compiler invocation command line, errors, recompilation causality chains, etc.

@matklad
Copy link
Member

matklad commented Aug 12, 2016

I've started the work on option 2 variant here: matklad@c9a6062

@alexcrichton
Copy link
Member

@nrc in order to preserve live updates we'd have to stream the output, e.g. capture it on the fly. Cargo does this for build scripts today so some of the work is implemented, but it's in general a pretty hairy prospect. Additionally it unfortunately disables colors, but I guess that doesn't matter for JSON?

@nrc
Copy link
Member

nrc commented Aug 14, 2016

ah, streaming, yeah, I'd been thinking about that for the compiler too. It is a pain.It seems solvable in the long term though (and necessary).

Yeah, I'd assume JSON errors would disable colours in any case - it's not meant to be user-readable.

bors added a commit that referenced this issue 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 bors closed this as completed in #3000 Oct 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants