From 9ccbee2583c9974cc475ccae19b3faf57b916670 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Mon, 29 Jan 2024 13:44:27 -0800 Subject: [PATCH 1/2] fix: forward cwd to pty if none set --- crates/turborepo-lib/src/process/command.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/turborepo-lib/src/process/command.rs b/crates/turborepo-lib/src/process/command.rs index 04b694d49f0b6..55be566c63183 100644 --- a/crates/turborepo-lib/src/process/command.rs +++ b/crates/turborepo-lib/src/process/command.rs @@ -152,6 +152,11 @@ impl From for portable_pty::CommandBuilder { cmd.args(args); if let Some(cwd) = cwd { cmd.cwd(cwd.as_std_path()); + } else if let Ok(cwd) = std::env::current_dir() { + // portably_pty defaults to a users home directory instead of cwd if one isn't + // configured on the command builder. + // We explicitly set the cwd if one exists to avoid this behavior + cmd.cwd(&cwd); } for (key, value) in env { cmd.env(key, value); From 4309cafc134d24f9fcbcdbdab6be98212dd00be0 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Mon, 29 Jan 2024 14:00:56 -0800 Subject: [PATCH 2/2] fix: switch process manager tests to explicitly not use pty --- crates/turborepo-lib/src/process/mod.rs | 55 +++++++++++++++---------- crates/turborepo-lib/src/run/mod.rs | 2 +- 2 files changed, 34 insertions(+), 23 deletions(-) diff --git a/crates/turborepo-lib/src/process/mod.rs b/crates/turborepo-lib/src/process/mod.rs index fbeac976dadbe..c7a8b510a8d15 100644 --- a/crates/turborepo-lib/src/process/mod.rs +++ b/crates/turborepo-lib/src/process/mod.rs @@ -30,7 +30,10 @@ pub use self::child::{Child, ChildExit}; /// using `spawn`. When the manager is Closed, all currently-running children /// will be closed, and no new children can be spawned. #[derive(Debug, Clone)] -pub struct ProcessManager(Arc>); +pub struct ProcessManager { + state: Arc>, + use_pty: bool, +} #[derive(Debug)] struct ProcessManagerInner { @@ -39,11 +42,22 @@ struct ProcessManagerInner { } impl ProcessManager { - pub fn new() -> Self { - Self(Arc::new(Mutex::new(ProcessManagerInner { - is_closing: false, - children: Vec::new(), - }))) + pub fn new(use_pty: bool) -> Self { + Self { + state: Arc::new(Mutex::new(ProcessManagerInner { + is_closing: false, + children: Vec::new(), + })), + use_pty, + } + } + + /// Construct a process manager and infer if pty should be used + pub fn infer() -> Self { + // Only use PTY if we're not on windows and we're currently hooked up to a + // in a TTY + let use_pty = !cfg!(windows) && atty::is(atty::Stream::Stdout); + Self::new(use_pty) } } @@ -61,17 +75,14 @@ impl ProcessManager { command: Command, stop_timeout: Duration, ) -> Option> { - let mut lock = self.0.lock().unwrap(); + let mut lock = self.state.lock().unwrap(); if lock.is_closing { return None; } - // Only use PTY if we're not on windows and we're currently hooked up to a - // in a TTY - let use_pty = !cfg!(windows) && atty::is(atty::Stream::Stdout); let child = child::Child::spawn( command, child::ShutdownStyle::Graceful(stop_timeout), - use_pty, + self.use_pty, ); if let Ok(child) = &child { lock.children.push(child.clone()); @@ -108,7 +119,7 @@ impl ProcessManager { let mut set = JoinSet::new(); { - let mut lock = self.0.lock().expect("not poisoned"); + let mut lock = self.state.lock().expect("not poisoned"); lock.is_closing = true; for child in lock.children.iter() { let child = child.clone(); @@ -123,7 +134,7 @@ impl ProcessManager { } { - let mut lock = self.0.lock().expect("not poisoned"); + let mut lock = self.state.lock().expect("not poisoned"); // just allocate a new vec rather than clearing the old one lock.children = vec![]; @@ -155,7 +166,7 @@ mod test { #[tokio::test] async fn test_basic() { - let manager = ProcessManager::new(); + let manager = ProcessManager::new(false); let mut child = manager .spawn(get_script_command("hello_world.js"), Duration::from_secs(2)) .unwrap() @@ -168,7 +179,7 @@ mod test { #[tokio::test] async fn test_multiple() { - let manager = ProcessManager::new(); + let manager = ProcessManager::new(false); let children = (0..2) .map(|_| { @@ -191,7 +202,7 @@ mod test { #[tokio::test] async fn test_closed() { - let manager = ProcessManager::new(); + let manager = ProcessManager::new(false); let mut child = manager .spawn(get_command(), Duration::from_secs(2)) .unwrap() @@ -218,7 +229,7 @@ mod test { #[tokio::test] async fn test_exit_code() { - let manager = ProcessManager::new(); + let manager = ProcessManager::new(false); let mut child = manager .spawn(get_script_command("hello_world.js"), Duration::from_secs(2)) .unwrap() @@ -235,7 +246,7 @@ mod test { #[tokio::test] #[traced_test] async fn test_message_after_stop() { - let manager = ProcessManager::new(); + let manager = ProcessManager::new(false); let mut child = manager .spawn(get_script_command("hello_world.js"), Duration::from_secs(2)) .unwrap() @@ -258,14 +269,14 @@ mod test { #[tokio::test] async fn test_reuse_manager() { - let manager = ProcessManager::new(); + let manager = ProcessManager::new(false); manager.spawn(get_command(), Duration::from_secs(2)); sleep(Duration::from_millis(100)).await; manager.stop().await; - assert!(manager.0.lock().unwrap().children.is_empty()); + assert!(manager.state.lock().unwrap().children.is_empty()); // TODO: actually do some check that this is idempotent // idempotent @@ -280,7 +291,7 @@ mod test { script: &str, expected: Option, ) { - let manager = ProcessManager::new(); + let manager = ProcessManager::new(false); let tasks = FuturesUnordered::new(); for _ in 0..10 { @@ -315,7 +326,7 @@ mod test { #[tokio::test] async fn test_wait_multiple_tasks() { - let manager = ProcessManager::new(); + let manager = ProcessManager::new(false); let mut out = Vec::new(); let mut child = manager diff --git a/crates/turborepo-lib/src/run/mod.rs b/crates/turborepo-lib/src/run/mod.rs index 6d26149028670..3b0c949246620 100644 --- a/crates/turborepo-lib/src/run/mod.rs +++ b/crates/turborepo-lib/src/run/mod.rs @@ -69,7 +69,7 @@ pub struct Run { impl Run { pub fn new(base: CommandBase, api_auth: Option) -> Result { - let processes = ProcessManager::new(); + let processes = ProcessManager::infer(); let mut opts: Opts = base.args().try_into()?; let config = base.config()?; let is_linked = turborepo_api_client::is_linked(&api_auth);