diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index 3392b9a038..90a08ed22f 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -6,7 +6,6 @@ use nu_protocol::did_you_mean; use nu_protocol::engine::{EngineState, Stack}; use nu_protocol::{ast::Call, engine::Command, ShellError, Signature, SyntaxShape, Value}; use nu_protocol::{Category, Example, ListStream, PipelineData, RawStream, Span, Spanned}; -use nu_system::ForegroundProcess; use pathdiff::diff_paths; use std::collections::HashMap; use std::io::{BufRead, BufReader, Write}; @@ -121,7 +120,7 @@ impl ExternalCommand { let ctrlc = engine_state.ctrlc.clone(); - let mut fg_process = ForegroundProcess::new(self.create_process(&input, false, head)?); + let mut process = self.create_process(&input, false, head)?; let child; #[cfg(windows)] @@ -140,10 +139,9 @@ impl ExternalCommand { .iter() .any(|&cmd| command_name_upper == cmd); - match fg_process.spawn() { + match process.spawn() { Err(_) => { - let mut fg_process = - ForegroundProcess::new(self.create_process(&input, use_cmd, head)?); + let mut fg_process = self.create_process(&input, use_cmd, head)?; child = fg_process.spawn(); } Ok(process) => { @@ -154,7 +152,7 @@ impl ExternalCommand { #[cfg(not(windows))] { - child = fg_process.spawn() + child = process.spawn() } match child { @@ -197,7 +195,7 @@ impl ExternalCommand { engine_state.config.use_ansi_coloring = false; // if there is a string or a stream, that is sent to the pipe std - if let Some(mut stdin_write) = child.as_mut().stdin.take() { + if let Some(mut stdin_write) = child.stdin.take() { std::thread::spawn(move || { let input = crate::Table::run( &crate::Table, @@ -238,7 +236,7 @@ impl ExternalCommand { // and we create a ListStream that can be consumed if redirect_stderr { - let stderr = child.as_mut().stderr.take().ok_or_else(|| { + let stderr = child.stderr.take().ok_or_else(|| { ShellError::ExternalCommand( "Error taking stderr from external".to_string(), "Redirects need access to stderr of an external command" @@ -277,7 +275,7 @@ impl ExternalCommand { } if redirect_stdout { - let stdout = child.as_mut().stdout.take().ok_or_else(|| { + let stdout = child.stdout.take().ok_or_else(|| { ShellError::ExternalCommand( "Error taking stdout from external".to_string(), "Redirects need access to stdout of an external command" @@ -315,7 +313,7 @@ impl ExternalCommand { } } - match child.as_mut().wait() { + match child.wait() { Err(err) => Err(ShellError::ExternalCommand( "External command exited with error".into(), err.to_string(), diff --git a/crates/nu-system/src/foreground.rs b/crates/nu-system/src/foreground.rs deleted file mode 100644 index 05da611f92..0000000000 --- a/crates/nu-system/src/foreground.rs +++ /dev/null @@ -1,111 +0,0 @@ -use std::process::{Child, Command}; - -/// A simple wrapper for `std::process::Command` -/// -/// ## spawn behavior -/// ### Unix -/// When invoke `spawn`, current process will ignore `SIGTTOU`, `SIGTTIN` signal, spawned child process will get it's -/// own process group id, and it's going to foreground(by making stdin belong's to child's process group). -/// -/// When child is to over, `SIGTTOU`, `SIGTTIN` signal will be reset, foreground process is back to callers' process. -/// -/// ### Windows -/// It does nothing special on windows system, `spawn` is the same as [std::process::Command::spawn](std::process::Command::spawn) -pub struct ForegroundProcess { - inner: Command, -} - -/// A simple wrapper for `std::process::Child` -/// -/// It can only be created by `ForegroundProcess::spawn`. -pub struct ForegroundChild { - inner: Child, -} - -impl ForegroundProcess { - pub fn new(cmd: Command) -> Self { - Self { inner: cmd } - } - - pub fn spawn(&mut self) -> std::io::Result { - fg_process_setup::prepare_to_foreground(&mut self.inner); - self.inner.spawn().map(|child| { - fg_process_setup::set_foreground(&child); - ForegroundChild { inner: child } - }) - } -} - -impl AsMut for ForegroundChild { - fn as_mut(&mut self) -> &mut Child { - &mut self.inner - } -} - -impl Drop for ForegroundChild { - fn drop(&mut self) { - // It's ok to use here because we have called `set_foreground` during creation. - unsafe { fg_process_setup::reset_foreground_id() } - } -} - -// It's a simpler version of fish shell's external process handling. -// -// For more information, please check `child_setup_process` function in fish shell. -// https://github.com/fish-shell/fish-shell/blob/3f90efca38079922b4b21707001d7bb9630107eb/src/postfork.cpp#L140 -#[cfg(target_family = "unix")] -mod fg_process_setup { - use std::os::unix::prelude::CommandExt; - pub(super) fn prepare_to_foreground(external_command: &mut std::process::Command) { - unsafe { - libc::signal(libc::SIGTTOU, libc::SIG_IGN); - libc::signal(libc::SIGTTIN, libc::SIG_IGN); - - // Safety: - // POSIX only allows async-signal-safe functions to be called. - // And `setpgid` is async-signal-safe function according to: - // https://manpages.ubuntu.com/manpages/bionic/man7/signal-safety.7.html - // So we're ok to invoke `libc::setpgid` inside `pre_exec`. - external_command.pre_exec(|| { - // make the command startup with new process group. - // The process group id must be the same as external commands' pid. - // Or else we'll failed to set it as foreground process. - // For more information, check `fork_child_for_process` function: - // https://github.com/fish-shell/fish-shell/blob/023042098396aa450d2c4ea1eb9341312de23126/src/exec.cpp#L398 - libc::setpgid(0, 0); - Ok(()) - }); - } - } - - // If `prepare_to_foreground` function is not called, the function will fail with silence and do nothing. - pub(super) fn set_foreground(process: &std::process::Child) { - // it's ok to use unsafe here - // the implementaion here is just the same as - // https://docs.rs/nix/latest/nix/unistd/fn.tcsetpgrp.html, which is a safe function. - unsafe { - libc::tcsetpgrp(libc::STDIN_FILENO, process.id() as i32); - } - } - - /// Reset foreground to current process, and reset back `SIGTTOU`, `SIGTTIN` single handler. - /// - /// ## Safety - /// It can only be called when you have called `set_foreground`, or results in undefined behavior. - pub(super) unsafe fn reset_foreground_id() { - libc::tcsetpgrp(libc::STDIN_FILENO, libc::getpgrp()); - libc::signal(libc::SIGTTOU, libc::SIG_DFL); - libc::signal(libc::SIGTTIN, libc::SIG_DFL); - } -} - -// TODO: investigate if we can set foreground process through windows system call. -#[cfg(target_family = "windows")] -mod fg_process_setup { - - pub(super) fn prepare_to_foreground(_external_command: &mut std::process::Command) {} - - pub(super) fn set_foreground(_process: &std::process::Child) {} - - pub(super) unsafe fn reset_foreground_id() {} -} diff --git a/crates/nu-system/src/lib.rs b/crates/nu-system/src/lib.rs index 2283a94b78..1ad89d73d5 100644 --- a/crates/nu-system/src/lib.rs +++ b/crates/nu-system/src/lib.rs @@ -1,4 +1,3 @@ -mod foreground; #[cfg(any(target_os = "android", target_os = "linux"))] mod linux; #[cfg(target_os = "macos")] @@ -6,7 +5,6 @@ mod macos; #[cfg(target_os = "windows")] mod windows; -pub use self::foreground::{ForegroundChild, ForegroundProcess}; #[cfg(any(target_os = "android", target_os = "linux"))] pub use self::linux::*; #[cfg(target_os = "macos")]