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

command: Add process_group method to Command struct #5114

Merged
merged 2 commits into from Oct 29, 2022

Conversation

HarveyHunt
Copy link
Contributor

Rust 1.64 stabilised the process_group method of the std::os::unix::process::CommandExt trait. This allows the caller to set the process group that a command is spawned in.

Expose that functionality from Tokio's Command structure.

This fixes #4312.


I'm not really familiar with the process for adding a new Tokio method, so I've gated it behind the tokio_unstable check - please let me know if I should be doing it differently. Thanks!

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-process Module: tokio/process labels Oct 21, 2022
@HarveyHunt HarveyHunt force-pushed the command-process-group branch 2 times, most recently from c4edb3a to c52b0cc Compare October 21, 2022 14:43
@HarveyHunt
Copy link
Contributor Author

It looks like the FreeBSD doc tests are failing because the nightly version used there is old, whilst the other FreeBSD tests are using stable.

Nightly is 1.61.0 (logs)
Stable is 1.64.0 (logs)

The Cirrus CI config is hardcoding an old nightly version - perhaps that could be updated?

https://github.com/tokio-rs/tokio/blob/master/.cirrus.yml#L5

@ipetkov
Copy link
Member

ipetkov commented Oct 22, 2022

Doc tests are also failing, I would be okay to remove that example as it's pretty simplistic anyway

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

I don't mind adding this. Bumping the nightly version in CI is perfectly fine.

Comment on lines +693 to +696
/// Sets the process group ID (PGID) of the child process. Equivalent to a
/// setpgid call in the child process, but may be more efficient.
///
/// Process groups determine which processes receive signals.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should document that it is unstable, how to enable unstable features (a link is fine - see doc for other unstable methods), and that it will become stable when our MSRV is new enough to allow this in a stable manner. It would also be nice to mention that you can do this today by constructing an std::process::Command and using our From impl to convert it.

@carllerche
Copy link
Member

I'm fine adding it as an unstable feature w/ a comment saying to "stabilize" it when possible.

@HarveyHunt
Copy link
Contributor Author

Thanks for the feedback, I've updated the original commit to include some more documentation.

I've also added another commit that bumps the Cirrus CI version - please let me know if you'd prefer that in a separate PR.

///
/// If you want similar behaviour without using this unstable feature you can
/// create a [`std::process::Command`] and convert that into a
/// [`crate::process::Command`] using the `From` trait.
Copy link
Contributor

@Darksonn Darksonn Oct 26, 2022

Choose a reason for hiding this comment

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

We don't use crate in our documentation.

Suggested change
/// [`crate::process::Command`] using the `From` trait.
/// [`tokio::process::Command`] using the `From` trait.

If this link doesn't work out of the box, then you can add

[`tokio::process::Command`]: crate::process::Command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks for the pointer about the alias - I tried tokio::process::Command earlier and the docs wouldn't build. With that alias, it works. :)

@Darksonn
Copy link
Contributor

I've also added another commit that bumps the Cirrus CI version - please let me know if you'd prefer that in a separate PR.

If it also requires a bunch of unrelated fixes of warnings, then I'd prefer a separate PR. Otherwise it is fine here.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Lgtm

@Darksonn
Copy link
Contributor

You'll need to fix the CI failure. Otherwise, I'm ready to merge this.

The nightly version used by Cirrus CI is a few months old now. Update it to a
recent nightly so that features relying on 1.64.0 can be landed.

Further, fix up a doc generation warning that causes the FreeBSD doc CI to fail.
Rust 1.64 stabilised the `process_group` method of the
`std::os::unix::process::CommandExt` trait. This allows the
caller to set the process group that a command is spawned in.

Expose that functionality from Tokio's `Command` structure,
behind the unstable feature.

This fixes tokio-rs#4312.
@github-actions github-actions bot added the R-loom Run loom tests on this PR label Oct 27, 2022
@HarveyHunt
Copy link
Contributor Author

Thanks for accepting! I've fixed up the warning in the doc test so this change should be good to merge now. :)

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thanks.

@Darksonn Darksonn enabled auto-merge (squash) October 29, 2022 14:26
@Darksonn Darksonn merged commit 1ab80ba into tokio-rs:master Oct 29, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Nov 22, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.21.2` -> `1.22.0` |
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.21.2` -> `1.22.0` |

---

### Release Notes

<details>
<summary>tokio-rs/tokio</summary>

### [`v1.22.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.22.0): Tokio v1.22.0

