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

Implement Sync for `process::Command on unix and vxworks #72472

Merged
merged 1 commit into from
May 25, 2020
Merged

Implement Sync for `process::Command on unix and vxworks #72472

merged 1 commit into from
May 25, 2020

Conversation

LeSeulArtichaut
Copy link
Contributor

Closes #72387.
r? @cuviper

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2020
@cuviper
Copy link
Member

cuviper commented May 22, 2020

I think this is technically fine, but it should have a libs team member review for the public API change.

r? @dtolnay

@rust-highfive rust-highfive assigned dtolnay and unassigned cuviper May 22, 2020
dtolnay
dtolnay reviewed May 22, 2020
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

To clarify, it's already Sync on all other supported platforms?

@cuviper
Copy link
Member

cuviper commented May 22, 2020

I think so -- every other Command is "boring", just OsStrings and such. But the test change in src/libstd/process.rs will confirm that in CI.

@dtolnay
Copy link
Member

dtolnay commented May 22, 2020

@bors r+

FYI @rust-lang/libs - this adds Sync for std::process::Command on unix, which was missing for an insignificant reason. Sync was already implemented on windows and other platforms.

@bors
Copy link
Contributor

bors commented May 22, 2020

📌 Commit 01630b2 has been approved by dtolnay

@bors 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 May 22, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 25, 2020
@Dylan-DPC
Rollup merge of rust-lang#72472 - LeSeulArtichaut:sync-command, r=dto…
5a26be3 
…lnay

Implement `Sync` for `process::Command on unix and vxworks

Closes rust-lang#72387.
r? @cuviper
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request May 25, 2020
Closed
@bors
Copy link
Contributor

bors commented May 25, 2020

⌛ Testing commit 01630b2 with merge 2679c38...

@bors
Copy link
Contributor

bors commented May 25, 2020

☀️ Test successful - checks-azure
Approved by: dtolnay
Pushing 2679c38 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 25, 2020
@bors bors merged commit 2679c38 into rust-lang:master May 25, 2020
@LeSeulArtichaut LeSeulArtichaut deleted the sync-command branch February 18, 2021 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay left review comments

Assignees

@dtolnay dtolnay

Labels
merged-by-bors This PR was explicitly merged by bors. 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.

Implement Sync for process::Command
5 participants
@LeSeulArtichaut @cuviper @dtolnay @bors @rust-highfive