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=json support #53

Closed
wants to merge 7 commits into from
Closed

Conversation

roblabla
Copy link
Contributor

Fixes #45

Most of the structs come directly from cargo or rustc. This is not ready yet, there are a few things that need to be done. Most importantly, the enums (mostly Message) need to have a catch-all variant so that it won't break if new variants are added. Also, I think we'd do well to change the names of some structs here and there.

Also, I need to add some tests and fix the doctest example.

@roblabla
Copy link
Contributor Author

roblabla commented Sep 10, 2018

For my sanity's sake, I'm currently working on fixing serde-rs/serde#912. This will allow turning Message into

enum Message {
    CompilerArtifact(Artifact),
    CompilerMessage(FromCompiler),
    BuildScript(BuildScriptExecution),
    #[doc(hidden)]
    #[serde(other)]
    Unknown
}

Blocking on serde-rs/serde#1382

@@ -0,0 +1,118 @@
//! This module contains `Diagnostic` and the types/functions it uses for deserialization.
Copy link
Owner

Choose a reason for hiding this comment

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

This file should be shared with rustfix in some manner

cc @killercup

@roblabla
Copy link
Contributor Author

roblabla commented Sep 16, 2018

Serde 1.0.79 is released, and with it #[serde(other)] is now available. I pushed a commit updating the serde dep to reflect that.

Question: Should all enums currently having a DoNotMatchExhaustively arm be switched to #[serde(other)]? There are only two as of this PR (not counting Message): dependency::DependencyKind and diagnostic::Applicability.

This is what it would change:

