diff --git a/plugins/csharp/src/plugin.rs b/plugins/csharp/src/plugin.rs index 841b82609ea..1066fe3f8b8 100644 --- a/plugins/csharp/src/plugin.rs +++ b/plugins/csharp/src/plugin.rs @@ -221,7 +221,7 @@ impl LanguagePlugin for CSharpPlugin { // run command let bootstrap_path = Self::get_bootstrap_path()?; - let _output = TmcCommand::new_with_file_io("dotnet")? + let _output = TmcCommand::piped("dotnet") .with(|e| { e.cwd(path) .arg(bootstrap_path) @@ -261,7 +261,7 @@ impl LanguagePlugin for CSharpPlugin { // run command let bootstrap_path = Self::get_bootstrap_path()?; - let command = TmcCommand::new_with_file_io("dotnet")? + let command = TmcCommand::piped("dotnet") .with(|e| e.cwd(path).arg(bootstrap_path).arg("--run-tests")); let output = if let Some(timeout) = timeout { command.output_with_timeout(timeout) diff --git a/plugins/java/src/ant_plugin.rs b/plugins/java/src/ant_plugin.rs index 1c0c4341fb2..08b8340e594 100644 --- a/plugins/java/src/ant_plugin.rs +++ b/plugins/java/src/ant_plugin.rs @@ -32,11 +32,10 @@ impl AntPlugin { fn get_ant_executable(&self) -> &'static str { if cfg!(windows) { - if let Ok(command) = TmcCommand::new_with_file_io("ant") { - if let Ok(status) = command.with(|e| e.arg("-version")).status() { - if status.success() { - return "ant"; - } + let command = TmcCommand::piped("ant"); + if let Ok(status) = command.with(|e| e.arg("-version")).status() { + if status.success() { + return "ant"; } } // if ant not found on windows, try ant.bat @@ -188,7 +187,7 @@ impl JavaPlugin for AntPlugin { log::info!("building project at {}", project_root_path.display()); let ant_exec = self.get_ant_executable(); - let output = TmcCommand::new_with_file_io(ant_exec)? + let output = TmcCommand::piped(ant_exec) .with(|e| e.arg("compile-test").cwd(project_root_path)) .output()?; @@ -263,7 +262,7 @@ impl JavaPlugin for AntPlugin { } log::debug!("java args '{}' in {}", arguments.join(" "), path.display()); - let command = TmcCommand::new_with_file_io("java")?.with(|e| e.cwd(path).args(&arguments)); + let command = TmcCommand::piped("java").with(|e| e.cwd(path).args(&arguments)); let output = if let Some(timeout) = timeout { command.output_with_timeout(timeout)? } else { diff --git a/plugins/java/src/java_plugin.rs b/plugins/java/src/java_plugin.rs index 2801917a5f1..f0934eb5d66 100644 --- a/plugins/java/src/java_plugin.rs +++ b/plugins/java/src/java_plugin.rs @@ -124,7 +124,7 @@ pub(crate) trait JavaPlugin: LanguagePlugin { /// Tries to find the java.home property. fn get_java_home() -> Result { - let output = TmcCommand::new_with_file_io("java")? + let output = TmcCommand::piped("java") .with(|e| e.arg("-XshowSettings:properties").arg("-version")) .output()?; diff --git a/plugins/java/src/maven_plugin.rs b/plugins/java/src/maven_plugin.rs index ed1ad50be1a..b15de790683 100644 --- a/plugins/java/src/maven_plugin.rs +++ b/plugins/java/src/maven_plugin.rs @@ -40,7 +40,7 @@ impl MavenPlugin { // the executable used from within the extracted maven differs per platform fn get_mvn_command() -> Result { // check if mvn is in PATH - if let Ok(status) = TmcCommand::new_with_file_io("mvn")? + if let Ok(status) = TmcCommand::piped("mvn") .with(|e| e.arg("--batch-mode").arg("--version")) .status() { @@ -118,7 +118,7 @@ impl LanguagePlugin for MavenPlugin { log::info!("Cleaning maven project at {}", path.display()); let mvn_command = Self::get_mvn_command()?; - let _output = TmcCommand::new_with_file_io(mvn_command)? + let _output = TmcCommand::piped(mvn_command) .with(|e| e.cwd(path).arg("--batch-mode").arg("clean")) .output_checked()?; @@ -153,7 +153,7 @@ impl JavaPlugin for MavenPlugin { let output_arg = format!("-Dmdep.outputFile={}", class_path_file.display()); let mvn_path = Self::get_mvn_command()?; - let _output = TmcCommand::new_with_file_io(mvn_path)? + let _output = TmcCommand::piped(mvn_path) .with(|e| { e.cwd(path) .arg("--batch-mode") @@ -182,7 +182,7 @@ impl JavaPlugin for MavenPlugin { log::info!("Building maven project at {}", project_root_path.display()); let mvn_path = Self::get_mvn_command()?; - let output = TmcCommand::new_with_file_io(mvn_path)? + let output = TmcCommand::piped(mvn_path) .with(|e| { e.cwd(project_root_path) .arg("--batch-mode") @@ -209,7 +209,7 @@ impl JavaPlugin for MavenPlugin { log::info!("Running tests for maven project at {}", path.display()); let mvn_path = Self::get_mvn_command()?; - let command = TmcCommand::new_with_file_io(mvn_path)?.with(|e| { + let command = TmcCommand::piped(mvn_path).with(|e| { e.cwd(path) .arg("--batch-mode") .arg("fi.helsinki.cs.tmc:tmc-maven-plugin:1.12:test") diff --git a/plugins/make/src/plugin.rs b/plugins/make/src/plugin.rs index 99c044fe7ff..de24f329a40 100644 --- a/plugins/make/src/plugin.rs +++ b/plugins/make/src/plugin.rs @@ -86,7 +86,7 @@ impl MakePlugin { }; log::info!("Running make {}", arg); - let output = TmcCommand::new_with_file_io("make")? + let output = TmcCommand::piped("make") .with(|e| e.cwd(path).arg(arg)) .output()?; @@ -112,7 +112,7 @@ impl MakePlugin { /// the process finished successfully or not. fn build(&self, dir: &Path) -> Result { log::debug!("building {}", dir.display()); - let output = TmcCommand::new_with_file_io("make")? + let output = TmcCommand::piped("make") .with(|e| e.cwd(dir).arg("test")) .output()?; @@ -327,7 +327,7 @@ impl LanguagePlugin for MakePlugin { // does not check for success fn clean(&self, path: &Path) -> Result<(), TmcError> { - let output = TmcCommand::new_with_file_io("make")? + let output = TmcCommand::piped("make") .with(|e| e.cwd(path).arg("clean")) .output()?; diff --git a/plugins/python3/src/plugin.rs b/plugins/python3/src/plugin.rs index bd53bfcd59c..b65c7e1c8ee 100644 --- a/plugins/python3/src/plugin.rs +++ b/plugins/python3/src/plugin.rs @@ -28,7 +28,7 @@ impl Python3Plugin { Self {} } - fn get_local_python_command() -> Result { + fn get_local_python_command() -> TmcCommand { lazy_static! { // the correct python command is platform-dependent static ref LOCAL_PY: LocalPy = { @@ -62,17 +62,16 @@ impl Python3Plugin { Custom { python_exec: String }, } - let command = match &*LOCAL_PY { - LocalPy::Unix => TmcCommand::new_with_file_io("python3")?, - LocalPy::Windows => TmcCommand::new_with_file_io("py")?.with(|e| e.arg("-3")), - LocalPy::WindowsConda { conda_path } => TmcCommand::new_with_file_io(conda_path)?, - LocalPy::Custom { python_exec } => TmcCommand::new_with_file_io(python_exec)?, - }; - Ok(command) + match &*LOCAL_PY { + LocalPy::Unix => TmcCommand::piped("python3"), + LocalPy::Windows => TmcCommand::piped("py").with(|e| e.arg("-3")), + LocalPy::WindowsConda { conda_path } => TmcCommand::piped(conda_path), + LocalPy::Custom { python_exec } => TmcCommand::piped(python_exec), + } } fn get_local_python_ver() -> Result<(usize, usize, usize), PythonError> { - let output = Self::get_local_python_command()? + let output = Self::get_local_python_command() .with(|e| e.args(&["-c", "import sys; print(sys.version_info.major); print(sys.version_info.minor); print(sys.version_info.micro);"])) .output_checked()?; let stdout = String::from_utf8_lossy(&output.stdout); @@ -139,7 +138,7 @@ impl Python3Plugin { log::debug!("running tmc command at {}", path.display()); let common_args = ["-m", "tmc"]; - let command = Self::get_local_python_command()?; + let command = Self::get_local_python_command(); let command = command.with(|e| e.args(&common_args).args(extra_args).cwd(path)); let output = if let Some(timeout) = timeout { command.output_with_timeout(timeout)? @@ -446,7 +445,7 @@ mod test { fn gets_local_python_command() { init(); - let _cmd = Python3Plugin::get_local_python_command().unwrap(); + let _cmd = Python3Plugin::get_local_python_command(); } #[test] diff --git a/plugins/r/src/plugin.rs b/plugins/r/src/plugin.rs index a2c47a5d548..24abbd9e64e 100644 --- a/plugins/r/src/plugin.rs +++ b/plugins/r/src/plugin.rs @@ -45,7 +45,7 @@ impl LanguagePlugin for RPlugin { } else { &["-e", "library(tmcRtestrunner);run_available_points()"] }; - let _output = TmcCommand::new_with_file_io("Rscript")? + let _output = TmcCommand::piped("Rscript") .with(|e| e.cwd(path).args(args)) .output_checked()?; @@ -85,11 +85,11 @@ impl LanguagePlugin for RPlugin { }; if let Some(timeout) = timeout { - let _command = TmcCommand::new_with_file_io("Rscript")? + let _command = TmcCommand::piped("Rscript") .with(|e| e.cwd(path).args(args)) .output_with_timeout_checked(timeout)?; } else { - let _command = TmcCommand::new_with_file_io("Rscript")? + let _command = TmcCommand::piped("Rscript") .with(|e| e.cwd(path).args(args)) .output_checked()?; } diff --git a/tmc-langs-framework/src/command.rs b/tmc-langs-framework/src/command.rs index ef177c21c00..3f89783712a 100644 --- a/tmc-langs-framework/src/command.rs +++ b/tmc-langs-framework/src/command.rs @@ -1,36 +1,16 @@ //! Custom wrapper for Command that supports timeouts and contains custom error handling. -use crate::{ - error::{CommandError, FileIo}, - file_util, TmcError, -}; -use std::ffi::OsStr; +use crate::{error::CommandError, TmcError}; use std::fs::File; -use std::io::{Read, Seek, SeekFrom}; +use std::io::Read; use std::time::Duration; -use subprocess::{Exec, ExitStatus, PopenError}; +use std::{ffi::OsStr, thread::JoinHandle}; +use subprocess::{Exec, ExitStatus, PopenError, Redirection}; /// Wrapper around subprocess::Exec #[must_use] pub struct TmcCommand { exec: Exec, - stdout: Option, - stderr: Option, -} - -fn read_output( - file_handle: Option, - exec_output: Option<&mut File>, -) -> Result, TmcError> { - let mut v = vec![]; - if let Some(mut file) = file_handle { - let _ = file.seek(SeekFrom::Start(0)); // ignore errors - file.read_to_end(&mut v).map_err(FileIo::Generic)?; - } else if let Some(file) = exec_output { - let _ = file.seek(SeekFrom::Start(0)); // ignore errors - file.read_to_end(&mut v).map_err(FileIo::Generic)?; - } - Ok(v) } impl TmcCommand { @@ -38,31 +18,22 @@ impl TmcCommand { pub fn new(cmd: impl AsRef) -> Self { Self { exec: Exec::cmd(cmd).env("LANG", "en_US.UTF-8"), - stdout: None, - stderr: None, } } - /// Creates a new command with stdout and stderr redirected to files - pub fn new_with_file_io(cmd: impl AsRef) -> Result { - let stdout = file_util::temp_file()?; - let stderr = file_util::temp_file()?; - Ok(Self { + /// Creates a new command, defaults to piped stdout/stderr. + pub fn piped(cmd: impl AsRef) -> Self { + Self { exec: Exec::cmd(cmd) - .stdout(stdout.try_clone().map_err(FileIo::FileHandleClone)?) - .stderr(stderr.try_clone().map_err(FileIo::FileHandleClone)?) - .env("LANG", "en_US.UTF-8"), // some languages may error on UTF-8 files if the LANG variable is unset or set to some non-UTF-8 value - stdout: Some(stdout), - stderr: Some(stderr), - }) + .stdout(Redirection::Pipe) + .stderr(Redirection::Pipe) + .env("LANG", "en_US.UTF-8"), + } } /// Allows modification of the internal command without providing access to it. pub fn with(self, f: impl FnOnce(Exec) -> Exec) -> Self { - Self { - exec: f(self.exec), - ..self - } + Self { exec: f(self.exec) } } // executes the given command and collects its output @@ -70,14 +41,12 @@ impl TmcCommand { let cmd = self.exec.to_cmdline_lossy(); log::info!("executing {}", cmd); - let Self { - exec, - stdout, - stderr, - } = self; + let Self { exec } = self; // starts executing the command let mut popen = exec.popen().map_err(|e| popen_to_tmc_err(cmd.clone(), e))?; + let stdout_handle = spawn_reader(popen.stdout.take()); + let stderr_handle = spawn_reader(popen.stderr.take()); let exit_status = if let Some(timeout) = timeout { // timeout set @@ -92,8 +61,12 @@ impl TmcCommand { popen .terminate() .map_err(|e| CommandError::Terminate(cmd.clone(), e))?; - let stdout = read_output(stdout, popen.stdout.as_mut())?; - let stderr = read_output(stderr, popen.stderr.as_mut())?; + let stdout = stdout_handle + .join() + .expect("the thread should not be able to panic"); + let stderr = stderr_handle + .join() + .expect("the thread should not be able to panic"); return Err(TmcError::Command(CommandError::TimeOut { command: cmd, timeout, @@ -108,8 +81,12 @@ impl TmcCommand { }; log::info!("finished executing {}", cmd); - let stdout = read_output(stdout, popen.stdout.as_mut())?; - let stderr = read_output(stderr, popen.stderr.as_mut())?; + let stdout = stdout_handle + .join() + .expect("the thread should not be able to panic"); + let stderr = stderr_handle + .join() + .expect("the thread should not be able to panic"); // on success, log stdout trace and stderr debug // on failure if checked, log warn @@ -117,8 +94,8 @@ impl TmcCommand { if !exit_status.success() { // if checked is set, error with failed exit status if checked { - log::warn!("stdout: {}", String::from_utf8_lossy(&stdout)); - log::warn!("stderr: {}", String::from_utf8_lossy(&stderr)); + log::warn!("stdout: {}", String::from_utf8_lossy(&stdout).into_owned()); + log::warn!("stderr: {}", String::from_utf8_lossy(&stderr).into_owned()); return Err(CommandError::Failed { command: cmd, status: exit_status, @@ -127,12 +104,12 @@ impl TmcCommand { } .into()); } else { - log::debug!("stdout: {}", String::from_utf8_lossy(&stdout)); - log::debug!("stderr: {}", String::from_utf8_lossy(&stderr)); + log::debug!("stdout: {}", String::from_utf8_lossy(&stdout).into_owned()); + log::debug!("stderr: {}", String::from_utf8_lossy(&stderr).into_owned()); } } else { - log::trace!("stdout: {}", String::from_utf8_lossy(&stdout)); - log::debug!("stderr: {}", String::from_utf8_lossy(&stderr)); + log::trace!("stdout: {}", String::from_utf8_lossy(&stdout).into_owned()); + log::debug!("stderr: {}", String::from_utf8_lossy(&stderr).into_owned()); } Ok(Output { status: exit_status, @@ -167,6 +144,19 @@ impl TmcCommand { } } +// it's assumed the thread will never panic +fn spawn_reader(file: Option) -> JoinHandle> { + std::thread::spawn(move || { + if let Some(mut file) = file { + let mut buf = vec![]; + let _ = file.read_to_end(&mut buf); + buf + } else { + vec![] + } + }) +} + // convenience function to convert an error while checking for command not found error fn popen_to_tmc_err(cmd: String, err: PopenError) -> TmcError { if let PopenError::IoError(io) = &err { @@ -193,21 +183,19 @@ mod test { #[test] fn timeout() { - let cmd = TmcCommand::new_with_file_io("sleep") - .unwrap() - .with(|e| e.arg("1")); + let cmd = TmcCommand::piped("sleep").with(|e| e.arg("1")); assert!(matches!( cmd.output_with_timeout(Duration::from_nanos(1)), - Err(TmcError::Command(CommandError::TimeOut {..})) + Err(TmcError::Command(CommandError::TimeOut { .. })) )); } #[test] fn not_found() { - let cmd = TmcCommand::new_with_file_io("nonexistent command").unwrap(); + let cmd = TmcCommand::piped("nonexistent command"); assert!(matches!( cmd.output(), - Err(TmcError::Command(CommandError::NotFound {..})) + Err(TmcError::Command(CommandError::NotFound { .. })) )); } } diff --git a/tmc-langs-util/src/task_executor/course_refresher.rs b/tmc-langs-util/src/task_executor/course_refresher.rs index 54135067237..14b54c1541e 100644 --- a/tmc-langs-util/src/task_executor/course_refresher.rs +++ b/tmc-langs-util/src/task_executor/course_refresher.rs @@ -10,7 +10,7 @@ use serde::{Deserialize, Serialize}; use serde_yaml::Mapping; use std::path::{Path, PathBuf}; use std::{io::Write, time::Duration}; -use tmc_langs_framework::{command::TmcCommand, file_util, subprocess::Redirection}; +use tmc_langs_framework::{command::TmcCommand, file_util}; use walkdir::WalkDir; #[cfg(unix)] @@ -210,13 +210,8 @@ fn initialize_new_cache_clone( file_util::copy(old_clone_path, new_course_root)?; let run_git = |args: &[&str]| { - TmcCommand::new("git".to_string()) - .with(|e| { - e.cwd(new_clone_path) - .args(args) - .stdout(Redirection::Pipe) - .stderr(Redirection::Pipe) - }) + TmcCommand::piped("git".to_string()) + .with(|e| e.cwd(new_clone_path).args(args)) .output_with_timeout_checked(Duration::from_secs(60 * 2)) }; @@ -243,14 +238,12 @@ fn initialize_new_cache_clone( log::info!("could not copy from previous cache, cloning"); // clone_repository - TmcCommand::new("git".to_string()) + TmcCommand::piped("git".to_string()) .with(|e| { e.args(&["clone", "-q", "-b"]) .arg(course_git_branch) .arg(course_source_url) .arg(new_clone_path) - .stdout(Redirection::Pipe) - .stderr(Redirection::Pipe) }) .output_with_timeout_checked(Duration::from_secs(60 * 2))?; Ok(())