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

Ignore broken console output in some situations. #8236

Merged
merged 3 commits into from
May 14, 2020

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented May 12, 2020

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, where grep will 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 like make.

All output, including stdout, now goes through Shell.

Closes #5234

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 12, 2020
@alexcrichton
Copy link
Member

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 make about what to do once we're building, but otherwise if we're, for example, in resolution where some statuses may be printed do we keep going in the background once stdout/stderr are closed?

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...

@ehuss
Copy link
Contributor Author

ehuss commented May 12, 2020

it's going to be very difficult to remember to always use not-println and not-eprintln

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?

I'm also a little worried about ignoring the results of println/eprintln here too.

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 CargoResult, and then at the top level check for the equivalent of EPIPE, and ignore it (exit with 0). I'm reluctant to do that just due to the added complexity, and like I mentioned, I don't know of any circumstances where that would have a significant improvement. However, I'm willing to add it if you think it is important. I don't think I would want to catch panics.

The complete list of affected commands are:

  • -Z help
  • cargo version and --version
  • cargo list and --list
  • cargo pkgid
  • cargo tree
  • -Zfeatures=compare
  • cargo install --list
  • cargo package --list
  • cargo login
  • cargo owners --list
  • cargo search
  • cargo vendor (the final "here's your config")
  • JSON output:
    • cargo locate-project
    • cargo metadata
    • cargo read-manifest
    • cargo verify-project
    • -Ztimings
    • --unit-graph

AFAIK, all of these emit a limited amount of data. cargo metadata might be the largest. I'm not sure how tools like jq works, if it is capable of closing the input prematurely. But once cargo starts sending the content out, I think it only takes a few milliseconds to finish.

@alexcrichton
Copy link
Member

We discussed this in the Cargo meeting today and the conclusion was:

  • The semantics here seem good. They match make for the build phase and other CLI tools for everything else.
  • Let's add a CI check of some form that println! and eprintln! aren't part of the codebase
  • We should confirm what happens when make detects stdout isn't there and there are also active jobs elsewhere.

@ehuss ehuss force-pushed the no-panic-output branch from 51c8549 to 62711bd Compare May 13, 2020 23:23
@ehuss
Copy link
Contributor Author

ehuss commented May 13, 2020

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 make, and it does the same as Cargo when it tries to print to stdout and it is closed (fails the build, prints Waiting for unfinished jobs, etc.).

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented May 14, 2020

📌 Commit 5a096da has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2020
@bors
Copy link
Contributor

bors commented May 14, 2020

⌛ Testing commit 5a096da with merge cb7cff1...

@bors
Copy link
Contributor

bors commented May 14, 2020

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing cb7cff1 to master...

@bors bors merged commit cb7cff1 into rust-lang:master May 14, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request May 20, 2020
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)
Xanewok added a commit to Xanewok/rust-semverver that referenced this pull request Nov 9, 2020
Adapt to changes made in rust-lang/cargo#8236
by redirecting output through Cargo's `Shell` type rather than stdio.
bors added a commit to rust-lang/rust-semverver that referenced this pull request Nov 10, 2020
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`
@ehuss ehuss added this to the 1.45.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

install panics on broken pipe
4 participants