diff --git a/nextest-runner/src/lib.rs b/nextest-runner/src/lib.rs index f3b51300181..aa158c9178c 100644 --- a/nextest-runner/src/lib.rs +++ b/nextest-runner/src/lib.rs @@ -23,6 +23,7 @@ pub mod runner; pub mod signal; mod stopwatch; pub mod target_runner; +mod test_command; pub mod test_filter; #[cfg(feature = "self-update")] pub mod update; diff --git a/nextest-runner/src/list/test_list.rs b/nextest-runner/src/list/test_list.rs index b45b6d23404..034a0246a69 100644 --- a/nextest-runner/src/list/test_list.rs +++ b/nextest-runner/src/list/test_list.rs @@ -9,6 +9,7 @@ use crate::{ list::{BinaryList, OutputFormat, RustBuildMeta, Styles, TestListState}, reuse_build::PathMapper, target_runner::{PlatformRunner, TargetRunner}, + test_command::{LocalExecuteContext, TestCommand}, test_filter::TestFilterBuilder, }; use camino::{Utf8Path, Utf8PathBuf}; @@ -24,7 +25,7 @@ use nextest_metadata::{ use once_cell::sync::{Lazy, OnceCell}; use owo_colors::OwoColorize; use std::{ - collections::{BTreeMap, BTreeSet, HashMap}, + collections::{BTreeMap, BTreeSet}, ffi::{OsStr, OsString}, io, io::Write, @@ -681,7 +682,7 @@ impl<'g> RustTestArtifact<'g> { argv.push("--ignored"); } - let cmd = make_test_command( + let mut cmd = TestCommand::new( ctx, program.clone(), &argv, @@ -689,39 +690,50 @@ impl<'g> RustTestArtifact<'g> { &self.package, &self.non_test_binaries, ); - let mut cmd = tokio::process::Command::from(cmd); - match cmd.output().await { - Ok(output) => { - if output.status.success() { - String::from_utf8(output.stdout).map_err(|err| { - CreateTestListError::CommandNonUtf8 { - binary_id: self.binary_id.clone(), - command: std::iter::once(program) - .chain(argv.iter().map(|&s| s.to_owned())) - .collect(), - stdout: err.into_bytes(), - stderr: output.stderr, - } - }) - } else { - Err(CreateTestListError::CommandFail { - binary_id: self.binary_id.clone(), - command: std::iter::once(program) - .chain(argv.iter().map(|&s| s.to_owned())) - .collect(), - exit_status: output.status, - stdout: output.stdout, - stderr: output.stderr, - }) - } + // Capture stdout and stderr. + cmd.command_mut() + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()); + + let child = cmd + .spawn() + .map_err(|error| CreateTestListError::CommandExecFail { + binary_id: self.binary_id.clone(), + command: std::iter::once(program.clone()) + .chain(argv.iter().map(|&s| s.to_owned())) + .collect(), + error, + })?; + + let output = child.wait_with_output().await.map_err(|error| { + CreateTestListError::CommandExecFail { + binary_id: self.binary_id.clone(), + command: std::iter::once(program.clone()) + .chain(argv.iter().map(|&s| s.to_owned())) + .collect(), + error, } - Err(error) => Err(CreateTestListError::CommandExecFail { + })?; + + if output.status.success() { + String::from_utf8(output.stdout).map_err(|err| CreateTestListError::CommandNonUtf8 { binary_id: self.binary_id.clone(), command: std::iter::once(program) .chain(argv.iter().map(|&s| s.to_owned())) .collect(), - error, - }), + stdout: err.into_bytes(), + stderr: output.stderr, + }) + } else { + Err(CreateTestListError::CommandFail { + binary_id: self.binary_id.clone(), + command: std::iter::once(program) + .chain(argv.iter().map(|&s| s.to_owned())) + .collect(), + exit_status: output.status, + stdout: output.stdout, + stderr: output.stderr, + }) } } } @@ -817,12 +829,12 @@ impl<'a> TestInstance<'a> { (&self.suite_info.binary_id, self.name) } - /// Creates the command expression for this test instance. - pub(crate) fn make_expression( + /// Creates the command for this test instance. + pub(crate) fn make_command( &self, ctx: &TestExecuteContext<'_>, test_list: &TestList<'_>, - ) -> std::process::Command { + ) -> TestCommand { let platform_runner = ctx .target_runner .for_build_platform(self.suite_info.build_platform); @@ -851,7 +863,7 @@ impl<'a> TestInstance<'a> { env: &test_list.env, }; - make_test_command( + TestCommand::new( &ctx, program, &args, @@ -872,153 +884,6 @@ pub struct TestExecuteContext<'a> { pub target_runner: &'a TargetRunner, } -#[derive(Clone, Debug)] -struct LocalExecuteContext<'a> { - double_spawn: &'a DoubleSpawnInfo, - runner: &'a TargetRunner, - dylib_path: &'a OsStr, - env: &'a EnvironmentMap, -} - -/// Create a duct Expression for a test binary with the given arguments, using the specified [`PackageMetadata`]. -fn make_test_command( - ctx: &LocalExecuteContext<'_>, - program: String, - args: &[&str], - cwd: &Utf8PathBuf, - package: &PackageMetadata<'_>, - non_test_binaries: &BTreeSet<(String, Utf8PathBuf)>, -) -> std::process::Command { - // This is a workaround for a macOS SIP issue: - // https://github.com/nextest-rs/nextest/pull/84 - // - // Basically, if SIP is enabled, macOS removes any environment variables that start with - // "LD_" or "DYLD_" when spawning system-protected processes. This unfortunately includes - // processes like bash -- this means that if nextest invokes a shell script, paths might - // end up getting sanitized. - // - // This is particularly relevant for target runners, which are often shell scripts. - // - // To work around this, re-export any variables that begin with LD_ or DYLD_ as "NEXTEST_LD_" - // or "NEXTEST_DYLD_". Do this on all platforms for uniformity. - // - // Nextest never changes these environment variables within its own process, so caching them is - // valid. - fn is_sip_sanitized(var: &str) -> bool { - // Look for variables starting with LD_ or DYLD_. - // https://briandfoy.github.io/macos-s-system-integrity-protection-sanitizes-your-environment/ - var.starts_with("LD_") || var.starts_with("DYLD_") - } - - static LD_DYLD_ENV_VARS: Lazy> = Lazy::new(|| { - std::env::vars_os() - .filter_map(|(k, v)| match k.into_string() { - Ok(k) => is_sip_sanitized(&k).then_some((k, v)), - Err(_) => None, - }) - .collect() - }); - - // TODO: pass in double spawn option - let mut cmd = if let Some(current_exe) = ctx.double_spawn.current_exe() { - let mut cmd = std::process::Command::new(current_exe); - cmd.args([DoubleSpawnInfo::SUBCOMMAND_NAME, "--", program.as_str()]); - cmd.arg(&shell_words::join(args)); - cmd - } else { - let mut cmd = std::process::Command::new(program); - cmd.args(args); - cmd - }; - - // NB: we will always override user-provided environment variables with the - // `CARGO_*` and `NEXTEST_*` variables set directly on `cmd` below. - ctx.env.apply_env(&mut cmd); - - cmd.current_dir(cwd) - // This environment variable is set to indicate that tests are being run under nextest. - .env("NEXTEST", "1") - // This environment variable is set to indicate that each test is being run in its own process. - .env("NEXTEST_EXECUTION_MODE", "process-per-test") - // These environment variables are set at runtime by cargo test: - // https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-crates - .env( - "CARGO_MANIFEST_DIR", - // CARGO_MANIFEST_DIR is set to the *new* cwd after path mapping. - cwd, - ) - .env( - "__NEXTEST_ORIGINAL_CARGO_MANIFEST_DIR", - // This is a test-only environment variable set to the *old* cwd. Not part of the - // public API. - package.manifest_path().parent().unwrap(), - ) - .env("CARGO_PKG_VERSION", format!("{}", package.version())) - .env( - "CARGO_PKG_VERSION_MAJOR", - format!("{}", package.version().major), - ) - .env( - "CARGO_PKG_VERSION_MINOR", - format!("{}", package.version().minor), - ) - .env( - "CARGO_PKG_VERSION_PATCH", - format!("{}", package.version().patch), - ) - .env( - "CARGO_PKG_VERSION_PRE", - format!("{}", package.version().pre), - ) - .env("CARGO_PKG_AUTHORS", package.authors().join(":")) - .env("CARGO_PKG_NAME", package.name()) - .env( - "CARGO_PKG_DESCRIPTION", - package.description().unwrap_or_default(), - ) - .env("CARGO_PKG_HOMEPAGE", package.homepage().unwrap_or_default()) - .env("CARGO_PKG_LICENSE", package.license().unwrap_or_default()) - .env( - "CARGO_PKG_LICENSE_FILE", - package.license_file().unwrap_or_else(|| "".as_ref()), - ) - .env( - "CARGO_PKG_REPOSITORY", - package.repository().unwrap_or_default(), - ) - .env( - "CARGO_PKG_RUST_VERSION", - package.rust_version().map_or(String::new(), |v| { - // A Rust version e.g. "1.58" has v.to_string() generated as "^1.58". - // Remove the prefix ^. - let s = v.to_string(); - match s.strip_prefix('^') { - Some(suffix) => suffix.to_owned(), - None => s, - } - }), - ) - .env(dylib_path_envvar(), ctx.dylib_path); - - for (k, v) in &*LD_DYLD_ENV_VARS { - if k != dylib_path_envvar() { - cmd.env("NEXTEST_".to_owned() + k, v); - } - } - // Also add the dylib path envvar under the NEXTEST_ prefix. - if is_sip_sanitized(dylib_path_envvar()) { - cmd.env("NEXTEST_".to_owned() + dylib_path_envvar(), ctx.dylib_path); - } - - // Expose paths to non-test binaries at runtime so that relocated paths work. - // These paths aren't exposed by Cargo at runtime, so use a NEXTEST_BIN_EXE prefix. - for (name, path) in non_test_binaries { - cmd.env(format!("NEXTEST_BIN_EXE_{}", name), path); - } - - cmd -} - #[cfg(test)] mod tests { use super::*; diff --git a/nextest-runner/src/runner.rs b/nextest-runner/src/runner.rs index 80ba72f066c..0aa6ff3d15b 100644 --- a/nextest-runner/src/runner.rs +++ b/nextest-runner/src/runner.rs @@ -589,13 +589,14 @@ impl<'a> TestRunnerInner<'a> { double_spawn: &self.double_spawn, target_runner: &self.target_runner, }; - let mut cmd = test.make_expression(&ctx, self.test_list); + let mut cmd = test.make_command(&ctx, self.test_list); + let command_mut = cmd.command_mut(); // Debug environment variable for testing. - cmd.env("__NEXTEST_ATTEMPT", format!("{}", retry_data.attempt)); - cmd.env("NEXTEST_RUN_ID", format!("{}", self.run_id)); - cmd.stdin(Stdio::null()); - imp::cmd_pre_exec(&mut cmd); + command_mut.env("__NEXTEST_ATTEMPT", format!("{}", retry_data.attempt)); + command_mut.env("NEXTEST_RUN_ID", format!("{}", self.run_id)); + command_mut.stdin(Stdio::null()); + imp::cmd_pre_exec(command_mut); // If creating a job fails, we might be on an old system. Ignore this -- job objects are a // best-effort thing. @@ -603,11 +604,11 @@ impl<'a> TestRunnerInner<'a> { if !self.no_capture { // Capture stdout and stderr. - cmd.stdout(std::process::Stdio::piped()) + command_mut + .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()); }; - let mut cmd = tokio::process::Command::from(cmd); let mut child = cmd.spawn()?; // If assigning the child to the job fails, ignore this. This can happen if the process has diff --git a/nextest-runner/src/test_command.rs b/nextest-runner/src/test_command.rs new file mode 100644 index 00000000000..caa59e33fe8 --- /dev/null +++ b/nextest-runner/src/test_command.rs @@ -0,0 +1,179 @@ +// Copyright (c) The nextest Contributors +// SPDX-License-Identifier: MIT OR Apache-2.0 + +use crate::{ + cargo_config::EnvironmentMap, double_spawn::DoubleSpawnInfo, helpers::dylib_path_envvar, + target_runner::TargetRunner, +}; +use camino::Utf8PathBuf; +use guppy::graph::PackageMetadata; +use once_cell::sync::Lazy; +use std::{ + collections::{BTreeSet, HashMap}, + ffi::{OsStr, OsString}, +}; + +#[derive(Clone, Debug)] +pub(crate) struct LocalExecuteContext<'a> { + pub(crate) double_spawn: &'a DoubleSpawnInfo, + pub(crate) runner: &'a TargetRunner, + pub(crate) dylib_path: &'a OsStr, + pub(crate) env: &'a EnvironmentMap, +} + +/// Represents a to-be-run test command for a test binary with a certain set of arguments. +pub(crate) struct TestCommand { + /// The command to be run. + command: std::process::Command, +} + +impl TestCommand { + /// Creates a new test command. + pub(crate) fn new( + ctx: &LocalExecuteContext<'_>, + program: String, + args: &[&str], + cwd: &Utf8PathBuf, + package: &PackageMetadata<'_>, + non_test_binaries: &BTreeSet<(String, Utf8PathBuf)>, + ) -> Self { + // This is a workaround for a macOS SIP issue: + // https://github.com/nextest-rs/nextest/pull/84 + // + // Basically, if SIP is enabled, macOS removes any environment variables that start with + // "LD_" or "DYLD_" when spawning system-protected processes. This unfortunately includes + // processes like bash -- this means that if nextest invokes a shell script, paths might + // end up getting sanitized. + // + // This is particularly relevant for target runners, which are often shell scripts. + // + // To work around this, re-export any variables that begin with LD_ or DYLD_ as "NEXTEST_LD_" + // or "NEXTEST_DYLD_". Do this on all platforms for uniformity. + // + // Nextest never changes these environment variables within its own process, so caching them is + // valid. + fn is_sip_sanitized(var: &str) -> bool { + // Look for variables starting with LD_ or DYLD_. + // https://briandfoy.github.io/macos-s-system-integrity-protection-sanitizes-your-environment/ + var.starts_with("LD_") || var.starts_with("DYLD_") + } + + static LD_DYLD_ENV_VARS: Lazy> = Lazy::new(|| { + std::env::vars_os() + .filter_map(|(k, v)| match k.into_string() { + Ok(k) => is_sip_sanitized(&k).then_some((k, v)), + Err(_) => None, + }) + .collect() + }); + + // TODO: pass in double spawn option + let mut cmd = if let Some(current_exe) = ctx.double_spawn.current_exe() { + let mut cmd = std::process::Command::new(current_exe); + cmd.args([DoubleSpawnInfo::SUBCOMMAND_NAME, "--", program.as_str()]); + cmd.arg(&shell_words::join(args)); + cmd + } else { + let mut cmd = std::process::Command::new(program); + cmd.args(args); + cmd + }; + + // NB: we will always override user-provided environment variables with the + // `CARGO_*` and `NEXTEST_*` variables set directly on `cmd` below. + ctx.env.apply_env(&mut cmd); + + cmd.current_dir(cwd) + // This environment variable is set to indicate that tests are being run under nextest. + .env("NEXTEST", "1") + // This environment variable is set to indicate that each test is being run in its own process. + .env("NEXTEST_EXECUTION_MODE", "process-per-test") + // These environment variables are set at runtime by cargo test: + // https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-crates + .env( + "CARGO_MANIFEST_DIR", + // CARGO_MANIFEST_DIR is set to the *new* cwd after path mapping. + cwd, + ) + .env( + "__NEXTEST_ORIGINAL_CARGO_MANIFEST_DIR", + // This is a test-only environment variable set to the *old* cwd. Not part of the + // public API. + package.manifest_path().parent().unwrap(), + ) + .env("CARGO_PKG_VERSION", format!("{}", package.version())) + .env( + "CARGO_PKG_VERSION_MAJOR", + format!("{}", package.version().major), + ) + .env( + "CARGO_PKG_VERSION_MINOR", + format!("{}", package.version().minor), + ) + .env( + "CARGO_PKG_VERSION_PATCH", + format!("{}", package.version().patch), + ) + .env( + "CARGO_PKG_VERSION_PRE", + format!("{}", package.version().pre), + ) + .env("CARGO_PKG_AUTHORS", package.authors().join(":")) + .env("CARGO_PKG_NAME", package.name()) + .env( + "CARGO_PKG_DESCRIPTION", + package.description().unwrap_or_default(), + ) + .env("CARGO_PKG_HOMEPAGE", package.homepage().unwrap_or_default()) + .env("CARGO_PKG_LICENSE", package.license().unwrap_or_default()) + .env( + "CARGO_PKG_LICENSE_FILE", + package.license_file().unwrap_or_else(|| "".as_ref()), + ) + .env( + "CARGO_PKG_REPOSITORY", + package.repository().unwrap_or_default(), + ) + .env( + "CARGO_PKG_RUST_VERSION", + package.rust_version().map_or(String::new(), |v| { + // A Rust version e.g. "1.58" has v.to_string() generated as "^1.58". + // Remove the prefix ^. + let s = v.to_string(); + match s.strip_prefix('^') { + Some(suffix) => suffix.to_owned(), + None => s, + } + }), + ) + .env(dylib_path_envvar(), ctx.dylib_path); + + for (k, v) in &*LD_DYLD_ENV_VARS { + if k != dylib_path_envvar() { + cmd.env("NEXTEST_".to_owned() + k, v); + } + } + // Also add the dylib path envvar under the NEXTEST_ prefix. + if is_sip_sanitized(dylib_path_envvar()) { + cmd.env("NEXTEST_".to_owned() + dylib_path_envvar(), ctx.dylib_path); + } + + // Expose paths to non-test binaries at runtime so that relocated paths work. + // These paths aren't exposed by Cargo at runtime, so use a NEXTEST_BIN_EXE prefix. + for (name, path) in non_test_binaries { + cmd.env(format!("NEXTEST_BIN_EXE_{}", name), path); + } + + Self { command: cmd } + } + + #[inline] + pub(crate) fn command_mut(&mut self) -> &mut std::process::Command { + &mut self.command + } + + pub(crate) fn spawn(self) -> std::io::Result { + let mut command = tokio::process::Command::from(self.command); + command.spawn() + } +}