libstd: use block buffering when stdout is not a TTY #60904
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
sfackler
reviewed
May 17, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
BurntSushi
reviewed
May 17, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I'm not really sure how to fix the test cases other than adding explicit flushes as we can't lock |
|
It seems like adding explicit flushes is the expected necessary change after this PR. |
Dylan-DPC-zz
added
S-waiting-on-author
|
ping from triage @Arcterus any updates on this? |
|
Sorry, I’ve been dealing with some other stuff for the past couple weeks. I was trying to add the ability to switch buffering strategies the last time I was working on it, so I’ll try to get (at least) a preliminary implementation up this week. |
|
To enable switching blocking strategies, it'd probably be easiest to move from an enum to a single type that just has a bool inside tracking if it should flush on linebreaks or not. |
|
ping from triage @Arcterus |
|
It's still open. Did you forget to close it? |
|
@nico-abram yep 😄 |
Dylan-DPC-zz
added
S-inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is meant to resolve #60673.
At the moment, there are a couple issues. I am unfamiliar with several of the platforms in
libstd::io::sys, so I don't actually know if they have methods to determine ifstdoutis a TTY (or some equivalent).Additionally, the original issue mentioned buffering for
stderrin addition tostdout, which I have not implemented at the moment (although doing so is pretty straightforward). I also have not added methods to manually switch between block buffering and line buffering. If either of these are desired, let me know and I'll try to add them.I noticed that
run-pass/command-pre-exec.rsseems to fail with this change because (AFAICT) theprintln!()inCommand::pre_exec()doesn't get flushed, so the contents ofstdoutare missing ahello. I haven't modified the test yet because I am unsure if we should just tell users to flushstdout(in which case I'll change the test), or if we should special case functions that can exit inlibstd, or something else.run-pass/issues/issue-30490.rsfails for a similar reason (doesn't flush beforeexec).