Allow redirecting subprocess stdout to our stderr etc. (redux) #114590

ijackson · 2023-08-07T15:56:26Z

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

This involves adding a new variant `imp::Stdio::StaticFd`. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

We're going to want to reuse this bit of code.

This involves a new variant `imp;::Stdio::InheritSpecific`. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

rustbot · 2023-08-07T15:56:33Z

r? @joshtriplett

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

pitaj · 2023-08-07T16:20:32Z

I can understand splitting the refactor out from the rest, but I would recommend squashing the first, third, and fourth commits together since they each touch a different file and are tightly linked.

ijackson · 2023-08-07T16:26:28Z

I can understand splitting the refactor out from the rest, but I would recommend squashing the first, third, and fourth commits together since they each touch a different file and are tightly linked

#88561 as initially submitted had the same commit structure and the maintainers didn't seem to object to it then, and I prefer it this way so I will leave this as it is unless the reviewer would like me to squash it.

(When I'm a reviewer, I generally prefer the commits as small as possible. It's always possible to review successive commits together if the submitter has split them up; whereas if the submitter has merged them, I can only review them in that form or ask for a resubmission.)

pitaj · 2023-08-07T17:32:21Z

#88561 as initially submitted had the same commit structure and the maintainers didn't seem to object to it then

I don't think that really means anything. I don't think they care about commit structure at all until the very end, where they might ask for everything to be squashed.

(When I'm a reviewer, I generally prefer the commits as small as possible. It's always possible to review successive commits together if the submitter has split them up; whereas if the submitter has merged them, I can only review them in that form or ask for a resubmission.)

The way I see it commits 1, 3, and 4 are all part of the same change. Neither 1 nor 3 make sense without 4, so they should be combined. I would never have committed them separately in the first place.

But that's just my suggestion. Keep it how you like it.

dtolnay

Thank you! This is terrific.

T-libs-api FCP has completed in #88561 (comment).

Thank you for sticking with this over 2 years 😮👏 while our team has been so backlogged -- we're starting to make better progress again recently.

dtolnay · 2023-09-07T22:50:33Z

@bors r+

bors · 2023-09-07T22:50:35Z

📌 Commit 36295fa has been approved by dtolnay

It is now in the queue for this repository.

bors · 2023-09-08T02:09:26Z

⌛ Testing commit 36295fa with merge 8f0fd01...

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.

bors · 2023-09-08T02:24:46Z

💔 Test failed - checks-actions

dtolnay · 2023-09-08T04:04:15Z

@rustbot author

…Stdio This implementation is wrong. Like the impl for From<File>, it is forced to panic because process::Stdio in unsupported/process.rs doesn't have a suitable variant. The root cause of the problem is that process::Stdio in unsupported/process.rs has any information in it at all. I'm pretty sure that it should just be a unit struct. However, making that build on all platforms is going to be a lot of work, iterating through CI and/or wrestling Docker. I don't think this extra panic is making things significantly worse. For now I have added some TODOs.

ijackson · 2023-09-09T18:01:53Z

Thanks for the review and the encouragement.

I've fixed WASM (with some flail). Please LMK when you want me to squash this. Hopefully it will turn out not to break some other platform too.

@rustbot reviewer

dtolnay · 2023-09-09T18:56:50Z

@bors r+

bors · 2023-09-09T18:56:51Z

📌 Commit 436fe01 has been approved by dtolnay

It is now in the queue for this repository.

bors · 2023-09-09T19:56:55Z

⌛ Testing commit 436fe01 with merge 559421e...

bors · 2023-09-09T21:45:38Z

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 559421e to master...

rust-timer · 2023-09-10T00:00:20Z

Finished benchmarking commit (559421e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.2%	[3.2%, 3.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.3%, -2.0%]	3
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 629.671s -> 628.555s (-0.18%)
Artifact size: 317.95 MiB -> 317.86 MiB (-0.03%)

ijackson added 4 commits August 7, 2023 15:18

std::process (unix): Implement From<io::Stdout> etc. for imp::Stdio

fb7f1a8

This involves adding a new variant `imp::Stdio::StaticFd`. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

std::process (windows): refactor Stdio::to_handle slightly

deda7d3

We're going to want to reuse this bit of code.

std::process (windows): Implement From<io::Stdout> etc. for imp::Stdio

1bab95b

This involves a new variant `imp;::Stdio::InheritSpecific`. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

std::process: impl From<io::Stdout> (etc.) for Stdio

36295fa

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

rustbot assigned joshtriplett Aug 7, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 7, 2023

ijackson mentioned this pull request Aug 7, 2023

Allow redirecting subprocess stdout to our stderr etc. #88561

Closed

dtolnay assigned dtolnay and unassigned joshtriplett Sep 7, 2023

dtolnay added A-process Area: `std::process` and `std::env` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 7, 2023

dtolnay approved these changes Sep 7, 2023

View reviewed changes

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 Sep 7, 2023

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 8, 2023

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 8, 2023

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 8, 2023

rustbot added O-unix Operating system: Unix-like O-windows Operating system: Windows labels Sep 9, 2023

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 9, 2023

dtolnay force-pushed the stdio-stdio-2 branch from 3124903 to 436fe01 Compare September 9, 2023 18:56

dtolnay approved these changes Sep 9, 2023

View reviewed changes

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 Sep 9, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 9, 2023

bors merged commit 559421e into rust-lang:master Sep 9, 2023

rustbot added this to the 1.74.0 milestone Sep 9, 2023

bors mentioned this pull request Sep 9, 2023

Add the ability to see inherited envs on Command #110327

Closed

fmease mentioned this pull request Dec 28, 2024

Command cannot replumb stdout to stderr #79731

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow redirecting subprocess stdout to our stderr etc. (redux) #114590

Allow redirecting subprocess stdout to our stderr etc. (redux) #114590

ijackson commented Aug 7, 2023

rustbot commented Aug 7, 2023

pitaj commented Aug 7, 2023

ijackson commented Aug 7, 2023

pitaj commented Aug 7, 2023

dtolnay left a comment

dtolnay commented Sep 7, 2023

bors commented Sep 7, 2023

bors commented Sep 8, 2023

This comment has been minimized.

bors commented Sep 8, 2023

dtolnay commented Sep 8, 2023

This comment has been minimized.

ijackson commented Sep 9, 2023

dtolnay commented Sep 9, 2023

bors commented Sep 9, 2023

bors commented Sep 9, 2023

bors commented Sep 9, 2023

rust-timer commented Sep 10, 2023

Allow redirecting subprocess stdout to our stderr etc. (redux) #114590

Allow redirecting subprocess stdout to our stderr etc. (redux) #114590

Conversation

ijackson commented Aug 7, 2023

Reviewer doing a de novo review

Reviewer doing an incremental review from some version of #88561

rustbot commented Aug 7, 2023

pitaj commented Aug 7, 2023

ijackson commented Aug 7, 2023

pitaj commented Aug 7, 2023

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay commented Sep 7, 2023

bors commented Sep 7, 2023

bors commented Sep 8, 2023

This comment has been minimized.

bors commented Sep 8, 2023

dtolnay commented Sep 8, 2023

This comment has been minimized.

ijackson commented Sep 9, 2023

dtolnay commented Sep 9, 2023

bors commented Sep 9, 2023

bors commented Sep 9, 2023

bors commented Sep 9, 2023

rust-timer commented Sep 10, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size