// Before
assert!(serde_json::from_str::<DependencyKind>(r#" "unknown" "#).is_error());

// After
assert_eq!(serde_json::from_str::<DependencyKind>(r#" "unknown" "#),
     Ok(DependencyKind::Unknown));

The Unknown arm would still be #[doc(hidden)]'d, so the user would have to use the catchall to match it.

@roblabla roblabla changed the title [WIP] add message-format=json support Add message-format=json support Sep 19, 2018
@konstin
Copy link
Contributor

konstin commented Sep 20, 2018

I just tried the branch and I really like it. It makes a nice improvements over my earlier self-written structs, esp. with parse_message_stream.

One thing that would be useful is a ToString impl on FromCompiler using the rendered message. (I also wonder why rendered is an Option<String>. Is there a case were there is no rendered message?)

/// The enabled features for this artifact
pub features: Vec<String>,
/// The full paths to the generated artifacts
pub filenames: Vec<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Might those be better as PathBuf than as String?

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 thought so too, but cargo_metadata uses String in places where PathBuf makes more sense, e.g.

pub workspace_root: String,
. Maybe that should be changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Imho workspace_root should also be changed, but maybe in a different PR. @oli-obk What do you think?

Copy link

Choose a reason for hiding this comment

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

@konstin Oliver is away until October.

Copy link
Owner

Choose a reason for hiding this comment

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

I remember trying PathBuf in more places, but then ending up with errors because not everything must be a path. For these two cases it seems fine though.

//! .stdout(Stdio::piped())
//! .spawn().unwrap();
//!
//! for message in cargo_metadata::parse_message_stream(command.stdout.unwrap()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Imo this example should .take() before unwrapping. Otherwise you can't wait for the command to finish afterwards.

@roblabla
Copy link
Contributor Author

roblabla commented Sep 20, 2018

The Option<> around rendered comes from the structs defined in Rustc. I think it only happens in children though, the root seems to always have a rendered message. E.G. https://github.com/rust-lang/rust/blob/215bf3abd9c0eb1fd0c6cae7c92a842732dc033d/src/test/ui/lint/unused_parens_json_suggestion.stderr#L60 .

ToString might be a good idea. Display also. I was also planning on writing a print function that would also properly colorize everything in order to have identical output to rustc/cargo. (I believe it to be possible, all information is readily available).

EDIT: While ToString/Display might come in this PR if it's desirable, the print function will likely come with another PR, as I don't have the time to work on it right now.

@oli-obk
Copy link
Owner

oli-obk commented Oct 1, 2018

Should all enums currently having a DoNotMatchExhaustively arm be switched to #[serde(other)]? There are only two as of this PR (not counting Message): dependency::DependencyKind and diagnostic::Applicability.

seems ok. It's more useful to get a partial parse than an error if new thing are added to the enums on the rustc/cargo side

@konstin
Copy link
Contributor

konstin commented Nov 25, 2018

What would be required to get this merged and released on crates.io? I'm using those types in pyo3-pack and would like to release on crates.io without vendoring tricks.

@roblabla
Copy link
Contributor Author

I'll get around to addressing all the comments in the PR tomorrow (Using PathBuf, changing enums into #[serde(other)], fixing the doctest, implementing ToString). Then I think it should be ready for merging?

epage added a commit to epage/escargot that referenced this pull request Dec 31, 2018
Inspired by oli-obk/cargo_metadata#53

This allowed `CargoRun` to improve the logged output.
/// The associated error code for this diagnostic
pub code: Option<DiagnosticCode>,
/// "error: internal compiler error", "error", "warning", "note", "help"
pub level: String,
Copy link

Choose a reason for hiding this comment

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

Is there a reason this isn't an enum?

Copy link

@epage epage Dec 31, 2018

Choose a reason for hiding this comment

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

I guess the same could be said for cargo-metadatas existing kind, crate_types, opt_levels, and anywhere else supported values are documented in a comment.

Copy link
Owner

Choose a reason for hiding this comment

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

Yea, this was mostly chose for forward compatibility

@epage
Copy link

epage commented Dec 31, 2018

I've forked this in my PR for escargot
crate-ci/escargot#20

Main differences

  • I use Cow / serde(borrowed) everywhere
  • I went ahead and switched to Path
  • Diagnostic::level is an enum.
  • Enums are Copy where possible
  • I have a strict_unstable feature that removes the Unknown variants and adds serde(deny_unknown_fields) that I use with my tests in the hope that it will help me identify when new fields are added

In addition, I went ahead and forked WorkspaceMember and Target rather than re-exporting cargo-metadata. I find it curious that WorkspaceMember serializes to raw: String with parse functions rather than the components it makes up but I went ahead and kept that.

epage added a commit to epage/escargot that referenced this pull request Dec 31, 2018
Inspired by oli-obk/cargo_metadata#53

This allowed `CargoRun` to improve the logged output.
Copy link
Owner

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

I've forked this in my PR for escargot

Maybe we should create a library-only crate that exposes the shared types?

/// An iterator of Message.
type MessageIterator<R> = serde_json::StreamDeserializer<'static, serde_json::de::IoRead<R>, Message>;

// TODO: Should I use de::Read instead, to support deserializing from a slice?
Copy link
Owner

Choose a reason for hiding this comment

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

If it's strictly more powerful to use de::Read, then by all means

Copy link
Contributor Author

@roblabla roblabla Jan 10, 2019

Choose a reason for hiding this comment

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

On investigating this a bit, de::Read isn't very convenient. It's implemented on serde_json::de::IoRead, serde_json::de::SliceRead, etc... and not on the actual libstd. Besides, I have a hard time envisioning under which situation would someone try to read an array of cargo messages from a byte array instead of an std::io::Read...

I'll remove the TODO.


// TODO: Should I use de::Read instead, to support deserializing from a slice?
/// Creates an iterator of Message from a Read outputting a stream of JSON
/// messages. For usuage information, look at the top-level documentation.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/// messages. For usuage information, look at the top-level documentation.
/// messages. For usage information, look at the top-level documentation.

/// The associated error code for this diagnostic
pub code: Option<DiagnosticCode>,
/// "error: internal compiler error", "error", "warning", "note", "help"
pub level: String,
Copy link
Owner

Choose a reason for hiding this comment

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

Yea, this was mostly chose for forward compatibility

@epage epage mentioned this pull request Jan 16, 2019
@roblabla
Copy link
Contributor Author

Superseded by #64

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

Successfully merging this pull request may close these issues.

Add structs output by --message-format=json?
5 participants