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 conversion from tokio::process::Command to std::process::Command #7001

Closed
czy-29 opened this issue Dec 2, 2024 · 5 comments · Fixed by #7014
Closed

Add conversion from tokio::process::Command to std::process::Command #7001

czy-29 opened this issue Dec 2, 2024 · 5 comments · Fixed by #7014
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-process Module: tokio/process

Comments

@czy-29
Copy link
Contributor

czy-29 commented Dec 2, 2024

Is your feature request related to a problem? Please describe.
The reverse conversion currently exists, but the conversion from tokio to std (with ownership) does not currently exist.
The as_std and as_std_mut methods only return references, and because std::process::Command is not Clone, its ownership cannot be moved out through simple dereference. Therefore, a separate conversion method which can move out ownership is needed.

Describe the solution you'd like

impl From<Command> for StdCommand {
    fn from(cmd: Command) -> StdCommand {
        cmd.std
    }
}

Describe alternatives you've considered
Or some named methods such as:

pub fn into_std(self) -> StdCommand {
    self.std
}
@czy-29 czy-29 added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Dec 2, 2024
@czy-29
Copy link
Contributor Author

czy-29 commented Dec 2, 2024

Since this feature is relatively easy to implement, opening this issue is mainly to initiate some discussions and see if there are any reasons that I have not considered that actually make this feature unnecessary. We can also discuss which solution we choose to implement it with. If everything is clear, then I am willing to directly submit an associated PR.

@Darksonn Darksonn added the M-process Module: tokio/process label Dec 3, 2024
@Darksonn
Copy link
Contributor

Darksonn commented Dec 3, 2024

Thoughts @ipetkov ?

Since we already have as_std_mut this may be okay ... but the tokio::process::Command type may have additional options that are not reflected in the std::process::Command type. Right now it's only the kill_on_drop boolean, though.

@ipetkov
Copy link
Member

ipetkov commented Dec 3, 2024

I suppose it would be fine. Most of the tokio-specific logic happens on spawn (and not before we forward calls to the std version) and since we already expose as_std_mut we might as well support unwrapping it. Yeah you'd lose the tokio-specific data, but I don't imagine the caller would care too much if they're interested in the std type (or else they'd be working with the tokio version directly)

@czy-29
Copy link
Contributor Author

czy-29 commented Dec 4, 2024

I suppose it would be fine. Most of the tokio-specific logic happens on spawn (and not before we forward calls to the std version) and since we already expose as_std_mut we might as well support unwrapping it. Yeah you'd lose the tokio-specific data, but I don't imagine the caller would care too much if they're interested in the std type (or else they'd be working with the tokio version directly)

Which signature do you expect to implement?

  1. impl From<Command> for StdCommand
  2. pub fn into_std(self) -> StdCommand
  3. pub fn unwrap_std(self) -> StdCommand

@ipetkov
Copy link
Member

ipetkov commented Dec 5, 2024

We can start with just into_std since we already have precedent for that style of API

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 C-feature-request Category: A feature request. M-process Module: tokio/process
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants