Skip to content

Fix process module tests to run on Windows #38797

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

Merged
merged 3 commits into from
Jan 8, 2017

Conversation

abhijeetbhagat
Copy link
Contributor

Fixes #38565
r? @retep998

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @retep998 (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.

@retep998
Copy link
Member

retep998 commented Jan 3, 2017

You have a lot of missing spaces, in particular ){ which needs to be ) {.

@AraHaan
Copy link

AraHaan commented Jan 3, 2017

don't rely on cmd.exe because in future versions of windows that will be removed for powershell. Not unless you expect them to rip cmd.exe from Windows 7.

@retep998
Copy link
Member

retep998 commented Jan 3, 2017

cmd.exe has not been removed from Windows yet, not even in insider builds. Microsoft has merely hidden it from certain places to make powershell more of a default option. We can reevaluate the decision to use cmd.exe if Microsoft ever does take such a drastic step, but at the moment cmd.exe is a significantly better choice than the status quo of using gnu utilities that don't exist on Windows in the first place.

@petrochenkov
Copy link
Contributor

r=me with fixed formatting (#38797 (comment))
@bors delegate=retep998

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 4, 2017

📌 Commit 7152bce has been approved by petrochenkov

@retep998
Copy link
Member

retep998 commented Jan 5, 2017

@AraHaan To dissuade your fears, Microsoft has even published a blog post saying that they will not be removing cmd.exe in the near or distant future. https://blogs.msdn.microsoft.com/commandline/2017/01/04/rumors-of-cmds-death-have-been-greatly-exaggerated/

@bors
Copy link
Collaborator

bors commented Jan 7, 2017

⌛ Testing commit 7152bce with merge 1d5f4e8...

@bors
Copy link
Collaborator

bors commented Jan 8, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

alexcrichton commented Jan 8, 2017 via email

@bors
Copy link
Collaborator

bors commented Jan 8, 2017

⌛ Testing commit 7152bce with merge f258d84...

@bors
Copy link
Collaborator

bors commented Jan 8, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

alexcrichton commented Jan 8, 2017 via email

@bors
Copy link
Collaborator

bors commented Jan 8, 2017

⌛ Testing commit 7152bce with merge 9830f38...

bors added a commit that referenced this pull request Jan 8, 2017
Fix process module tests to run on Windows

Fixes #38565
r? @retep998
@bors
Copy link
Collaborator

bors commented Jan 8, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

alexcrichton commented Jan 8, 2017 via email

@bors
Copy link
Collaborator

bors commented Jan 8, 2017

⌛ Testing commit 7152bce with merge e350c44...

bors added a commit that referenced this pull request Jan 8, 2017
Fix process module tests to run on Windows

Fixes #38565
r? @retep998
@bors
Copy link
Collaborator

bors commented Jan 8, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing e350c44 to master...

@bors bors merged commit 7152bce into rust-lang:master Jan 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants