refactor: Cleaner separation of OS-specific code (#459)

Fixes some warnings on Windows

.github/workflows/tests.yml

@@ -197,6 +197,7 @@ jobs:
         run: >
           cargo mutants --no-shuffle -vV --in-diff git.diff
           --test-tool=${{matrix.test_tool}} --timeout=500 --build-timeout=500
+          --exclude=windows.rs --exclude=console.rs
       - name: Archive mutants.out
         uses: actions/upload-artifact@v4
         if: always()
@@ -234,7 +235,8 @@ jobs:
         run: >
           cargo mutants --no-shuffle -vV --shard ${{ matrix.shard }}/10
           --test-tool ${{ matrix.test_tool }} --baseline=skip --timeout=500
-          --build-timeout=500 --in-place
+          --build-timeout=500 --in-place --exclude=windows.rs
+          --exclude=console.rs
       - name: Archive mutants.out
         uses: actions/upload-artifact@v4
         if: always()

src/cargo.rs

@@ -16,7 +16,7 @@ use crate::options::{Options, TestTool};
 use crate::outcome::{Phase, PhaseResult};
 use crate::output::ScenarioOutput;
 use crate::package::PackageSelection;
-use crate::process::{Process, ProcessStatus};
+use crate::process::{Exit, Process};
 use crate::Result;
 
 /// Run cargo build, check, or test.
@@ -56,7 +56,7 @@ pub fn run_cargo(
     )?;
     check_interrupted()?;
     debug!(?process_status, elapsed = ?start.elapsed());
-    if let ProcessStatus::Failure(code) = process_status {
+    if let Exit::Failure(code) = process_status {
         // 100 "one or more tests failed" from <https://docs.rs/nextest-metadata/latest/nextest_metadata/enum.NextestExitCode.html>;
         // I'm not addind a dependency to just get one integer.
         if argv[1] == "nextest" && code != 100 {

src/outcome.rs

@@ -14,7 +14,7 @@ use serde::Serializer;
 use tracing::warn;
 
 use crate::console::plural;
-use crate::process::ProcessStatus;
+use crate::process::Exit;
 use crate::*;
 
 /// What phase of running a scenario.
@@ -193,7 +193,7 @@ impl ScenarioOutcome {
         self.phase_results.last().unwrap().phase
     }
 
-    pub fn last_phase_result(&self) -> ProcessStatus {
+    pub fn last_phase_result(&self) -> Exit {
         self.phase_results.last().unwrap().process_status
     }
 
@@ -284,7 +284,7 @@ pub struct PhaseResult {
     /// How long did it take?
     pub duration: Duration,
     /// Did it succeed?
-    pub process_status: ProcessStatus,
+    pub process_status: Exit,
     /// What command was run, as an argv list.
     pub argv: Vec<String>,
 }
@@ -324,7 +324,7 @@ pub enum SummaryOutcome {
 mod test {
     use std::time::Duration;
 
-    use crate::process::ProcessStatus;
+    use crate::process::Exit;
 
     use super::{Phase, PhaseResult, Scenario, ScenarioOutcome};
 
@@ -339,13 +339,13 @@ mod test {
                 PhaseResult {
                     phase: Phase::Build,
                     duration: Duration::from_secs(2),
-                    process_status: ProcessStatus::Success,
+                    process_status: Exit::Success,
                     argv: vec!["cargo".into(), "build".into()],
                 },
                 PhaseResult {
                     phase: Phase::Test,
                     duration: Duration::from_secs(3),
-                    process_status: ProcessStatus::Success,
+                    process_status: Exit::Success,
                     argv: vec!["cargo".into(), "test".into()],
                 },
             ],
@@ -355,7 +355,7 @@ mod test {
             Some(&PhaseResult {
                 phase: Phase::Build,
                 duration: Duration::from_secs(2),
-                process_status: ProcessStatus::Success,
+                process_status: Exit::Success,
                 argv: vec!["cargo".into(), "build".into()],
             })
         );

src/process.rs

@@ -8,16 +8,14 @@
 #![allow(clippy::option_map_unit_fn)] // I don't think it's clearer with if/let.
 
 use std::ffi::OsStr;
-#[cfg(unix)]
-use std::os::unix::process::{CommandExt, ExitStatusExt};
 use std::process::{Child, Command, Stdio};
 use std::thread::sleep;
 use std::time::{Duration, Instant};
 
-use anyhow::{bail, Context};
+use anyhow::Context;
 use camino::Utf8Path;
 use serde::Serialize;
-use tracing::{debug, span, trace, warn, Level};
+use tracing::{debug, span, trace, Level};
 
 use crate::console::Console;
 use crate::interrupt::check_interrupted;
@@ -27,6 +25,16 @@ use crate::Result;
 /// How frequently to check if a subprocess finished.
 const WAIT_POLL_INTERVAL: Duration = Duration::from_millis(50);
 
+#[cfg(windows)]
+mod windows;
+#[cfg(windows)]
+use windows::{configure_command, terminate_child};
+
+#[cfg(unix)]
+mod unix;
+#[cfg(unix)]
+use unix::{configure_command, terminate_child};
+
 pub struct Process {
     child: Child,
     start: Instant,
@@ -44,7 +52,7 @@ impl Process {
         jobserver: &Option<jobserver::Client>,
         scenario_output: &mut ScenarioOutput,
         console: &Console,
-    ) -> Result<ProcessStatus> {
+    ) -> Result<Exit> {
         let mut child = Process::start(argv, env, cwd, timeout, jobserver, scenario_output)?;
         let process_status = loop {
             if let Some(exit_status) = child.poll()? {
@@ -68,22 +76,21 @@ impl Process {
         scenario_output: &mut ScenarioOutput,
     ) -> Result<Process> {
         let start = Instant::now();
-        let quoted_argv = cheap_shell_quote(argv);
+        let quoted_argv = quote_argv(argv);
         scenario_output.message(&quoted_argv)?;
         debug!(%quoted_argv, "start process");
         let os_env = env.iter().map(|(k, v)| (OsStr::new(k), OsStr::new(v)));
-        let mut child = Command::new(&argv[0]);
-        child
+        let mut command = Command::new(&argv[0]);
+        command
             .args(&argv[1..])
             .envs(os_env)
             .stdin(Stdio::null())
             .stdout(scenario_output.open_log_append()?)
             .stderr(scenario_output.open_log_append()?)
             .current_dir(cwd);
-        jobserver.as_ref().map(|js| js.configure(&mut child));
-        #[cfg(unix)]
-        child.process_group(0);
-        let child = child
+        jobserver.as_ref().map(|js| js.configure(&mut command));
+        configure_command(&mut command);
+        let child = command
             .spawn()
             .with_context(|| format!("failed to spawn {}", argv.join(" ")))?;
         Ok(Process {
@@ -95,28 +102,17 @@ impl Process {
 
     /// Check if the child process has finished; if so, return its status.
     #[mutants::skip] // It's hard to avoid timeouts if this never works...
-    pub fn poll(&mut self) -> Result<Option<ProcessStatus>> {
+    pub fn poll(&mut self) -> Result<Option<Exit>> {
         if self.timeout.is_some_and(|t| self.start.elapsed() > t) {
             debug!("timeout, terminating child process...",);
             self.terminate()?;
-            Ok(Some(ProcessStatus::Timeout))
+            Ok(Some(Exit::Timeout))
         } else if let Err(e) = check_interrupted() {
             debug!("interrupted, terminating child process...");
             self.terminate()?;
             Err(e)
         } else if let Some(status) = self.child.try_wait()? {
-            if let Some(code) = status.code() {
-                if code == 0 {
-                    return Ok(Some(ProcessStatus::Success));
-                } else {
-                    return Ok(Some(ProcessStatus::Failure(code as u32)));
-                }
-            }
-            #[cfg(unix)]
-            if let Some(signal) = status.signal() {
-                return Ok(Some(ProcessStatus::Signalled(signal as u8)));
-            }
-            Ok(Some(ProcessStatus::Other))
+            Ok(Some(status.into()))
         } else {
             Ok(None)
         }
@@ -126,12 +122,12 @@ impl Process {
     ///
     /// Blocks until the subprocess is terminated and then returns the exit status.
     ///
-    /// The status might not be Timeout if this raced with a normal exit.
+    /// The status might not be `Timeout` if this raced with a normal exit.
     #[mutants::skip] // would leak processes from tests if skipped
     fn terminate(&mut self) -> Result<()> {
         let _span = span!(Level::DEBUG, "terminate_child", pid = self.child.id()).entered();
         debug!("terminating child process");
-        terminate_child_impl(&mut self.child)?;
+        terminate_child(&mut self.child)?;
         trace!("wait for child after termination");
         match self.child.wait() {
             Err(err) => debug!(?err, "Failed to wait for child after termination"),
@@ -141,93 +137,77 @@ impl Process {
     }
 }
 
-#[cfg(unix)]
-#[allow(unknown_lints, clippy::needless_pass_by_ref_mut)] // To match Windows
-#[mutants::skip] // hard to exercise the ESRCH edge case
-fn terminate_child_impl(child: &mut Child) -> Result<()> {
-    use nix::errno::Errno;
-    use nix::sys::signal::{killpg, Signal};
-
-    let pid = nix::unistd::Pid::from_raw(child.id().try_into().unwrap());
-    match killpg(pid, Signal::SIGTERM) {
-        Ok(()) => Ok(()),
-        Err(Errno::ESRCH) => {
-            Ok(()) // Probably already gone
-        }
-        Err(Errno::EPERM) if cfg!(target_os = "macos") => {
-            Ok(()) // If the process no longer exists then macos can return EPERM (maybe?)
-        }
-        Err(errno) => {
-            let message = format!("failed to terminate child: {errno}");
-            warn!("{}", message);
-            bail!(message);
-        }
-    }
-}
-
-#[cfg(windows)]
-#[mutants::skip] // hard to exercise the ESRCH edge case
-fn terminate_child_impl(child: &mut Child) -> Result<()> {
-    child.kill().context("Kill child")
-}
-
 /// The result of running a single child process.
 #[derive(Copy, Clone, Debug, PartialEq, Eq, Serialize)]
-pub enum ProcessStatus {
+pub enum Exit {
     /// Exited with status 0.
     Success,
     /// Exited with status non-0.
     Failure(u32),
     /// Exceeded its timeout, and killed.
     Timeout,
     /// Killed by some signal.
+    #[cfg(unix)]
     Signalled(u8),
     /// Unknown or unexpected situation.
     Other,
 }
 
-impl ProcessStatus {
+impl Exit {
     pub fn is_success(&self) -> bool {
-        *self == ProcessStatus::Success
+        *self == Exit::Success
     }
 
     pub fn is_timeout(&self) -> bool {
-        *self == ProcessStatus::Timeout
+        *self == Exit::Timeout
     }
 
     pub fn is_failure(&self) -> bool {
-        matches!(self, ProcessStatus::Failure(_))
+        matches!(self, Exit::Failure(_))
     }
 }
 
 /// Quote an argv slice in Unix shell style.
 ///
-/// This is not completely guaranteed, but is only for debug logs.
-fn cheap_shell_quote<S: AsRef<str>, I: IntoIterator<Item = S>>(argv: I) -> String {
-    argv.into_iter()
-        .map(|s| {
-            s.as_ref()
-                .chars()
-                .flat_map(|c| match c {
-                    ' ' | '\t' | '\n' | '\r' | '\\' | '\'' | '"' => vec!['\\', c],
-                    _ => vec![c],
-                })
-                .collect::<String>()
-        })
-        .collect::<Vec<_>>()
-        .join(" ")
+/// This isn't guaranteed to match the interpretation of a shell or to be safe.
+/// It's just for debug logs.
+fn quote_argv<S: AsRef<str>, I: IntoIterator<Item = S>>(argv: I) -> String {
+    let mut r = String::new();
+    for s in argv {
+        if !r.is_empty() {
+            r.push(' ');
+        }
+        for c in s.as_ref().chars() {
+            match c {
+                '\t' => r.push_str(r"\t"),
+                '\n' => r.push_str(r"\n"),
+                '\r' => r.push_str(r"\r"),
+                ' ' | '\\' | '\'' | '"' => {
+                    r.push('\\');
+                    r.push(c);
+                }
+                _ => r.push(c),
+            }
+        }
+    }
+    r
 }
 
 #[cfg(test)]
 mod test {
-    use super::cheap_shell_quote;
+    use super::quote_argv;
 
     #[test]
     fn shell_quoting() {
-        assert_eq!(cheap_shell_quote(["foo".to_string()]), "foo");
+        assert_eq!(quote_argv(["foo".to_string()]), "foo");
+        assert_eq!(
+            quote_argv(["foo bar", r#"\blah\x"#, r#""quoted""#]),
+            r#"foo\ bar \\blah\\x \"quoted\""#
+        );
+        assert_eq!(quote_argv([""]), "");
         assert_eq!(
-            cheap_shell_quote(["foo bar", r#"\blah\t"#, r#""quoted""#]),
-            r#"foo\ bar \\blah\\t \"quoted\""#
+            quote_argv(["with whitespace", "\r\n\t\t"]),
+            r#"with\ whitespace \r\n\t\t"#
         );
     }
 }