Ignore broken console output in some situations. #8236
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
One of my main worries about this is how we're going to keep this working over time. This is a much larger PR than I would have expected it to be, and it seems like it's going to be very difficult to remember to always use not-println and not-eprintln. I'm also a little worried about ignoring the results of println/eprintln here too. Most unix-like tools effectively exit immediately on EPIPE instead of ignoring the result and keep going. I do think it makes sense to draw inspiration from other build tools like I think ideally it'd be great to say "println error means we die silently" by default and otherwise we'd whitelist a phase like the build phase where we handle println errors specifically. I don't think there's a great way to say "println error means we die silently", though, without overriding the panic handler and inspecting the panic message... |
It probably wouldn't be too difficult to write a test using syn that scans for println/eprintln. Would that make you feel more comfortable?
My thinking here is that all of these situations result in a very small amount of output, so exiting early doesn't save very much. Status messages (like "Downloading...") are still treated as errors if they cannot be displayed, so I believe any long-running steps (like resolution) should still exit early if stderr is closed. If you think the early exit is important, then we could probably change the print macros to return a The complete list of affected commands are:
AFAIK, all of these emit a limited amount of data. |
|
We discussed this in the Cargo meeting today and the conclusion was:
|
|
I added a test to check for print macros (and added dbg! for fun). It is not bullet-proof, but should be good enough. I checked |
|
@bors: r+ |
|
📌 Commit 5a096da has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-azure |
Update cargo 9 commits in cb06cb2696df2567ce06d1a39b1b40612a29f853..500b2bd01c958f5a33b6aa3f080bea015877b83c 2020-05-08 21:57:44 +0000 to 2020-05-18 17:12:54 +0000 - Handle LTO with an rlib/cdylib crate type (rust-lang/cargo#8254) - Gracefully handle errors during a build. (rust-lang/cargo#8247) - Update `im-rc` to 15.0.0 (rust-lang/cargo#8255) - Fix `cargo update` with unused patch. (rust-lang/cargo#8243) - Rephrased error message for disallowed sections in virtual workspace (rust-lang/cargo#8200) - Ignore broken console output in some situations. (rust-lang/cargo#8236) - Expand error message to explain that a string was found (rust-lang/cargo#8235) - Add context to some fs errors. (rust-lang/cargo#8232) - Move SipHasher to an isolated module. (rust-lang/cargo#8233)
Adapt to changes made in rust-lang/cargo#8236 by redirecting output through Cargo's `Shell` type rather than stdio.
Bump Cargo to 0.48 The main change is in 8938495, which works around rust-lang/cargo#8236 that introduced different mechanism to output stdio/shell-based data. Everything now goes through cargo `Shell` type so `std::io::set_print` trick stopped working - instead, we create our custom shell and fetch the build plan through it. r? `@Manishearth` or `@ibabushkin`
If stdout or stderr is closed while Cargo is running, Cargo would panic in some situations (usually with EPIPE). For example,
cargo install --list | grep -q cargo-fuzz, wheregrepwill close stdout once it gets a match. This changes it so that Cargo will ignore output errors in most situations. Failure to output a regular build message still results in an error, which follows the behavior of some tools likemake.All output, including stdout, now goes through
Shell.Closes #5234