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.

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

Add a process_group method to UNIX CommandExt #93858

Merged
merged 1 commit into from
Mar 19, 2022

Conversation

krallin
Copy link
Contributor

@krallin krallin commented Feb 10, 2022

Add a process_group method to std::os::unix::process::CommandExt that
allows setting the process group id (i.e. calling setpgid) in the child, thus
enabling users to set process groups while leveraging the posix_spawn fast
path.

@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 @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 10, 2022
@rust-log-analyzer

This comment has been minimized.

@joshtriplett
Copy link
Member

Looks good to me!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 10, 2022

📌 Commit abb4a0e has been approved by joshtriplett

@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 Feb 10, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 11, 2022
…shtriplett

Add a `process_group` method to UNIX `CommandExt`

- Tracking issue: rust-lang#93857
- RFC: rust-lang/rfcs#3228

Add a `process_group` method to `std::os::unix::process::CommandExt` that
allows setting the process group id (i.e. calling `setpgid`) in the child, thus
enabling users to set process groups while leveraging the `posix_spawn` fast
path.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 11, 2022
…shtriplett

Add a `process_group` method to UNIX `CommandExt`

- Tracking issue: rust-lang#93857
- RFC: rust-lang/rfcs#3228

Add a `process_group` method to `std::os::unix::process::CommandExt` that
allows setting the process group id (i.e. calling `setpgid`) in the child, thus
enabling users to set process groups while leveraging the `posix_spawn` fast
path.
@matthiaskrgr
Copy link
Member

@bors r- failed in a rollup
#93919 (comment)
on auto (dist-various-2, ubuntu-20.04-xl)

error: associated function is never used: `get_pgroup`
   --> library/std/src/sys/unix/process/process_common.rs:275:12
    |
275 |     pub fn get_pgroup(&self) -> Option<pid_t> {
    |            ^^^^^^^^^^
    |
    = note: `-D dead-code` implied by `-D warnings`

[RUSTC-TIMING] std test:false 3.999
error: could not compile `std` due to previous error

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 11, 2022
@krallin
Copy link
Contributor Author

krallin commented Feb 11, 2022 via email

@krallin
Copy link
Contributor Author

krallin commented Feb 11, 2022

(updated)

@krallin
Copy link
Contributor Author

krallin commented Feb 11, 2022

Hmm, is that the right fix though?

I see the failure is on fuchsia. It looks like it only supports a subset of the operations on the UNIX CommandExt (e.g. groups doesn't work either), as per library/std/src/sys/unix/process/process_fuchsia.rs.

Is this something I should tackle here (e.g. return an error when this is set on fuschia?), or is this OK for this to behave like e.g. groups, that is, to do nothing silently?

@krallin
Copy link
Contributor Author

krallin commented Feb 16, 2022

Hey @joshtriplett , sorry to ping, but since you were very helpful so far: any guidance on how I should deal with Fuschia?

(I should mention that I definitely don't have a Fuschia machine so testing changes in that build might be tricky for me!)

@dtolnay dtolnay assigned dtolnay and unassigned Mark-Simulacrum Feb 28, 2022
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.

This looks good to me.


It looks like fuchsia only supports a subset of the operations on the UNIX CommandExt (e.g. groups doesn't work either), as per library/std/src/sys/unix/process/process_fuchsia.rs. Is this something I should tackle here (e.g. return an error when this is set on fuschia?), or is this OK for this to behave like e.g. groups, that is, to do nothing silently?

Please add an item to the tracking issue to confirm how the Fuchsia folks want to implement this prior to stabilization.

FYI @tmandry.

@dtolnay
Copy link
Member

dtolnay commented Feb 28, 2022

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Feb 28, 2022

📌 Commit 6d7d6e1 has been approved by dtolnay

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 28, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 2, 2022
@bors
Copy link
Contributor

bors commented Mar 2, 2022

⌛ Testing commit 6d7d6e1 with merge 1474108e2fc91238a61b927275659a9a013ee202...

@bors
Copy link
Contributor

bors commented Mar 3, 2022

💥 Test timed out

@bors 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 Mar 3, 2022
@rust-log-analyzer

This comment has been minimized.

@krallin
Copy link
Contributor Author

krallin commented Mar 3, 2022

Looking at the logs I think the timeouts might be legit, since I can see this in the armhf-gnu, ubuntu-20.04-xl build:

test sys::unix::process::process_common::tests::test_process_group_no_posix_spawn has been running for over 60 seconds
test sys::unix::process::process_common::tests::test_process_group_posix_spawn has been running for over 60 seconds

Any suggestions for how I might repro this? My initial guess would be that the platform this is running on doesn't set the SIGINIT handler to kill by default, so I'd be inclined to update the test to send SIGKILL instead of SIGINT, but curious if there is a way I could check that works?

Thanks!

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 9, 2022
Add a team for '@rustbot ping fuchsia'

I was looking for this in response to rust-lang#93858 (comment).

Depends on rust-lang/team#710.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 9, 2022
Add a team for '@rustbot ping fuchsia'

I was looking for this in response to rust-lang#93858 (comment).

Depends on rust-lang/team#710.
@nikic
Copy link
Contributor

nikic commented Mar 9, 2022

@bors r- accidentally got back in the queue

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 9, 2022
@niklasf
Copy link
Contributor

niklasf commented Mar 12, 2022

One of the surrounding test cases is preceded by

#[cfg_attr(
any(
// See #14232 for more information, but it appears that signal delivery to a
// newly spawned process may just be raced in the macOS, so to prevent this
// test from being flaky we ignore it on macOS.
target_os = "macos",
// When run under our current QEMU emulation test suite this test fails,
// although the reason isn't very clear as to why. For now this test is
// ignored there.
target_arch = "arm",
target_arch = "aarch64",
target_arch = "riscv64",
),
ignore
)]

which was originally added together with that CI target (1747ce2).

So unless we can figure out the mysterious issue with signals under QEMU in CI, it's probably best to move along and apply the same attributes to the new tests.

@krallin
Copy link
Contributor Author

krallin commented Mar 14, 2022

Thanks @niklasf, I updated this accordingly :)

@dtolnay
Copy link
Member

dtolnay commented Mar 18, 2022

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Mar 18, 2022

📌 Commit b628497 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 18, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#93858 (Add a `process_group` method to UNIX `CommandExt`)
 - rust-lang#94650 (Relax tests for Windows dos device names)
 - rust-lang#94991 (Make Weak::new const)
 - rust-lang#95072 (Re-enable parallel debuginfo tests)
 - rust-lang#95109 (Extend --check-cfg tests to all predicate inside all/any)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3545003 into rust-lang:master Mar 19, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.