-
Notifications
You must be signed in to change notification settings - Fork 552
Forward lines from stderr of child process to stdout on the same thread, instead of spawning a thread #940
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
Conversation
|
There're a lot of changes here, it might take me some time to review it. |
|
If it helps, I have a build of the Rust toolchain that uses this: https://github.com/rust-lang/rust/actions/runs/7719025147/job/21041489650?pr=119199 |
|
@NobodyXu I'm going to have to rework this, since there are two issues: The first, fixable, issue is that The second, fundamental, issue is that There's a few different ways that we can address these:
Thoughts? |
If you bring back os_pipe, then you could read without blocking. |
|
@NobodyXu ok, I think I have this figured out: for Unix, I'm setting the pipe as non-blocking and checking for I also removed the |
5b47c14 to
7b2054b
Compare
NobodyXu
left a comment
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.
Thanks, it overall LGTM, just two nitpicks I want to discuss with you before I approve and merge it.
1fa9906 to
5a045cc
Compare
68ed37a to
d42c1e5
Compare
|
Thanj you! |
|
Can we please get a release with this change? |
|
Yes, I definitely want to have a release. There's a regression #902 that needs to be fixed before a release can be done. |
cc-rswas redirecting output fromstderrof its child processes to a pipe, which was then being read on dedicated thread and written tostdoutwith a wrapper message.This pattern caused a few issues:
cc-rstolibcwhen building the Rust standard library with optimized compiler intrinsics, resulting in the Rust build breaking (see Upgrading to [email protected] breaks libstd bootstrap #913).parallelfeature enabled, there was no synchronization between different child processes writing to the pipe, potentially resulting in corrupted output.The fix for this is to forward output from
stderrtostdouton the current thread. For theparallelfeature this required implementing a non-blocking version ofBufReader::read_untilthat consumes whatever data is available and then continues.Fixes #913