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

Robust message parsing #106

Merged
merged 1 commit into from May 9, 2020
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
Expand Up @@ -9,7 +9,7 @@ readme = "README.md"

[dependencies]
serde = "1.0.79"
serde_derive = "1.0.79"
serde_derive = "1.0.107"
serde_json = "1.0.1"

[dependencies.semver]
Expand Down
7 changes: 5 additions & 2 deletions src/lib.rs
Expand Up @@ -56,7 +56,8 @@
//! .spawn()
//! .unwrap();
//!
//! for message in cargo_metadata::parse_messages(command.stdout.take().unwrap()) {
//! let reader = std::io::BufReader::new(command.stdout.take().unwrap());
//! for message in cargo_metadata::Message::parse_stream(reader) {
//! match message.unwrap() {
//! Message::CompilerMessage(msg) => {
//! println!("{:?}", msg);
Expand Down Expand Up @@ -96,8 +97,10 @@ pub use dependency::{Dependency, DependencyKind};
use diagnostic::Diagnostic;
pub use errors::{Error, Result};
pub use messages::{
parse_messages, Artifact, ArtifactProfile, BuildFinished, BuildScript, CompilerMessage, Message,
Artifact, ArtifactProfile, BuildFinished, BuildScript, CompilerMessage, Message, MessageIter
};
#[allow(deprecated)]
pub use messages::parse_messages;

mod dependency;
pub mod diagnostic;
Expand Down
31 changes: 30 additions & 1 deletion src/messages.rs
@@ -1,7 +1,7 @@
use super::{Diagnostic, PackageId, Target};
use serde_json;
use std::fmt;
use std::io::Read;
use std::io::{self, BufRead, Lines, Read};
use std::path::PathBuf;

/// Profile settings used to determine which compiler flags to use for a
Expand Down Expand Up @@ -110,23 +110,52 @@ pub enum Message {
/// This is emitted at the end of the build as the last message.
/// Added in Rust 1.44.
BuildFinished(BuildFinished),
/// A line of text which isn't a cargo or compiler message.
/// Line separator is not included
#[serde(skip)]
TextLine(String),
Copy link
Owner

Choose a reason for hiding this comment

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

Please apply #[serde(skip)] to this variant, we should never see a text-line from cargo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, just slapping skip broke serde: matklad@0b1c8d8

I think it intrracts badly with #[serde(other)] below. I'll try to read on other and figure out how to convince serde do what we want to do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that was genuine serde bug: serde-rs/serde#1804

Bumped serde-derive req in Cargo.toml

#[doc(hidden)]
#[serde(other)]
Unknown,
}

impl Message {
/// Creates an iterator of Message from a Read outputting a stream of JSON
/// messages. For usage information, look at the top-level documentation.
pub fn parse_stream<R: BufRead>(input: R) -> MessageIter<R> {
MessageIter {
lines: input.lines(),
}
}
}

impl fmt::Display for CompilerMessage {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{}", self.message)
}
}

/// An iterator of Messages.
pub struct MessageIter<R> {
lines: Lines<R>,
}

impl<R: BufRead> Iterator for MessageIter<R> {
type Item = io::Result<Message>;
fn next(&mut self) -> Option<Self::Item> {
let line = self.lines.next()?;
let message = line.map(|it| serde_json::from_str(&it).unwrap_or(Message::TextLine(it)));
Some(message)
}
}

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

/// Creates an iterator of Message from a Read outputting a stream of JSON
/// messages. For usage information, look at the top-level documentation.
#[deprecated(note = "Use Message::parse_stream instead")]
pub fn parse_messages<R: Read>(input: R) -> MessageIterator<R> {
serde_json::Deserializer::from_reader(input).into_iter::<Message>()
}
21 changes: 21 additions & 0 deletions tests/test_samples.rs
Expand Up @@ -474,3 +474,24 @@ fn current_dir() {
let namedep = meta.packages.iter().find(|p| p.name == "namedep").unwrap();
assert!(namedep.name.starts_with("namedep"));
}

#[test]
fn parse_stream_is_robust() {
// Proc macros can print stuff to stdout, which naturally breaks JSON messages.
// Let's check that we don't die horribly in this case, and report an error.
let json_output = r##"{"reason":"compiler-artifact","package_id":"chatty 0.1.0 (path+file:///chatty-macro/chatty)","target":{"kind":["proc-macro"],"crate_types":["proc-macro"],"name":"chatty","src_path":"/chatty-macro/chatty/src/lib.rs","edition":"2018","doctest":true},"profile":{"opt_level":"0","debuginfo":2,"debug_assertions":true,"overflow_checks":true,"test":false},"features":[],"filenames":["/chatty-macro/target/debug/deps/libchatty-f2adcff24cdf3bb2.so"],"executable":null,"fresh":false}
Evil proc macro was here!
{"reason":"compiler-artifact","package_id":"chatty-macro 0.1.0 (path+file:///chatty-macro)","target":{"kind":["lib"],"crate_types":["lib"],"name":"chatty-macro","src_path":"/chatty-macro/src/lib.rs","edition":"2018","doctest":true},"profile":{"opt_level":"0","debuginfo":2,"debug_assertions":true,"overflow_checks":true,"test":false},"features":[],"filenames":["/chatty-macro/target/debug/libchatty_macro.rlib","/chatty-macro/target/debug/deps/libchatty_macro-cb5956ed52a11fb6.rmeta"],"executable":null,"fresh":false}
"##;
let mut n_messages = 0;
let mut text = String::new();
for message in cargo_metadata::Message::parse_stream(json_output.as_bytes()) {
let message = message.unwrap();
match message {
cargo_metadata::Message::TextLine(line) => text = line,
_ => n_messages += 1,
}
}
assert_eq!(n_messages, 2);
assert_eq!(text, "Evil proc macro was here!");
}