-
Notifications
You must be signed in to change notification settings - Fork 459
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
Forward lines from stderr of child process to stdout on the same thread, instead of spawning a thread #940
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
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-rs
was redirecting output fromstderr
of its child processes to a pipe, which was then being read on dedicated thread and written tostdout
with a wrapper message.This pattern caused a few issues:
cc-rs
tolibc
when building the Rust standard library with optimized compiler intrinsics, resulting in the Rust build breaking (see Upgrading to cc@1.0.84 breaks libstd bootstrap #913).parallel
feature 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
stderr
tostdout
on the current thread. For theparallel
feature this required implementing a non-blocking version ofBufReader::read_until
that consumes whatever data is available and then continues.Fixes #913