[Compare Source](tokio-rs/tokio@tokio-1.21.2...tokio-1.22.0)

##### Added

-   runtime: add `Handle::runtime_flavor` ([#&#8203;5138])
-   sync: add `Mutex::blocking_lock_owned` ([#&#8203;5130])
-   sync: add `Semaphore::MAX_PERMITS` ([#&#8203;5144])
-   sync: add `merge()` to semaphore permits ([#&#8203;4948])
-   sync: add `mpsc::WeakUnboundedSender` ([#&#8203;5189])

##### Added (unstable)

-   process: add `Command::process_group` ([#&#8203;5114])
-   runtime: export metrics about the blocking thread pool ([#&#8203;5161])
-   task: add `task::id()` and `task::try_id()` ([#&#8203;5171])

##### Fixed

-   macros: don't take ownership of futures in macros ([#&#8203;5087])
-   runtime: fix Stacked Borrows violation in `LocalOwnedTasks` ([#&#8203;5099])
-   runtime: mitigate ABA with 32-bit queue indices when possible ([#&#8203;5042])
-   task: wake local tasks to the local queue when woken by the same thread ([#&#8203;5095])
-   time: panic in release mode when `mark_pending` called illegally ([#&#8203;5093])
-   runtime: fix typo in expect message ([#&#8203;5169])
-   runtime: fix `unsync_load` on atomic types ([#&#8203;5175])
-   task: elaborate safety comments in task deallocation ([#&#8203;5172])
-   runtime: fix `LocalSet` drop in thread local ([#&#8203;5179])
-   net: remove libc type leakage in a public API ([#&#8203;5191])
-   runtime: update the alignment of `CachePadded` ([#&#8203;5106])

##### Changed

-   io: make `tokio::io::copy` continue filling the buffer when writer stalls ([#&#8203;5066])
-   runtime: remove `coop::budget` from `LocalSet::run_until` ([#&#8203;5155])
-   sync: make `Notify` panic safe ([#&#8203;5154])

##### Documented

-   io: fix doc for `write_i8` to use signed integers ([#&#8203;5040])
-   net: fix doc typos for TCP and UDP `set_tos` methods ([#&#8203;5073])
-   net: fix function name in `UdpSocket::recv` documentation ([#&#8203;5150])
-   sync: typo in `TryLockError` for `RwLock::try_write` ([#&#8203;5160])
-   task: document that spawned tasks execute immediately ([#&#8203;5117])
-   time: document return type of `timeout` ([#&#8203;5118])
-   time: document that `timeout` checks only before poll ([#&#8203;5126])
-   sync: specify return type of `oneshot::Receiver` in docs ([#&#8203;5198])

##### Internal changes

-   runtime: use const `Mutex::new` for globals ([#&#8203;5061])
-   runtime: remove `Option` around `mio::Events` in io driver ([#&#8203;5078])
-   runtime: remove a conditional compilation clause ([#&#8203;5104])
-   runtime: remove a reference to internal time handle ([#&#8203;5107])
-   runtime: misc time driver cleanup ([#&#8203;5120])
-   runtime: move signal driver to runtime module ([#&#8203;5121])
-   runtime: signal driver now uses I/O driver directly ([#&#8203;5125])
-   runtime: start decoupling I/O driver and I/O handle ([#&#8203;5127])
-   runtime: switch `io::handle` refs with scheduler:Handle ([#&#8203;5128])
-   runtime: remove Arc from I/O driver ([#&#8203;5134])
-   runtime: use signal driver handle via `scheduler::Handle` ([#&#8203;5135])
-   runtime: move internal clock fns out of context ([#&#8203;5139])
-   runtime: remove `runtime::context` module ([#&#8203;5140])
-   runtime: keep driver cfgs in `driver.rs` ([#&#8203;5141])
-   runtime: add `runtime::context` to unify thread-locals ([#&#8203;5143])
-   runtime: rename some confusing internal variables/fns ([#&#8203;5151])
-   runtime: move `coop` mod into `runtime` ([#&#8203;5152])
-   runtime: move budget state to context thread-local ([#&#8203;5157])
-   runtime: move park logic into runtime module ([#&#8203;5158])
-   runtime: move `Runtime` into its own file ([#&#8203;5159])
-   runtime: unify entering a runtime with `Handle::enter` ([#&#8203;5163])
-   runtime: remove handle reference from each scheduler ([#&#8203;5166])
-   runtime: move `enter` into `context` ([#&#8203;5167])
-   runtime: combine context and entered thread-locals ([#&#8203;5168])
-   runtime: fix accidental unsetting of current handle ([#&#8203;5178])
-   runtime: move `CoreStage` methods to `Core` ([#&#8203;5182])
-   sync: name mpsc semaphore types ([#&#8203;5146])

[#&#8203;4948]: tokio-rs/tokio#4948

[#&#8203;5040]: tokio-rs/tokio#5040

[#&#8203;5042]: tokio-rs/tokio#5042

[#&#8203;5061]: tokio-rs/tokio#5061

[#&#8203;5066]: tokio-rs/tokio#5066

[#&#8203;5073]: tokio-rs/tokio#5073

[#&#8203;5078]: tokio-rs/tokio#5078

[#&#8203;5087]: tokio-rs/tokio#5087

[#&#8203;5093]: tokio-rs/tokio#5093

[#&#8203;5095]: tokio-rs/tokio#5095

[#&#8203;5099]: tokio-rs/tokio#5099

[#&#8203;5104]: tokio-rs/tokio#5104

[#&#8203;5106]: tokio-rs/tokio#5106

[#&#8203;5107]: tokio-rs/tokio#5107

[#&#8203;5114]: tokio-rs/tokio#5114

[#&#8203;5117]: tokio-rs/tokio#5117

[#&#8203;5118]: tokio-rs/tokio#5118

[#&#8203;5120]: tokio-rs/tokio#5120

[#&#8203;5121]: tokio-rs/tokio#5121

[#&#8203;5125]: tokio-rs/tokio#5125

[#&#8203;5126]: tokio-rs/tokio#5126

[#&#8203;5127]: tokio-rs/tokio#5127

[#&#8203;5128]: tokio-rs/tokio#5128

[#&#8203;5130]: tokio-rs/tokio#5130

[#&#8203;5134]: tokio-rs/tokio#5134

[#&#8203;5135]: tokio-rs/tokio#5135

[#&#8203;5138]: tokio-rs/tokio#5138

[#&#8203;5138]: tokio-rs/tokio#5138

[#&#8203;5139]: tokio-rs/tokio#5139

[#&#8203;5140]: tokio-rs/tokio#5140

[#&#8203;5141]: tokio-rs/tokio#5141

[#&#8203;5143]: tokio-rs/tokio#5143

[#&#8203;5144]: tokio-rs/tokio#5144

[#&#8203;5144]: tokio-rs/tokio#5144

[#&#8203;5146]: tokio-rs/tokio#5146

[#&#8203;5150]: tokio-rs/tokio#5150

[#&#8203;5151]: tokio-rs/tokio#5151

[#&#8203;5152]: tokio-rs/tokio#5152

[#&#8203;5154]: tokio-rs/tokio#5154

[#&#8203;5155]: tokio-rs/tokio#5155

[#&#8203;5157]: tokio-rs/tokio#5157

[#&#8203;5158]: tokio-rs/tokio#5158

[#&#8203;5159]: tokio-rs/tokio#5159

[#&#8203;5160]: tokio-rs/tokio#5160

[#&#8203;5161]: tokio-rs/tokio#5161

[#&#8203;5163]: tokio-rs/tokio#5163

[#&#8203;5166]: tokio-rs/tokio#5166

[#&#8203;5167]: tokio-rs/tokio#5167

[#&#8203;5168]: tokio-rs/tokio#5168

[#&#8203;5169]: tokio-rs/tokio#5169

[#&#8203;5171]: tokio-rs/tokio#5171

[#&#8203;5172]: tokio-rs/tokio#5172

[#&#8203;5175]: tokio-rs/tokio#5175

[#&#8203;5178]: tokio-rs/tokio#5178

[#&#8203;5179]: tokio-rs/tokio#5179

[#&#8203;5182]: tokio-rs/tokio#5182

[#&#8203;5189]: tokio-rs/tokio#5189

[#&#8203;5191]: tokio-rs/tokio#5191

[#&#8203;5198]: tokio-rs/tokio#5198

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNy4xIiwidXBkYXRlZEluVmVyIjoiMzQuMjkuMiJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1651
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@cablehead
Copy link

cablehead commented Oct 13, 2023

Hi, just checking if this feature still needs to be gated behind tokio_unstable? Thank you for all your work!

@niklasf
Copy link
Contributor

niklasf commented Oct 13, 2023

Yes. The standard library function was added in Rust 1.64.0, but tokio is at MSRV 1.63.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-process Module: tokio/process R-loom Run loom tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a tokio::process::Child in a new process group
6 participants