Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Oct 17, 2017
Merged

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

merged 2 commits into from
Oct 17, 2017

Conversation

mistodon
Copy link

@mistodon mistodon commented Oct 9, 2017

For #29370

@rust-highfive
Copy link
Collaborator

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
Copy link
Member

@frewsxcv frewsxcv left a comment

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

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
Copy link
Member

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?

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.
Copy link
Member

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
Copy link
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
Copy link
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
Copy link
Member

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
Copy link
Contributor

bors commented Oct 17, 2017

📌 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

@frewsxcv frewsxcv frewsxcv left review comments

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