Propagate io and terminal errors upward and ignore EPIPE #1630
Conversation
|
HI @NickeZ thanks for taking the time to produce this alternative patch. I'm sorry I was away last week so didn't get a chance to review it before now. A brief look over suggests this is a reasonable approach, but I want to take some time to consider if there're other places where we ought to handle epipe in a similar fashion (e.g. component list) Please bear with me for a few days. |
|
Great, but can you check if the macro I have written is correct/idiomatic? edit: It could probably just be a regular function... |
|
Is it possible to add a test that runs the equivalent of |
There was a problem hiding this comment.
Yeah, I think as a function it's a bit nicer. I didn't mind the macro at all, but this is perhaps easier for others to grok.
Are there any other parts of the codebase where handling epipe would be useful? I'm not asking for them in this PR, it has a 👍 already from me, but perhaps once this is in you might go for a wander and see if there's other places worth checking on?
Regarding the testing for rustup show | head -1 I don't think we have a way to easily simulate that in our tests right now, so I don't mind not having that yet, but if you think of something I'll be pleased to review another PR (or change on this one) for it.
| @@ -1,3 +1,5 @@ | |||
| # This file is automatically @generated by Cargo. | |||
| # It is not intended for manual editing. | |||
There was a problem hiding this comment.
Is this added by a new version of Cargo?
There was a problem hiding this comment.
It must have been. I only ran cargo fmt, check and build. I didn't manually change that file. do you want me to remove it?
There was a problem hiding this comment.
Is this added by a new version of Cargo?
Yes: rust-lang/cargo#6548
|
Do you want me to squash it or anything else before merging? To me it is good to go as it is now. |
My preference is certainly that the commits are neat and tidy, so yes, a squash would be appreciated. Thanks |
|
OK, squashed. |
|
☔ The latest upstream changes (presumably #1643) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Sorry, this needs a rebase |
This commit solves the problem where rustup panics when the stdout is a pipe that has been closed. Rustup will now ignore EPIPE on write and instead quit with status code 0.
|
rebased and pushed |
|
ping |
This is an alternative to: #1622
In this solution I instead ignore the EPIPE error at the highest level. I haven't contributed to rustup before, so please check the PR carefully.
Fixes #1621