Skip to content

Document defaults for stdin, stdout, and stderr methods of Command - #45151

Merged
bors merged 2 commits into
rust-lang:masterfrom
mistodon:stdio_default_docs
Oct 17, 2017
Merged

Document defaults for stdin, stdout, and stderr methods of Command#45151
bors merged 2 commits into
rust-lang:masterfrom
mistodon:stdio_default_docs

Conversation

@mistodon

@mistodon mistodon commented Oct 9, 2017

Copy link
Copy Markdown

For #29370

@rust-highfive

rust-highfive commented Oct 9, 2017

Copy link
Copy Markdown
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

frewsxcv
frewsxcv reviewed Oct 10, 2017
View reviewed changes

@frewsxcv frewsxcv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! just a couple small comments

Comment thread src/libstd/process.rs Outdated
/// Defaults to [`inherit`] when used with `spawn` or `status`, and
/// defaults to [`piped`] when used with `output`.
///
/// On Windows, if the `#![windows_subsystem = "windows"]` attribute is

@frewsxcv frewsxcv Oct 10, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you put the windows-specific lines below a 'Platform-specific behavior' header like we do here?

Comment thread src/libstd/process.rs Outdated
/// defaults to [`piped`] when used with `output`.
///
/// On Windows, if the `#![windows_subsystem = "windows"]` attribute is
/// set, no stdin is connected unless explicitly assigned.

@frewsxcv frewsxcv Oct 10, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity, what do you mean by 'assigned' here? i'm guessing that's some windows-specific concept i'm not familiar with

@mistodon

mistodon commented Oct 10, 2017

Copy link
Copy Markdown
Author

It seems I was misunderstanding the Windows-specific behaviour there - it's only relevant when the child process uses the Windows subsystem, not the parent process. (This comes from the comment on #29370 by @retep998 who can maybe give more insight?) I've removed the comments about it in the above commit.

I did some testing and it doesn't look like the child's subsystem affects the defaults of the stdin, stdout, and stderr methods. It seems that, when a child process uses the Windows subsystem, Stdio::inherit works differently though. (It will not output to the console, for example, but you can still pipe into and out of it). Stdio::piped still works as expected (and so the output method behaves identically regardless of the Windows subsystem).

I still don't fully understand this, but it seems like it should be documented on Stdio::inherit (and possibly spawn and status) rather than on stdin, stdout, stderr. I wouldn't know what to write though, so I'll leave that for another PR.

@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2017
@carols10cents

carols10cents commented Oct 16, 2017

Copy link
Copy Markdown
Member

r? @frewsxcv

How's this looking, Corey? It looks like your concerns were addressed?

@rust-highfive rust-highfive assigned frewsxcv and unassigned BurntSushi Oct 16, 2017
@frewsxcv

frewsxcv commented Oct 17, 2017

Copy link
Copy Markdown
Contributor

i'm not familiar at all with the windows subsystem details regarding this, so i can't be much help there. but this pr looks great, thanks for your contribution!

@bors r+ rollup

@bors

bors commented Oct 17, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit 210c911 has been approved by frewsxcv

@kennytm kennytm 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 Oct 17, 2017
@kennytm kennytm mentioned this pull request Oct 17, 2017
Merged
kennytm added a commit to kennytm/rust that referenced this pull request Oct 17, 2017
@kennytm
Rollup merge of rust-lang#45151 - Pirh:stdio_default_docs, r=frewsxcv
18f2895 
Document defaults for stdin, stdout, and stderr methods of Command

For rust-lang#29370
bors added a commit that referenced this pull request Oct 17, 2017
@bors
Auto merge of #45344 - kennytm:rollup, r=kennytm
f6d7514 
Rollup of 10 pull requests

- Successful merges: #45097, #45151, #45307, #45308, #45310, #45315, #45321, #45329, #45338, #45339
- Failed merges:
@bors
bors merged commit 210c911 into rust-lang:master Oct 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@frewsxcv frewsxcv frewsxcv left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@frewsxcv frewsxcv

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@mistodon @rust-highfive @carols10cents @frewsxcv @bors @kennytm @BurntSushi @aidanhs