-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Move stream_output to ProcessBuilder #3091
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
})); | ||
child.wait() | ||
})().map_err(|e| { | ||
process_error(&format!("Could not execute process `{}`", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this start w/ a lowercase c
(to mesh with Cargo's other error messages)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change other messages in this file to lowecase as well.
status: status, | ||
}; | ||
if !output.status.success() { | ||
Err(process_error(&format!("Process didn't exit successfully: `{}`", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same here as above)
@@ -107,6 +107,63 @@ impl ProcessBuilder { | |||
} | |||
} | |||
|
|||
pub fn exec_with_streaming<F1, F2>(&self, mut on_stdout_line: F1, mut on_stderr_line: F2) | |||
-> Result<Output, ProcessError> | |||
where F1: FnMut(&str), F2: FnMut(&str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For big functions like this I tend to try to keep them concrete (to avoid codegen bloat), perhaps these could both be &mut FnMut(&str)
as arguments?
79a77b0
to
4f8975b
Compare
Should be OK now, Travis fails because of the stalled build. |
4f8975b
to
5eb80b7
Compare
@bors: r+ |
📌 Commit 5eb80b7 has been approved by |
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.
☀️ Test successful - cargo-cross-linux, cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64 |
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.