Allow redirecting subprocess stdout to our stderr etc. #88561
Conversation
|
r? @kennytm (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
kennytm
added
the
needs-fcp
the8472
added
the
T-libs-api
camelid
added
S-waiting-on-review
camelid
added
S-waiting-on-review
|
I have rebased onto current head, given it its own feature tag (and updated the version) and provided what I thiink is an implementation on Windows. I don't have any Windows here so I may have to iterate through the CI, in which case sorry for the noise. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The failing test is that for #30490. The test case, and the code there, involves passing a static I think I am going to have to break this MR up or work around this somehow. Nnnng. |
|
Just FYI: |
Indeed. This MR needs more work and an FCP, so there's no danger of them ending up in tree before I've had a chance to tell my git to autosquash them. But thanks for the note. |
ijackson
force-pushed
the
stdio-stdio
branch
from
5d857dc
to
ae5fd3d
Compare
|
Well, here's a version with Windows and which sidesteps whether |
This comment has been minimized.
This comment has been minimized.
|
Would someone be able to help with the Windows tests? Ideally, by adapting my new doctests so they work on Windows. (Breaking the Unix version while doing that would be fine; I can fix it up.) Alternatively, if someone could suggest a command to run that will succeed and produce some nonempty stdlout output, and no stderr output, that will probably suffice and I can iterate it through the CI. I am going to try setting the E-easy tag because I think this should be straightforward for someone familiar with Windows. @rustbot modify labels +O-windows +E-help-wanted +E-easy |
rustbot
added
E-easy
camelid
added
S-waiting-on-review
rfcbot
added
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
|
FTAOD, the actual MR branch here is not in a fit state to merge - it needs some squashing etc. So, assuming the FCP completes with approval, don't merge the branch right away. I will tidy it and force push, first. |
rfcbot
added
finished-final-comment-period
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
|
Thanks everyone for the approval, and the code review. I will update this branch soon, so that we can merge it. |
rustbot
added
A-testsuite
Co-authored-by: teor <teor@riseup.net>
This comment has been minimized.
This comment has been minimized.
|
For convenience, I have made the squashed and tidied branch into a new MR: #114590 |
|
Opinions will obviously differ, but personally, what you've done feels less convenient. |
There was a problem hiding this comment.
Thank you! I have approved your tidied PR in #114590 (review).
Allow redirecting subprocess stdout to our stderr etc. (redux) This is the code from rust-lang#88561, tidied up, including review suggestions, and with the for-testing-only CI commit removed. FCP for the API completed in rust-lang#88561. I have made a new MR to facilitate review. The discussion there is very cluttered and the branch is full of changes (in many cases as a result of changes to other Rust stdlib APIs since then). Assuming this MR is approvedl we should close that one. ### Reviewer doing a de novo review Just code review these four commits.. FCP discussion starts here: rust-lang#88561 (comment) Portability tests: you can see that this branch works on Windows too by looking at the CI results in rust-lang#88561, which has the same code changes as this branch but with an additional "DO NOT MERGE" commit to make the Windows tests run. ### Reviewer doing an incremental review from some version of rust-lang#88561 Review the new commits since your last review. I haven't force pushed the branch there. git diff the two branches (eg `git diff 176886197d6..0842b69c219`). You'll see that the only difference is in gitlab CI files. You can also see that *this* MR doesn't touch those files.
Allow redirecting subprocess stdout to our stderr etc. (redux) This is the code from rust-lang#88561, tidied up, including review suggestions, and with the for-testing-only CI commit removed. FCP for the API completed in rust-lang#88561. I have made a new MR to facilitate review. The discussion there is very cluttered and the branch is full of changes (in many cases as a result of changes to other Rust stdlib APIs since then). Assuming this MR is approvedl we should close that one. ### Reviewer doing a de novo review Just code review these four commits.. FCP discussion starts here: rust-lang#88561 (comment) Portability tests: you can see that this branch works on Windows too by looking at the CI results in rust-lang#88561, which has the same code changes as this branch but with an additional "DO NOT MERGE" commit to make the Windows tests run. ### Reviewer doing an incremental review from some version of rust-lang#88561 Review the new commits since your last review. I haven't force pushed the branch there. git diff the two branches (eg `git diff 176886197d6..0842b69c219`). You'll see that the only difference is in gitlab CI files. You can also see that *this* MR doesn't touch those files.
Allow redirecting subprocess stdout to our stderr etc. (redux) This is the code from #88561, tidied up, including review suggestions, and with the for-testing-only CI commit removed. FCP for the API completed in #88561. I have made a new MR to facilitate review. The discussion there is very cluttered and the branch is full of changes (in many cases as a result of changes to other Rust stdlib APIs since then). Assuming this MR is approvedl we should close that one. ### Reviewer doing a de novo review Just code review these four commits.. FCP discussion starts here: rust-lang/rust#88561 (comment) Portability tests: you can see that this branch works on Windows too by looking at the CI results in #88561, which has the same code changes as this branch but with an additional "DO NOT MERGE" commit to make the Windows tests run. ### Reviewer doing an incremental review from some version of #88561 Review the new commits since your last review. I haven't force pushed the branch there. git diff the two branches (eg `git diff 176886197d6..0842b69c219`). You'll see that the only difference is in gitlab CI files. You can also see that *this* MR doesn't touch those files.
Allow redirecting subprocess stdout to our stderr etc. (redux) This is the code from #88561, tidied up, including review suggestions, and with the for-testing-only CI commit removed. FCP for the API completed in #88561. I have made a new MR to facilitate review. The discussion there is very cluttered and the branch is full of changes (in many cases as a result of changes to other Rust stdlib APIs since then). Assuming this MR is approvedl we should close that one. ### Reviewer doing a de novo review Just code review these four commits.. FCP discussion starts here: rust-lang/rust#88561 (comment) Portability tests: you can see that this branch works on Windows too by looking at the CI results in #88561, which has the same code changes as this branch but with an additional "DO NOT MERGE" commit to make the Windows tests run. ### Reviewer doing an incremental review from some version of #88561 Review the new commits since your last review. I haven't force pushed the branch there. git diff the two branches (eg `git diff 176886197d6..0842b69c219`). You'll see that the only difference is in gitlab CI files. You can also see that *this* MR doesn't touch those files.
Allow redirecting subprocess stdout to our stderr etc. (redux) This is the code from #88561, tidied up, including review suggestions, and with the for-testing-only CI commit removed. FCP for the API completed in #88561. I have made a new MR to facilitate review. The discussion there is very cluttered and the branch is full of changes (in many cases as a result of changes to other Rust stdlib APIs since then). Assuming this MR is approvedl we should close that one. ### Reviewer doing a de novo review Just code review these four commits.. FCP discussion starts here: rust-lang/rust#88561 (comment) Portability tests: you can see that this branch works on Windows too by looking at the CI results in #88561, which has the same code changes as this branch but with an additional "DO NOT MERGE" commit to make the Windows tests run. ### Reviewer doing an incremental review from some version of #88561 Review the new commits since your last review. I haven't force pushed the branch there. git diff the two branches (eg `git diff 176886197d6..0842b69c219`). You'll see that the only difference is in gitlab CI files. You can also see that *this* MR doesn't touch those files.
Currently there is no safe or portable way to redirect a
Commands stdout to the invoking program's stderr, nor vice versa.There a comment in the source that suggests that this was intended, and trying to handle the fd shuffling problem properly, but it can never be reached other than by crazy unsafe stunts.
Instead, provide appropriate
Fromimpls.I have only tested the implementation for Unix because I know how to do that.There is now a Windows implementation which seems to pass the CI. This branch currently contains a DO NOT MERGE commit to enable the Windows CI build on this MR branch. That should be removed after code review and before merge.Being trait impls, these are insta-stable. The question is just whether we want to support platforms that cannot do this, and if so, whether we want that to be a compile error or are happy for it to be detected by
Commandmethods returning errors.Closes #79731