From 73b73fa82c342e3cccebc1fc3d9839b1e7201317 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Fri, 18 Feb 2022 15:32:38 +0100 Subject: [PATCH 1/9] Add support for target runners --- Cargo.lock | 12 ++ cargo-nextest/src/cargo_cli.rs | 2 +- cargo-nextest/src/dispatch.rs | 35 ++++- nextest-runner/Cargo.toml | 7 + nextest-runner/src/errors.rs | 68 +++++++++ nextest-runner/src/lib.rs | 1 + nextest-runner/src/runner.rs | 8 +- nextest-runner/src/target_runner.rs | 218 ++++++++++++++++++++++++++++ nextest-runner/src/test_list.rs | 59 ++++++-- nextest-runner/tests/basic.rs | 17 ++- 10 files changed, 395 insertions(+), 32 deletions(-) create mode 100644 nextest-runner/src/target_runner.rs diff --git a/Cargo.lock b/Cargo.lock index 71e127f889e..a0eb90bbeda 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -469,6 +469,15 @@ dependencies = [ "libc", ] +[[package]] +name = "home" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2456aef2e6b6a9784192ae780c0f15bc57df0e918585282325e8c8ac27737654" +dependencies = [ + "winapi", +] + [[package]] name = "humantime" version = "2.1.0" @@ -633,6 +642,7 @@ dependencies = [ "aho-corasick", "camino", "cargo_metadata", + "cfg-expr", "chrono", "color-eyre", "config", @@ -641,6 +651,7 @@ dependencies = [ "debug-ignore", "duct", "guppy", + "home", "humantime-serde", "indent_write", "indoc", @@ -657,6 +668,7 @@ dependencies = [ "serde", "serde_json", "strip-ansi-escapes", + "toml", "twox-hash", ] diff --git a/cargo-nextest/src/cargo_cli.rs b/cargo-nextest/src/cargo_cli.rs index 8619c13e7cb..74d21650373 100644 --- a/cargo-nextest/src/cargo_cli.rs +++ b/cargo-nextest/src/cargo_cli.rs @@ -89,7 +89,7 @@ pub(crate) struct CargoOptions { /// Build for the target triple #[clap(long, value_name = "TRIPLE")] - target: Option, + pub(crate) target: Option, /// Directory for all generated artifacts #[clap(long, value_name = "DIR")] diff --git a/cargo-nextest/src/dispatch.rs b/cargo-nextest/src/dispatch.rs index 9795b3db29c..d98a1479843 100644 --- a/cargo-nextest/src/dispatch.rs +++ b/cargo-nextest/src/dispatch.rs @@ -17,6 +17,7 @@ use nextest_runner::{ reporter::{StatusLevel, TestOutputDisplay, TestReporterBuilder}, runner::TestRunnerBuilder, signal::SignalHandler, + target_runner::TargetRunner, test_filter::{RunIgnored, TestFilterBuilder}, test_list::{OutputFormat, RustTestArtifact, SerializableFormat, TestList}, }; @@ -183,6 +184,7 @@ impl TestBuildFilter { manifest_path: Option<&'g Utf8Path>, graph: &'g PackageGraph, output: OutputContext, + runner: Option<&TargetRunner>, ) -> Result> { // Don't use the manifest path from the graph to ensure that if the user cd's into a // particular crate and runs cargo nextest, then it behaves identically to cargo test. @@ -209,7 +211,7 @@ impl TestBuildFilter { let test_filter = TestFilterBuilder::new(self.run_ignored, self.partition.clone(), &self.filter); - TestList::new(test_artifacts, &test_filter).wrap_err("error building test list") + TestList::new(test_artifacts, &test_filter, runner).wrap_err("error building test list") } } @@ -316,8 +318,15 @@ impl AppImpl { build_filter, message_format, } => { - let mut test_list = - build_filter.compute(self.manifest_path.as_deref(), &graph, output)?; + let target_runner = + TargetRunner::for_target(build_filter.cargo_options.target.as_deref())?; + + let mut test_list = build_filter.compute( + self.manifest_path.as_deref(), + &graph, + output, + target_runner.as_ref(), + )?; if output.color.should_colorize(Stream::Stdout) { test_list.colorize(); } @@ -343,8 +352,15 @@ impl AppImpl { std::fs::create_dir_all(&store_dir) .wrap_err_with(|| format!("failed to create store dir '{}'", store_dir))?; - let test_list = - build_filter.compute(self.manifest_path.as_deref(), &graph, output)?; + let target_runner = + TargetRunner::for_target(build_filter.cargo_options.target.as_deref())?; + + let test_list = build_filter.compute( + self.manifest_path.as_deref(), + &graph, + output, + target_runner.as_ref(), + )?; let mut reporter = reporter_opts .to_builder(no_capture) @@ -355,9 +371,12 @@ impl AppImpl { } let handler = SignalHandler::new().wrap_err("failed to set up Ctrl-C handler")?; - let runner = runner_opts - .to_builder(no_capture) - .build(&test_list, &profile, handler); + let runner = runner_opts.to_builder(no_capture).build( + &test_list, + &profile, + handler, + target_runner.as_ref(), + ); let stderr = std::io::stderr(); let mut writer = BufWriter::new(stderr); let run_stats = runner.try_execute(|event| { diff --git a/nextest-runner/Cargo.toml b/nextest-runner/Cargo.toml index 925cde8e05d..cc77eae0fb1 100644 --- a/nextest-runner/Cargo.toml +++ b/nextest-runner/Cargo.toml @@ -14,12 +14,17 @@ aho-corasick = "0.7.18" camino = { version = "1.0.7", features = ["serde1"] } config = { version = "0.11.0", default-features = false, features = ["toml"] } cargo_metadata = "0.14.1" +# For cfg expression evaluation for [target.'cfg()'] expressions +cfg-expr = { version = "0.10.1", features = ["targets"] } chrono = "0.4.19" crossbeam-channel = "0.5.2" ctrlc = { version = "3.2.1", features = ["termination"] } debug-ignore = "1.0.1" duct = "0.13.5" guppy = "0.13.0" +# Used to find the cargo root directory, which is needed in case the user has +# added a config.toml there +home = "0.5.3" humantime-serde = "1.0.1" indent_write = "2.2.0" once_cell = "1.9.0" @@ -29,6 +34,8 @@ rayon = "1.5.1" serde = { version = "1.0.136", features = ["derive"] } serde_json = "1.0.79" strip-ansi-escapes = "0.1.1" +# For parsing of .cargo/config.toml files +toml = "0.5.8" twox-hash = { version = "1.6.2", default-features = false } nextest-metadata = { version = "0.1.0", path = "../nextest-metadata" } diff --git a/nextest-runner/src/errors.rs b/nextest-runner/src/errors.rs index 94c6f8d5d39..67ec8fc76e3 100644 --- a/nextest-runner/src/errors.rs +++ b/nextest-runner/src/errors.rs @@ -405,3 +405,71 @@ impl error::Error for JunitError { Some(&self.err) } } + +/// An error occurred determining the target runner +#[derive(Debug)] +pub enum TargetRunnerError { + /// Failed to determine the host triple, which is needed to determine the + /// default target triple when a target is not explicitly specified + UnableToDetermineHostTriple, + /// An environment variable contained non-utf8 content + InvalidEnvironmentVar(String), + /// An environment variable or key was found that matches the target triple, + /// but it didn't actually contain a binary + BinaryNotSpecified(String, String), + /// Failed to retrieve a directory + UnableToReadDir(std::io::Error), + /// Failed to canonicalize a path + FailedPathCanonicalization(Utf8PathBuf, std::io::Error), + /// A path was non-utf8 + NonUtf8Path(std::path::PathBuf), + /// Failed to read config file + FailedToReadConfig(Utf8PathBuf, std::io::Error), + /// Failed to deserialize config file + FailedToParseConfig(Utf8PathBuf, toml::de::Error), +} + +impl fmt::Display for TargetRunnerError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::UnableToDetermineHostTriple => f.write_str("unable to determine host triple"), + Self::InvalidEnvironmentVar(key) => { + write!(f, "environment variable '{}' contained non-utf8 data", key) + } + Self::BinaryNotSpecified(key, value) => { + write!( + f, + "runner '{}' = '{}' did not contain a runner binary", + key, value + ) + } + Self::UnableToReadDir(io) => { + write!(f, "unable to read directory: {}", io) + } + Self::FailedPathCanonicalization(path, err) => { + write!(f, "failed to canonicalize path '{}': {}", path, err) + } + Self::NonUtf8Path(path) => { + write!(f, "path '{}' is non-utf8", path.display()) + } + Self::FailedToReadConfig(path, err) => { + write!(f, "failed to read '{}': {}", path, err) + } + Self::FailedToParseConfig(path, err) => { + write!(f, "failed to parse '{}': {}", path, err) + } + } + } +} + +impl error::Error for TargetRunnerError { + fn source(&self) -> Option<&(dyn error::Error + 'static)> { + match self { + Self::UnableToReadDir(io) => Some(io), + Self::FailedPathCanonicalization(_, io) => Some(io), + Self::FailedToReadConfig(_, io) => Some(io), + Self::FailedToParseConfig(_, toml) => Some(toml), + _ => None, + } + } +} diff --git a/nextest-runner/src/lib.rs b/nextest-runner/src/lib.rs index 3d31aac9839..e3531962f73 100644 --- a/nextest-runner/src/lib.rs +++ b/nextest-runner/src/lib.rs @@ -48,5 +48,6 @@ pub mod reporter; pub mod runner; pub mod signal; mod stopwatch; +pub mod target_runner; pub mod test_filter; pub mod test_list; diff --git a/nextest-runner/src/runner.rs b/nextest-runner/src/runner.rs index 7806c37bd90..be39b591970 100644 --- a/nextest-runner/src/runner.rs +++ b/nextest-runner/src/runner.rs @@ -10,6 +10,7 @@ use crate::{ reporter::{CancelReason, StatusLevel, TestEvent}, signal::{SignalEvent, SignalHandler}, stopwatch::{StopwatchEnd, StopwatchStart}, + target_runner::TargetRunner, test_list::{TestInstance, TestList}, }; use crossbeam_channel::{RecvTimeoutError, Sender}; @@ -67,6 +68,7 @@ impl TestRunnerBuilder { test_list: &'a TestList, profile: &NextestProfile<'_>, handler: SignalHandler, + target_runner: Option<&'a TargetRunner>, ) -> TestRunner<'a> { let test_threads = match self.no_capture { true => 1, @@ -82,6 +84,7 @@ impl TestRunnerBuilder { fail_fast, slow_timeout, test_list, + target_runner, run_pool: ThreadPoolBuilder::new() // The main run_pool closure will need its own thread. .num_threads(test_threads + 1) @@ -107,6 +110,7 @@ pub struct TestRunner<'a> { fail_fast: bool, slow_timeout: Duration, test_list: &'a TestList<'a>, + target_runner: Option<&'a TargetRunner>, run_pool: ThreadPool, wait_pool: ThreadPool, handler: SignalHandler, @@ -338,7 +342,7 @@ impl<'a> TestRunner<'a> { run_sender: &Sender>, ) -> std::io::Result { let cmd = test - .make_expression() + .make_expression(self.target_runner) .unchecked() // Debug environment variable for testing. .env("__NEXTEST_ATTEMPT", format!("{}", attempt)); @@ -819,7 +823,7 @@ mod tests { let config = NextestConfig::default_config("/fake/dir"); let profile = config.profile(NextestConfig::DEFAULT_PROFILE).unwrap(); let handler = SignalHandler::noop(); - let runner = builder.build(&test_list, &profile, handler); + let runner = builder.build(&test_list, &profile, handler, None); assert!(runner.no_capture, "no_capture is true"); assert_eq!( runner.run_pool.current_num_threads(), diff --git a/nextest-runner/src/target_runner.rs b/nextest-runner/src/target_runner.rs new file mode 100644 index 00000000000..8dd15d4296e --- /dev/null +++ b/nextest-runner/src/target_runner.rs @@ -0,0 +1,218 @@ +//! Adds support for [target runners](https://doc.rust-lang.org/cargo/reference/config.html#targettriplerunner) + +use crate::errors::TargetRunnerError; +use camino::Utf8PathBuf; + +/// A [target runner](https://doc.rust-lang.org/cargo/reference/config.html#targettriplerunner) +/// used to execute a test binary rather than the default of executing natively +pub struct TargetRunner { + /// This is required + runner_binary: Utf8PathBuf, + /// These are optional + args: Vec, +} + +impl TargetRunner { + /// Acquires the [target runner](https://doc.rust-lang.org/cargo/reference/config.html#targettriplerunner) + /// which can be set in a [.cargo/config.toml](https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure) + /// or via a `CARGO_TARGET_{TRIPLE}_RUNNER` environment variable + pub fn for_target(target_triple: Option<&str>) -> Result, TargetRunnerError> { + use std::borrow::Cow; + + let target_triple: Cow<'_, str> = match target_triple { + Some(target) => Cow::Borrowed(target), + None => Cow::Owned(cfg_expr::target_lexicon::HOST.to_string()), + }; + + // First check if have a CARGO_TARGET_{TRIPLE}_RUNNER environment variable + // set, and if so use that, as it takes precedence over the static config.toml + { + let env_key = format!( + "CARGO_TARGET_{}_RUNNER", + target_triple.to_ascii_uppercase().replace('-', "_") + ); + + if let Some(runner_var) = std::env::var_os(&env_key) { + let runner = runner_var + .into_string() + .map_err(|_osstr| TargetRunnerError::InvalidEnvironmentVar(env_key.clone()))?; + return Self::parse_runner(&env_key, runner).map(Some); + } + } + + Self::find_config(&target_triple) + } + + fn find_config(target_triple: &str) -> Result, TargetRunnerError> { + // This is a bit non-intuitive, but the .cargo/config.toml hierarchy is actually + // based on the current working directory, _not_ the manifest path, this bug + // has existed for a while https://github.com/rust-lang/cargo/issues/2930 + let root = std::env::current_dir() + .map_err(TargetRunnerError::UnableToReadDir) + .and_then(|cwd| { + Utf8PathBuf::from_path_buf(cwd).map_err(TargetRunnerError::NonUtf8Path) + })?; + + // Attempt lookup the $CARGO_HOME directory from the cwd, as that can + // contain a default config.toml + let mut cargo_home_path = home::cargo_home_with_cwd(root.as_std_path()) + .map_err(TargetRunnerError::UnableToReadDir) + .and_then(|home| { + Utf8PathBuf::from_path_buf(home).map_err(TargetRunnerError::NonUtf8Path) + })?; + + // Note versions of cargo older than 1.39 only support .cargo/config without + // the toml extension, but since this repo states its MSRV as 1.54 it should + // be ok to lose that particular piece of backwards compatibility + let mut configs = Vec::new(); + + fn read_config_dir(dir: &mut Utf8PathBuf) -> Option { + dir.push("config.toml"); + + if !dir.exists() { + dir.set_extension("toml"); + } + + if dir.exists() { + let ret = dir.clone(); + dir.pop(); + Some(ret) + } else { + dir.pop(); + None + } + } + + let mut dir = root + .canonicalize() + .map_err(|io| TargetRunnerError::FailedPathCanonicalization(root, io)) + .and_then(|canon| { + Utf8PathBuf::from_path_buf(canon).map_err(TargetRunnerError::NonUtf8Path) + })?; + + for _ in 0..dir.ancestors().count() { + dir.push(".cargo"); + + if !dir.exists() { + dir.pop(); + dir.pop(); + continue; + } + + if let Some(config) = read_config_dir(&mut dir) { + configs.push(config); + } + + dir.pop(); + dir.pop(); + } + + if let Some(home_config) = read_config_dir(&mut cargo_home_path) { + // Ensure we don't add a duplicate if the current directory is underneath + // the same root as $CARGO_HOME + if !configs.iter().any(|path| path == &home_config) { + configs.push(home_config); + } + } + + #[derive(serde::Deserialize, Debug)] + struct CargoConfigRunner { + runner: Option, + } + + #[derive(serde::Deserialize, Debug)] + struct CargoConfig { + target: Option>, + } + + let mut target_runner = None; + + let triple_runner_key = format!("target.{}.runner", target_triple); + // Attempt to get the target info from a triple, this can fail if the + // target is actually a .json target spec, or is otherwise of a form + // that target_lexicon is unable to parse, which can happen with newer + // niche targets + let target_info: Option = target_triple.parse().ok(); + + // Now that we've found all of the config files that could declare + // a runner that matches our target triple, we need to actually find + // all the matches, but in reverse order as the closer the config is + // to our current working directory, the higher precedence it has + 'config: for config_path in configs.into_iter().rev() { + let config_contents = std::fs::read_to_string(&config_path) + .map_err(|io| TargetRunnerError::FailedToReadConfig(config_path.clone(), io))?; + let config: CargoConfig = toml::from_str(&config_contents) + .map_err(|io| TargetRunnerError::FailedToParseConfig(config_path.clone(), io))?; + + if let Some(mut targets) = config.target { + // First lookup by the exact triple, as that one always takes precedence + if let Some(target) = targets.remove(target_triple) { + if let Some(runner) = target.runner { + target_runner = Some(Self::parse_runner(&triple_runner_key, runner)?); + continue; + } + } + + if let Some(target_info) = &target_info { + // Next check if there are target.'cfg(..)' expressions that match + // the target. cargo states that it is not allowed for more than + // 1 cfg runner to match the target, but we let cargo handle that + // error itself, we just use the first one that matches + for (cfg, runner) in targets.into_iter().filter_map(|(k, v)| match v.runner { + Some(runner) if k.starts_with("cfg(") => Some((k, runner)), + _ => None, + }) { + // Treat these as non-fatal, but would be good to log maybe + let expr = match cfg_expr::Expression::parse(&cfg) { + Ok(expr) => expr, + Err(_err) => continue, + }; + + if expr.eval(|pred| match pred { + cfg_expr::Predicate::Target(tp) => tp.matches(target_info), + _ => false, + }) { + target_runner = Some(Self::parse_runner(&triple_runner_key, runner)?); + continue 'config; + } + } + } + } + } + + Ok(target_runner) + } + + fn parse_runner(key: &str, value: String) -> Result { + // We only split on whitespace, which doesn't take quoting into account, + // but I believe that cargo doesn't do that either + let mut runner_iter = value.split_whitespace(); + + let runner_binary = runner_iter + .next() + .ok_or_else(|| TargetRunnerError::BinaryNotSpecified(key.to_owned(), value.clone()))?; + let args = runner_iter.map(String::from).collect(); + + Ok(Self { + runner_binary: runner_binary.into(), + args, + }) + } + + /// Gets the runner binary path. + /// + /// Note that this is returned as a `str` specifically to avoid duct's + /// behavior of prepending `.` to paths it thinks are relative, the path + /// specified for a runner can be a full path, but it is most commonly a + /// binary found in `PATH` + #[inline] + pub fn binary(&self) -> &str { + &self.runner_binary.as_str() + } + + /// Gets the (optional) runner binary arguments + #[inline] + pub fn args(&self) -> impl Iterator { + self.args.iter().map(AsRef::as_ref) + } +} diff --git a/nextest-runner/src/test_list.rs b/nextest-runner/src/test_list.rs index e8da92f3860..3fd236c2944 100644 --- a/nextest-runner/src/test_list.rs +++ b/nextest-runner/src/test_list.rs @@ -11,6 +11,7 @@ pub use output_format::*; use crate::{ errors::{FromMessagesError, ParseTestListError, WriteTestListError}, helpers::write_test_name, + target_runner::TargetRunner, test_filter::TestFilterBuilder, }; use camino::{Utf8Path, Utf8PathBuf}; @@ -23,7 +24,7 @@ use guppy::{ use nextest_metadata::{RustTestCaseSummary, RustTestSuiteSummary, TestListSummary}; use once_cell::sync::OnceCell; use owo_colors::{OwoColorize, Style}; -use std::{collections::BTreeMap, io, io::Write, path::Path}; +use std::{collections::BTreeMap, io, io::Write}; /// A Rust test binary built by Cargo. This artifact hasn't been run yet so there's no information /// about the tests within it. @@ -143,13 +144,14 @@ impl<'g> TestList<'g> { pub fn new( test_artifacts: impl IntoIterator>, filter: &TestFilterBuilder, + runner: Option<&TargetRunner>, ) -> Result { let mut test_count = 0; let test_artifacts = test_artifacts .into_iter() .map(|test_binary| { - let (non_ignored, ignored) = test_binary.exec()?; + let (non_ignored, ignored) = test_binary.exec(runner)?; let (bin, info) = Self::process_output( test_binary, filter, @@ -430,25 +432,39 @@ impl<'g> TestList<'g> { impl<'g> RustTestArtifact<'g> { /// Run this binary with and without --ignored and get the corresponding outputs. - fn exec(&self) -> Result<(String, String), ParseTestListError> { - let non_ignored = self.exec_single(false)?; - let ignored = self.exec_single(true)?; + fn exec(&self, runner: Option<&TargetRunner>) -> Result<(String, String), ParseTestListError> { + let non_ignored = self.exec_single(false, runner)?; + let ignored = self.exec_single(true, runner)?; Ok((non_ignored, ignored)) } - fn exec_single(&self, ignored: bool) -> Result { - let mut argv = vec!["--list", "--format", "terse"]; + fn exec_single( + &self, + ignored: bool, + runner: Option<&TargetRunner>, + ) -> Result { + let mut argv = Vec::new(); + + let program: std::ffi::OsString = if let Some(runner) = runner { + argv.push(self.binary_path.as_str()); + argv.extend(runner.args()); + runner.binary().into() + } else { + use duct::IntoExecutablePath; + self.binary_path.as_std_path().to_executable() + }; + + argv.extend(["--list", "--format", "terse"]); if ignored { argv.push("--ignored"); } - let cmd = cmd(AsRef::::as_ref(&self.binary_path), argv) - .dir(&self.cwd) - .stdout_capture(); + + let cmd = cmd(program, argv).dir(&self.cwd).stdout_capture(); cmd.read().map_err(|error| { ParseTestListError::command( format!( - "'{} --list --format --terse{}'", + "'{} --list --format terse{}'", self.binary_path, if ignored { " --ignored" } else { "" } ), @@ -491,16 +507,31 @@ impl<'a> TestInstance<'a> { } /// Creates the command expression for this test instance. - pub(crate) fn make_expression(&self) -> Expression { + pub(crate) fn make_expression(&self, target_runner: Option<&TargetRunner>) -> Expression { // TODO: non-rust tests - let mut args = vec!["--exact", self.name, "--nocapture"]; + + let mut args = Vec::new(); + + let program: std::ffi::OsString = match target_runner { + Some(tr) => { + args.push(self.binary.as_str()); + args.extend(tr.args()); + tr.binary().into() + } + None => { + use duct::IntoExecutablePath; + self.binary.as_std_path().to_executable() + } + }; + + args.extend(["--exact", self.name, "--nocapture"]); if self.test_info.ignored { args.push("--ignored"); } let package = self.bin_info.package; - let cmd = cmd(AsRef::::as_ref(self.binary), args) + let cmd = cmd(program, args) .dir(&self.bin_info.cwd) // This environment variable is set to indicate that tests are being run under nextest. .env("NEXTEST", "1") diff --git a/nextest-runner/tests/basic.rs b/nextest-runner/tests/basic.rs index 9bee8fc8c6f..f68df5fca20 100644 --- a/nextest-runner/tests/basic.rs +++ b/nextest-runner/tests/basic.rs @@ -150,7 +150,7 @@ fn init_fixture_targets() -> BTreeMap> { fn test_list_tests() -> Result<()> { let test_filter = TestFilterBuilder::any(RunIgnored::Default); let test_bins: Vec<_> = FIXTURE_TARGETS.values().cloned().collect(); - let test_list = TestList::new(test_bins, &test_filter)?; + let test_list = TestList::new(test_bins, &test_filter, None)?; for (name, expected) in &*EXPECTED_TESTS { let test_binary = FIXTURE_TARGETS @@ -210,14 +210,15 @@ impl fmt::Debug for InstanceStatus { fn test_run() -> Result<()> { let test_filter = TestFilterBuilder::any(RunIgnored::Default); let test_bins: Vec<_> = FIXTURE_TARGETS.values().cloned().collect(); - let test_list = TestList::new(test_bins, &test_filter)?; + let test_list = TestList::new(test_bins, &test_filter, None)?; let config = NextestConfig::from_sources(&workspace_root(), None).expect("loaded fixture config"); let profile = config .profile(NextestConfig::DEFAULT_PROFILE) .expect("default config is valid"); - let runner = TestRunnerBuilder::default().build(&test_list, &profile, SignalHandler::noop()); + let runner = + TestRunnerBuilder::default().build(&test_list, &profile, SignalHandler::noop(), None); let (instance_statuses, run_stats) = execute_collect(&runner); @@ -259,14 +260,15 @@ fn test_run() -> Result<()> { fn test_run_ignored() -> Result<()> { let test_filter = TestFilterBuilder::any(RunIgnored::IgnoredOnly); let test_bins: Vec<_> = FIXTURE_TARGETS.values().cloned().collect(); - let test_list = TestList::new(test_bins, &test_filter)?; + let test_list = TestList::new(test_bins, &test_filter, None)?; let config = NextestConfig::from_sources(&workspace_root(), None).expect("loaded fixture config"); let profile = config .profile(NextestConfig::DEFAULT_PROFILE) .expect("default config is valid"); - let runner = TestRunnerBuilder::default().build(&test_list, &profile, SignalHandler::noop()); + let runner = + TestRunnerBuilder::default().build(&test_list, &profile, SignalHandler::noop(), None); let (instance_statuses, run_stats) = execute_collect(&runner); @@ -308,7 +310,7 @@ fn test_run_ignored() -> Result<()> { fn test_retries() -> Result<()> { let test_filter = TestFilterBuilder::any(RunIgnored::Default); let test_bins: Vec<_> = FIXTURE_TARGETS.values().cloned().collect(); - let test_list = TestList::new(test_bins, &test_filter)?; + let test_list = TestList::new(test_bins, &test_filter, None)?; let config = NextestConfig::from_sources(&workspace_root(), None).expect("loaded fixture config"); let profile = config @@ -318,7 +320,8 @@ fn test_retries() -> Result<()> { let retries = profile.retries(); assert_eq!(retries, 2, "retries set in with-retries profile"); - let runner = TestRunnerBuilder::default().build(&test_list, &profile, SignalHandler::noop()); + let runner = + TestRunnerBuilder::default().build(&test_list, &profile, SignalHandler::noop(), None); let (instance_statuses, run_stats) = execute_collect(&runner); From 36a31f5537e64ebd9391275f31ba1ba0e6fd57cf Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Mon, 21 Feb 2022 07:43:21 +0100 Subject: [PATCH 2/9] Support old <1.39 cargo versions --- nextest-runner/src/target_runner.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/nextest-runner/src/target_runner.rs b/nextest-runner/src/target_runner.rs index 8dd15d4296e..530d2c323ea 100644 --- a/nextest-runner/src/target_runner.rs +++ b/nextest-runner/src/target_runner.rs @@ -61,26 +61,24 @@ impl TargetRunner { Utf8PathBuf::from_path_buf(home).map_err(TargetRunnerError::NonUtf8Path) })?; - // Note versions of cargo older than 1.39 only support .cargo/config without - // the toml extension, but since this repo states its MSRV as 1.54 it should - // be ok to lose that particular piece of backwards compatibility let mut configs = Vec::new(); fn read_config_dir(dir: &mut Utf8PathBuf) -> Option { - dir.push("config.toml"); + // Check for config before config.toml, same as cargo does + dir.push("config"); if !dir.exists() { dir.set_extension("toml"); } - if dir.exists() { - let ret = dir.clone(); - dir.pop(); - Some(ret) + let ret = if dir.exists() { + Some(dir.clone()) } else { - dir.pop(); None - } + }; + + dir.pop(); + ret } let mut dir = root From d5e6e7f5286855205e8d9fa94f33df01594596b7 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Mon, 21 Feb 2022 07:43:28 +0100 Subject: [PATCH 3/9] Fixup errors --- nextest-runner/src/errors.rs | 52 ++++++++++++++++++++--------- nextest-runner/src/target_runner.rs | 28 +++++++++++----- 2 files changed, 56 insertions(+), 24 deletions(-) diff --git a/nextest-runner/src/errors.rs b/nextest-runner/src/errors.rs index 67ec8fc76e3..179d7f3f528 100644 --- a/nextest-runner/src/errors.rs +++ b/nextest-runner/src/errors.rs @@ -414,19 +414,39 @@ pub enum TargetRunnerError { UnableToDetermineHostTriple, /// An environment variable contained non-utf8 content InvalidEnvironmentVar(String), - /// An environment variable or key was found that matches the target triple, - /// but it didn't actually contain a binary - BinaryNotSpecified(String, String), + /// An environment variable or config key was found that matches the target + /// triple, but it didn't actually contain a binary + BinaryNotSpecified { + /// The environment variable or config key path + key: String, + /// The value that was read from the key + value: String, + }, /// Failed to retrieve a directory UnableToReadDir(std::io::Error), /// Failed to canonicalize a path - FailedPathCanonicalization(Utf8PathBuf, std::io::Error), + FailedPathCanonicalization { + /// The path that failed to canonicalize + path: Utf8PathBuf, + /// The error the occurred during canonicalization + error: std::io::Error, + }, /// A path was non-utf8 NonUtf8Path(std::path::PathBuf), /// Failed to read config file - FailedToReadConfig(Utf8PathBuf, std::io::Error), + FailedToReadConfig { + /// The path of the config file + path: Utf8PathBuf, + /// The error that occurred trying to read the config file + error: std::io::Error, + }, /// Failed to deserialize config file - FailedToParseConfig(Utf8PathBuf, toml::de::Error), + FailedToParseConfig { + /// The path of the config file + path: Utf8PathBuf, + /// The error that occurred trying to deserialize the config file + error: toml::de::Error, + }, } impl fmt::Display for TargetRunnerError { @@ -436,7 +456,7 @@ impl fmt::Display for TargetRunnerError { Self::InvalidEnvironmentVar(key) => { write!(f, "environment variable '{}' contained non-utf8 data", key) } - Self::BinaryNotSpecified(key, value) => { + Self::BinaryNotSpecified { key, value } => { write!( f, "runner '{}' = '{}' did not contain a runner binary", @@ -446,17 +466,17 @@ impl fmt::Display for TargetRunnerError { Self::UnableToReadDir(io) => { write!(f, "unable to read directory: {}", io) } - Self::FailedPathCanonicalization(path, err) => { - write!(f, "failed to canonicalize path '{}': {}", path, err) + Self::FailedPathCanonicalization { path, error } => { + write!(f, "failed to canonicalize path '{}': {}", path, error) } Self::NonUtf8Path(path) => { write!(f, "path '{}' is non-utf8", path.display()) } - Self::FailedToReadConfig(path, err) => { - write!(f, "failed to read '{}': {}", path, err) + Self::FailedToReadConfig { path, error } => { + write!(f, "failed to read '{}': {}", path, error) } - Self::FailedToParseConfig(path, err) => { - write!(f, "failed to parse '{}': {}", path, err) + Self::FailedToParseConfig { path, error } => { + write!(f, "failed to parse '{}': {}", path, error) } } } @@ -466,9 +486,9 @@ impl error::Error for TargetRunnerError { fn source(&self) -> Option<&(dyn error::Error + 'static)> { match self { Self::UnableToReadDir(io) => Some(io), - Self::FailedPathCanonicalization(_, io) => Some(io), - Self::FailedToReadConfig(_, io) => Some(io), - Self::FailedToParseConfig(_, toml) => Some(toml), + Self::FailedPathCanonicalization { error, .. } => Some(error), + Self::FailedToReadConfig { error, .. } => Some(error), + Self::FailedToParseConfig { error, .. } => Some(error), _ => None, } } diff --git a/nextest-runner/src/target_runner.rs b/nextest-runner/src/target_runner.rs index 530d2c323ea..85cfc3bda3a 100644 --- a/nextest-runner/src/target_runner.rs +++ b/nextest-runner/src/target_runner.rs @@ -83,7 +83,7 @@ impl TargetRunner { let mut dir = root .canonicalize() - .map_err(|io| TargetRunnerError::FailedPathCanonicalization(root, io)) + .map_err(|error| TargetRunnerError::FailedPathCanonicalization { path: root, error }) .and_then(|canon| { Utf8PathBuf::from_path_buf(canon).map_err(TargetRunnerError::NonUtf8Path) })?; @@ -137,10 +137,18 @@ impl TargetRunner { // all the matches, but in reverse order as the closer the config is // to our current working directory, the higher precedence it has 'config: for config_path in configs.into_iter().rev() { - let config_contents = std::fs::read_to_string(&config_path) - .map_err(|io| TargetRunnerError::FailedToReadConfig(config_path.clone(), io))?; - let config: CargoConfig = toml::from_str(&config_contents) - .map_err(|io| TargetRunnerError::FailedToParseConfig(config_path.clone(), io))?; + let config_contents = std::fs::read_to_string(&config_path).map_err(|error| { + TargetRunnerError::FailedToReadConfig { + path: config_path.clone(), + error, + } + })?; + let config: CargoConfig = toml::from_str(&config_contents).map_err(|error| { + TargetRunnerError::FailedToParseConfig { + path: config_path.clone(), + error, + } + })?; if let Some(mut targets) = config.target { // First lookup by the exact triple, as that one always takes precedence @@ -186,9 +194,13 @@ impl TargetRunner { // but I believe that cargo doesn't do that either let mut runner_iter = value.split_whitespace(); - let runner_binary = runner_iter - .next() - .ok_or_else(|| TargetRunnerError::BinaryNotSpecified(key.to_owned(), value.clone()))?; + let runner_binary = + runner_iter + .next() + .ok_or_else(|| TargetRunnerError::BinaryNotSpecified { + key: key.to_owned(), + value: value.clone(), + })?; let args = runner_iter.map(String::from).collect(); Ok(Self { From 117cd41154b17cd032522aabdb808a5e271a7fb8 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Mon, 21 Feb 2022 08:07:58 +0100 Subject: [PATCH 4/9] Replace use of cfg_expr with target-spec --- Cargo.lock | 2 +- nextest-runner/Cargo.toml | 4 +- nextest-runner/src/errors.rs | 20 ++++++-- nextest-runner/src/target_runner.rs | 72 ++++++++++++++--------------- 4 files changed, 55 insertions(+), 43 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a0eb90bbeda..6d393a7eefc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -642,7 +642,6 @@ dependencies = [ "aho-corasick", "camino", "cargo_metadata", - "cfg-expr", "chrono", "color-eyre", "config", @@ -668,6 +667,7 @@ dependencies = [ "serde", "serde_json", "strip-ansi-escapes", + "target-spec", "toml", "twox-hash", ] diff --git a/nextest-runner/Cargo.toml b/nextest-runner/Cargo.toml index cc77eae0fb1..1c789ad8733 100644 --- a/nextest-runner/Cargo.toml +++ b/nextest-runner/Cargo.toml @@ -14,8 +14,6 @@ aho-corasick = "0.7.18" camino = { version = "1.0.7", features = ["serde1"] } config = { version = "0.11.0", default-features = false, features = ["toml"] } cargo_metadata = "0.14.1" -# For cfg expression evaluation for [target.'cfg()'] expressions -cfg-expr = { version = "0.10.1", features = ["targets"] } chrono = "0.4.19" crossbeam-channel = "0.5.2" ctrlc = { version = "3.2.1", features = ["termination"] } @@ -34,6 +32,8 @@ rayon = "1.5.1" serde = { version = "1.0.136", features = ["derive"] } serde_json = "1.0.79" strip-ansi-escapes = "0.1.1" +# For cfg expression evaluation for [target.'cfg()'] expressions +target-spec = "1.0" # For parsing of .cargo/config.toml files toml = "0.5.8" twox-hash = { version = "1.6.2", default-features = false } diff --git a/nextest-runner/src/errors.rs b/nextest-runner/src/errors.rs index 179d7f3f528..63ab201b21b 100644 --- a/nextest-runner/src/errors.rs +++ b/nextest-runner/src/errors.rs @@ -411,7 +411,7 @@ impl error::Error for JunitError { pub enum TargetRunnerError { /// Failed to determine the host triple, which is needed to determine the /// default target triple when a target is not explicitly specified - UnableToDetermineHostTriple, + UnknownHostPlatform(target_spec::Error), /// An environment variable contained non-utf8 content InvalidEnvironmentVar(String), /// An environment variable or config key was found that matches the target @@ -447,12 +447,21 @@ pub enum TargetRunnerError { /// The error that occurred trying to deserialize the config file error: toml::de::Error, }, + /// Failed to parse the specified target triple + FailedToParseTargetTriple { + /// The triple that failed to parse + triple: String, + /// The error that occurred parsing the triple + error: target_spec::errors::TripleParseError, + }, } impl fmt::Display for TargetRunnerError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Self::UnableToDetermineHostTriple => f.write_str("unable to determine host triple"), + Self::UnknownHostPlatform(error) => { + write!(f, "unable to determine host triple: {}", error) + } Self::InvalidEnvironmentVar(key) => { write!(f, "environment variable '{}' contained non-utf8 data", key) } @@ -476,7 +485,10 @@ impl fmt::Display for TargetRunnerError { write!(f, "failed to read '{}': {}", path, error) } Self::FailedToParseConfig { path, error } => { - write!(f, "failed to parse '{}': {}", path, error) + write!(f, "failed to parse config '{}': {}", path, error) + } + Self::FailedToParseTargetTriple { triple, error } => { + write!(f, "failed to parse triple '{}': {}", triple, error) } } } @@ -485,10 +497,12 @@ impl fmt::Display for TargetRunnerError { impl error::Error for TargetRunnerError { fn source(&self) -> Option<&(dyn error::Error + 'static)> { match self { + Self::UnknownHostPlatform(error) => Some(error), Self::UnableToReadDir(io) => Some(io), Self::FailedPathCanonicalization { error, .. } => Some(error), Self::FailedToReadConfig { error, .. } => Some(error), Self::FailedToParseConfig { error, .. } => Some(error), + Self::FailedToParseTargetTriple { error, .. } => Some(error), _ => None, } } diff --git a/nextest-runner/src/target_runner.rs b/nextest-runner/src/target_runner.rs index 85cfc3bda3a..19e1a8a58d6 100644 --- a/nextest-runner/src/target_runner.rs +++ b/nextest-runner/src/target_runner.rs @@ -17,11 +17,19 @@ impl TargetRunner { /// which can be set in a [.cargo/config.toml](https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure) /// or via a `CARGO_TARGET_{TRIPLE}_RUNNER` environment variable pub fn for_target(target_triple: Option<&str>) -> Result, TargetRunnerError> { - use std::borrow::Cow; - - let target_triple: Cow<'_, str> = match target_triple { - Some(target) => Cow::Borrowed(target), - None => Cow::Owned(cfg_expr::target_lexicon::HOST.to_string()), + let target = match target_triple { + Some(target) => target_spec::Platform::from_triple( + target_spec::Triple::new(target.to_owned()).map_err(|error| { + TargetRunnerError::FailedToParseTargetTriple { + triple: target.to_owned(), + error, + } + })?, + target_spec::TargetFeatures::Unknown, + ), + None => { + target_spec::Platform::current().map_err(TargetRunnerError::UnknownHostPlatform)? + } }; // First check if have a CARGO_TARGET_{TRIPLE}_RUNNER environment variable @@ -29,7 +37,7 @@ impl TargetRunner { { let env_key = format!( "CARGO_TARGET_{}_RUNNER", - target_triple.to_ascii_uppercase().replace('-', "_") + target.triple_str().to_ascii_uppercase().replace('-', "_") ); if let Some(runner_var) = std::env::var_os(&env_key) { @@ -40,10 +48,10 @@ impl TargetRunner { } } - Self::find_config(&target_triple) + Self::find_config(target) } - fn find_config(target_triple: &str) -> Result, TargetRunnerError> { + fn find_config(target: target_spec::Platform) -> Result, TargetRunnerError> { // This is a bit non-intuitive, but the .cargo/config.toml hierarchy is actually // based on the current working directory, _not_ the manifest path, this bug // has existed for a while https://github.com/rust-lang/cargo/issues/2930 @@ -125,12 +133,7 @@ impl TargetRunner { let mut target_runner = None; - let triple_runner_key = format!("target.{}.runner", target_triple); - // Attempt to get the target info from a triple, this can fail if the - // target is actually a .json target spec, or is otherwise of a form - // that target_lexicon is unable to parse, which can happen with newer - // niche targets - let target_info: Option = target_triple.parse().ok(); + let triple_runner_key = format!("target.{}.runner", target.triple_str()); // Now that we've found all of the config files that could declare // a runner that matches our target triple, we need to actually find @@ -152,35 +155,30 @@ impl TargetRunner { if let Some(mut targets) = config.target { // First lookup by the exact triple, as that one always takes precedence - if let Some(target) = targets.remove(target_triple) { + if let Some(target) = targets.remove(target.triple_str()) { if let Some(runner) = target.runner { target_runner = Some(Self::parse_runner(&triple_runner_key, runner)?); continue; } } - if let Some(target_info) = &target_info { - // Next check if there are target.'cfg(..)' expressions that match - // the target. cargo states that it is not allowed for more than - // 1 cfg runner to match the target, but we let cargo handle that - // error itself, we just use the first one that matches - for (cfg, runner) in targets.into_iter().filter_map(|(k, v)| match v.runner { - Some(runner) if k.starts_with("cfg(") => Some((k, runner)), - _ => None, - }) { - // Treat these as non-fatal, but would be good to log maybe - let expr = match cfg_expr::Expression::parse(&cfg) { - Ok(expr) => expr, - Err(_err) => continue, - }; - - if expr.eval(|pred| match pred { - cfg_expr::Predicate::Target(tp) => tp.matches(target_info), - _ => false, - }) { - target_runner = Some(Self::parse_runner(&triple_runner_key, runner)?); - continue 'config; - } + // Next check if there are target.'cfg(..)' expressions that match + // the target. cargo states that it is not allowed for more than + // 1 cfg runner to match the target, but we let cargo handle that + // error itself, we just use the first one that matches + for (cfg, runner) in targets.into_iter().filter_map(|(k, v)| match v.runner { + Some(runner) if k.starts_with("cfg(") => Some((k, runner)), + _ => None, + }) { + // Treat these as non-fatal, but would be good to log maybe + let expr = match target_spec::TargetExpression::new(&cfg) { + Ok(expr) => expr, + Err(_err) => continue, + }; + + if expr.eval(&target) == Some(true) { + target_runner = Some(Self::parse_runner(&triple_runner_key, runner)?); + continue 'config; } } } From 979da6bd5117b3e08201452fc1d019749a754d5a Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Mon, 21 Feb 2022 08:19:38 +0100 Subject: [PATCH 5/9] Add target runner to runner builder --- cargo-nextest/src/dispatch.rs | 10 ++++------ nextest-runner/src/runner.rs | 19 ++++++++++++++----- nextest-runner/src/target_runner.rs | 3 ++- nextest-runner/tests/basic.rs | 9 +++------ 4 files changed, 23 insertions(+), 18 deletions(-) diff --git a/cargo-nextest/src/dispatch.rs b/cargo-nextest/src/dispatch.rs index d98a1479843..568bf882dff 100644 --- a/cargo-nextest/src/dispatch.rs +++ b/cargo-nextest/src/dispatch.rs @@ -371,12 +371,10 @@ impl AppImpl { } let handler = SignalHandler::new().wrap_err("failed to set up Ctrl-C handler")?; - let runner = runner_opts.to_builder(no_capture).build( - &test_list, - &profile, - handler, - target_runner.as_ref(), - ); + let mut runner_builder = runner_opts.to_builder(no_capture); + runner_builder.set_target_runner(target_runner); + + let runner = runner_builder.build(&test_list, &profile, handler); let stderr = std::io::stderr(); let mut writer = BufWriter::new(stderr); let run_stats = runner.try_execute(|event| { diff --git a/nextest-runner/src/runner.rs b/nextest-runner/src/runner.rs index be39b591970..b018dc9efd0 100644 --- a/nextest-runner/src/runner.rs +++ b/nextest-runner/src/runner.rs @@ -33,6 +33,7 @@ pub struct TestRunnerBuilder { retries: Option, fail_fast: Option, test_threads: Option, + target_runner: Option, } impl TestRunnerBuilder { @@ -62,13 +63,19 @@ impl TestRunnerBuilder { self } + /// Sets the target specific runner to use, instead of trying to execute + /// the binary natively + pub fn set_target_runner(&mut self, target_runner: Option) -> &mut Self { + self.target_runner = target_runner; + self + } + /// Creates a new test runner. pub fn build<'a>( - &self, + self, test_list: &'a TestList, profile: &NextestProfile<'_>, handler: SignalHandler, - target_runner: Option<&'a TargetRunner>, ) -> TestRunner<'a> { let test_threads = match self.no_capture { true => 1, @@ -77,6 +84,8 @@ impl TestRunnerBuilder { let retries = self.retries.unwrap_or_else(|| profile.retries()); let fail_fast = self.fail_fast.unwrap_or_else(|| profile.fail_fast()); let slow_timeout = profile.slow_timeout(); + let target_runner = self.target_runner; + TestRunner { no_capture: self.no_capture, // The number of tries = retries + 1. @@ -110,7 +119,7 @@ pub struct TestRunner<'a> { fail_fast: bool, slow_timeout: Duration, test_list: &'a TestList<'a>, - target_runner: Option<&'a TargetRunner>, + target_runner: Option, run_pool: ThreadPool, wait_pool: ThreadPool, handler: SignalHandler, @@ -342,7 +351,7 @@ impl<'a> TestRunner<'a> { run_sender: &Sender>, ) -> std::io::Result { let cmd = test - .make_expression(self.target_runner) + .make_expression(self.target_runner.as_ref()) .unchecked() // Debug environment variable for testing. .env("__NEXTEST_ATTEMPT", format!("{}", attempt)); @@ -823,7 +832,7 @@ mod tests { let config = NextestConfig::default_config("/fake/dir"); let profile = config.profile(NextestConfig::DEFAULT_PROFILE).unwrap(); let handler = SignalHandler::noop(); - let runner = builder.build(&test_list, &profile, handler, None); + let runner = builder.build(&test_list, &profile, handler); assert!(runner.no_capture, "no_capture is true"); assert_eq!( runner.run_pool.current_num_threads(), diff --git a/nextest-runner/src/target_runner.rs b/nextest-runner/src/target_runner.rs index 19e1a8a58d6..aa3a2c1569b 100644 --- a/nextest-runner/src/target_runner.rs +++ b/nextest-runner/src/target_runner.rs @@ -5,6 +5,7 @@ use camino::Utf8PathBuf; /// A [target runner](https://doc.rust-lang.org/cargo/reference/config.html#targettriplerunner) /// used to execute a test binary rather than the default of executing natively +#[derive(Debug)] pub struct TargetRunner { /// This is required runner_binary: Utf8PathBuf, @@ -215,7 +216,7 @@ impl TargetRunner { /// binary found in `PATH` #[inline] pub fn binary(&self) -> &str { - &self.runner_binary.as_str() + self.runner_binary.as_str() } /// Gets the (optional) runner binary arguments diff --git a/nextest-runner/tests/basic.rs b/nextest-runner/tests/basic.rs index f68df5fca20..f8b9aa45fd4 100644 --- a/nextest-runner/tests/basic.rs +++ b/nextest-runner/tests/basic.rs @@ -217,8 +217,7 @@ fn test_run() -> Result<()> { .profile(NextestConfig::DEFAULT_PROFILE) .expect("default config is valid"); - let runner = - TestRunnerBuilder::default().build(&test_list, &profile, SignalHandler::noop(), None); + let runner = TestRunnerBuilder::default().build(&test_list, &profile, SignalHandler::noop()); let (instance_statuses, run_stats) = execute_collect(&runner); @@ -267,8 +266,7 @@ fn test_run_ignored() -> Result<()> { .profile(NextestConfig::DEFAULT_PROFILE) .expect("default config is valid"); - let runner = - TestRunnerBuilder::default().build(&test_list, &profile, SignalHandler::noop(), None); + let runner = TestRunnerBuilder::default().build(&test_list, &profile, SignalHandler::noop()); let (instance_statuses, run_stats) = execute_collect(&runner); @@ -320,8 +318,7 @@ fn test_retries() -> Result<()> { let retries = profile.retries(); assert_eq!(retries, 2, "retries set in with-retries profile"); - let runner = - TestRunnerBuilder::default().build(&test_list, &profile, SignalHandler::noop(), None); + let runner = TestRunnerBuilder::default().build(&test_list, &profile, SignalHandler::noop()); let (instance_statuses, run_stats) = execute_collect(&runner); From 2dc9e69bbaafa0dd262c03eaa9fda9f5f5d24d08 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Mon, 21 Feb 2022 08:40:44 +0100 Subject: [PATCH 6/9] Add support for NEXTEST_{TRIPLE}_RUNNER --- nextest-runner/src/target_runner.rs | 36 +++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/nextest-runner/src/target_runner.rs b/nextest-runner/src/target_runner.rs index aa3a2c1569b..cab4096acce 100644 --- a/nextest-runner/src/target_runner.rs +++ b/nextest-runner/src/target_runner.rs @@ -33,25 +33,47 @@ impl TargetRunner { } }; - // First check if have a CARGO_TARGET_{TRIPLE}_RUNNER environment variable - // set, and if so use that, as it takes precedence over the static config.toml + // Check for a nextest specific runner, this is highest precedence + { + let env_key = format!( + "NEXTEST_{}_RUNNER", + target.triple_str().to_ascii_uppercase().replace('-', "_") + ); + + if let Some(tr) = Self::from_env(env_key)? { + return Ok(Some(tr)); + } + } + + // Next check for a config in the nextest.toml config + + // Next check if have a CARGO_TARGET_{TRIPLE}_RUNNER environment variable + // set, and if so use that, as it takes precedence over the static config(:?.toml)? { let env_key = format!( "CARGO_TARGET_{}_RUNNER", target.triple_str().to_ascii_uppercase().replace('-', "_") ); - if let Some(runner_var) = std::env::var_os(&env_key) { - let runner = runner_var - .into_string() - .map_err(|_osstr| TargetRunnerError::InvalidEnvironmentVar(env_key.clone()))?; - return Self::parse_runner(&env_key, runner).map(Some); + if let Some(tr) = Self::from_env(env_key)? { + return Ok(Some(tr)); } } Self::find_config(target) } + fn from_env(env_key: String) -> Result, TargetRunnerError> { + if let Some(runner_var) = std::env::var_os(&env_key) { + let runner = runner_var + .into_string() + .map_err(|_osstr| TargetRunnerError::InvalidEnvironmentVar(env_key.clone()))?; + Self::parse_runner(&env_key, runner).map(Some) + } else { + Ok(None) + } + } + fn find_config(target: target_spec::Platform) -> Result, TargetRunnerError> { // This is a bit non-intuitive, but the .cargo/config.toml hierarchy is actually // based on the current working directory, _not_ the manifest path, this bug From fc6baa0c66963b9cd0f8252a0ccc2f3ed1af5dbe Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Mon, 21 Feb 2022 13:47:37 +0100 Subject: [PATCH 7/9] Oops, fix arg ordering --- nextest-runner/src/test_list.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nextest-runner/src/test_list.rs b/nextest-runner/src/test_list.rs index 3fd236c2944..34d915d89e5 100644 --- a/nextest-runner/src/test_list.rs +++ b/nextest-runner/src/test_list.rs @@ -446,8 +446,8 @@ impl<'g> RustTestArtifact<'g> { let mut argv = Vec::new(); let program: std::ffi::OsString = if let Some(runner) = runner { - argv.push(self.binary_path.as_str()); argv.extend(runner.args()); + argv.push(self.binary_path.as_str()); runner.binary().into() } else { use duct::IntoExecutablePath; @@ -514,8 +514,8 @@ impl<'a> TestInstance<'a> { let program: std::ffi::OsString = match target_runner { Some(tr) => { - args.push(self.binary.as_str()); args.extend(tr.args()); + args.push(self.binary.as_str()); tr.binary().into() } None => { From 1cb342387190031631079a801471ae65cc00053c Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Mon, 21 Feb 2022 13:50:05 +0100 Subject: [PATCH 8/9] Add target runner tests --- fixtures/nextest-tests/.cargo/config | 8 ++ fixtures/passthrough | 9 ++ nextest-runner/src/target_runner.rs | 110 +++++++++------ nextest-runner/tests/basic.rs | 132 +++++++++++++++++ nextest-runner/tests/target_runner.rs | 196 ++++++++++++++++++++++++++ 5 files changed, 412 insertions(+), 43 deletions(-) create mode 100644 fixtures/nextest-tests/.cargo/config create mode 100755 fixtures/passthrough create mode 100644 nextest-runner/tests/target_runner.rs diff --git a/fixtures/nextest-tests/.cargo/config b/fixtures/nextest-tests/.cargo/config new file mode 100644 index 00000000000..fdd755c4b5b --- /dev/null +++ b/fixtures/nextest-tests/.cargo/config @@ -0,0 +1,8 @@ +[target.x86_64-pc-windows-gnu] +runner = "wine" + +[target.'cfg(target_os = "android")'] +runner = "android-runner -x" + +[target.'cfg(all(target_arch = "x86_64", target_os = "linux", target_env = "musl"))'] +runner = "passthrough --ensure-this-arg-is-sent" diff --git a/fixtures/passthrough b/fixtures/passthrough new file mode 100755 index 00000000000..9a0859be32b --- /dev/null +++ b/fixtures/passthrough @@ -0,0 +1,9 @@ +#!/usr/bin/env bash +if [[ "$1" != "--ensure-this-arg-is-sent" ]] ; then + exit 1 +fi + +bin="$2" +shift 2 + +$bin "$@" diff --git a/nextest-runner/src/target_runner.rs b/nextest-runner/src/target_runner.rs index cab4096acce..1f8062b58ba 100644 --- a/nextest-runner/src/target_runner.rs +++ b/nextest-runner/src/target_runner.rs @@ -18,6 +18,26 @@ impl TargetRunner { /// which can be set in a [.cargo/config.toml](https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure) /// or via a `CARGO_TARGET_{TRIPLE}_RUNNER` environment variable pub fn for_target(target_triple: Option<&str>) -> Result, TargetRunnerError> { + Self::get_runner_by_precedence(target_triple, true, None) + } + + /// Configures the root directory that starts the search for cargo configs. + /// + /// The default is normally the current working directory, but this method + /// is made available for testing purposes. + pub fn with_root( + target_triple: Option<&str>, + use_cargo_home: bool, + root: Utf8PathBuf, + ) -> Result, TargetRunnerError> { + Self::get_runner_by_precedence(target_triple, use_cargo_home, Some(root)) + } + + fn get_runner_by_precedence( + target_triple: Option<&str>, + use_cargo_home: bool, + root: Option, + ) -> Result, TargetRunnerError> { let target = match target_triple { Some(target) => target_spec::Platform::from_triple( target_spec::Triple::new(target.to_owned()).map_err(|error| { @@ -33,34 +53,36 @@ impl TargetRunner { } }; + let triple_str = target.triple_str().to_ascii_uppercase().replace('-', "_"); + // Check for a nextest specific runner, this is highest precedence - { - let env_key = format!( - "NEXTEST_{}_RUNNER", - target.triple_str().to_ascii_uppercase().replace('-', "_") - ); - - if let Some(tr) = Self::from_env(env_key)? { - return Ok(Some(tr)); - } + if let Some(tr) = Self::from_env(format!("NEXTEST_{}_RUNNER", triple_str))? { + return Ok(Some(tr)); } // Next check for a config in the nextest.toml config // Next check if have a CARGO_TARGET_{TRIPLE}_RUNNER environment variable // set, and if so use that, as it takes precedence over the static config(:?.toml)? - { - let env_key = format!( - "CARGO_TARGET_{}_RUNNER", - target.triple_str().to_ascii_uppercase().replace('-', "_") - ); - - if let Some(tr) = Self::from_env(env_key)? { - return Ok(Some(tr)); - } + if let Some(tr) = Self::from_env(format!("CARGO_TARGET_{}_RUNNER", triple_str))? { + return Ok(Some(tr)); } - Self::find_config(target) + let root = match root { + Some(rp) => rp, + None => { + // This is a bit non-intuitive, but the .cargo/config.toml hierarchy is actually + // based on the current working directory, _not_ the manifest path, this bug + // has existed for a while https://github.com/rust-lang/cargo/issues/2930 + std::env::current_dir() + .map_err(TargetRunnerError::UnableToReadDir) + .and_then(|cwd| { + Utf8PathBuf::from_path_buf(cwd).map_err(TargetRunnerError::NonUtf8Path) + })? + } + }; + + Self::find_config(target, use_cargo_home, root) } fn from_env(env_key: String) -> Result, TargetRunnerError> { @@ -74,24 +96,13 @@ impl TargetRunner { } } - fn find_config(target: target_spec::Platform) -> Result, TargetRunnerError> { - // This is a bit non-intuitive, but the .cargo/config.toml hierarchy is actually - // based on the current working directory, _not_ the manifest path, this bug - // has existed for a while https://github.com/rust-lang/cargo/issues/2930 - let root = std::env::current_dir() - .map_err(TargetRunnerError::UnableToReadDir) - .and_then(|cwd| { - Utf8PathBuf::from_path_buf(cwd).map_err(TargetRunnerError::NonUtf8Path) - })?; - - // Attempt lookup the $CARGO_HOME directory from the cwd, as that can - // contain a default config.toml - let mut cargo_home_path = home::cargo_home_with_cwd(root.as_std_path()) - .map_err(TargetRunnerError::UnableToReadDir) - .and_then(|home| { - Utf8PathBuf::from_path_buf(home).map_err(TargetRunnerError::NonUtf8Path) - })?; - + /// Attempts to find a target runner for the specified target from a + /// [cargo config](https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure) + pub fn find_config( + target: target_spec::Platform, + use_cargo_home: bool, + root: Utf8PathBuf, + ) -> Result, TargetRunnerError> { let mut configs = Vec::new(); fn read_config_dir(dir: &mut Utf8PathBuf) -> Option { @@ -114,7 +125,10 @@ impl TargetRunner { let mut dir = root .canonicalize() - .map_err(|error| TargetRunnerError::FailedPathCanonicalization { path: root, error }) + .map_err(|error| TargetRunnerError::FailedPathCanonicalization { + path: root.clone(), + error, + }) .and_then(|canon| { Utf8PathBuf::from_path_buf(canon).map_err(TargetRunnerError::NonUtf8Path) })?; @@ -136,11 +150,21 @@ impl TargetRunner { dir.pop(); } - if let Some(home_config) = read_config_dir(&mut cargo_home_path) { - // Ensure we don't add a duplicate if the current directory is underneath - // the same root as $CARGO_HOME - if !configs.iter().any(|path| path == &home_config) { - configs.push(home_config); + if use_cargo_home { + // Attempt lookup the $CARGO_HOME directory from the cwd, as that can + // contain a default config.toml + let mut cargo_home_path = home::cargo_home_with_cwd(root.as_std_path()) + .map_err(TargetRunnerError::UnableToReadDir) + .and_then(|home| { + Utf8PathBuf::from_path_buf(home).map_err(TargetRunnerError::NonUtf8Path) + })?; + + if let Some(home_config) = read_config_dir(&mut cargo_home_path) { + // Ensure we don't add a duplicate if the current directory is underneath + // the same root as $CARGO_HOME + if !configs.iter().any(|path| path == &home_config) { + configs.push(home_config); + } } } diff --git a/nextest-runner/tests/basic.rs b/nextest-runner/tests/basic.rs index f8b9aa45fd4..6ceab591eb5 100644 --- a/nextest-runner/tests/basic.rs +++ b/nextest-runner/tests/basic.rs @@ -17,6 +17,7 @@ use nextest_runner::{ TestRunnerBuilder, }, signal::SignalHandler, + target_runner::TargetRunner, test_filter::{RunIgnored, TestFilterBuilder}, test_list::{RustTestArtifact, TestList}, }; @@ -439,3 +440,134 @@ fn execute_collect<'a>( (instance_statuses, run_stats) } + +mod target_runner; +use target_runner::with_env; + +fn passthrough_path() -> &'static Utf8Path { + static PP: once_cell::sync::OnceCell = once_cell::sync::OnceCell::new(); + PP.get_or_init(|| { + Utf8PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .unwrap() + .join("fixtures/passthrough") + }) +} + +#[cfg(all(target_arch = "x86_64", target_os = "linux", target_env = "gnu"))] +#[test] +fn test_listing_with_target_runner() -> Result<()> { + let test_filter = TestFilterBuilder::any(RunIgnored::Default); + let test_bins: Vec<_> = FIXTURE_TARGETS.values().cloned().collect(); + + let test_list = TestList::new(test_bins.clone(), &test_filter, None)?; + let bin_count = test_list.binary_count(); + let test_count = test_list.test_count(); + + { + let target_runner = with_env( + [( + "NEXTEST_X86_64_UNKNOWN_LINUX_GNU_RUNNER", + &format!("{} --ensure-this-arg-is-sent", passthrough_path()), + )], + || TargetRunner::for_target(None), + )? + .unwrap(); + + let test_list = TestList::new(test_bins.clone(), &test_filter, Some(&target_runner))?; + + assert_eq!(bin_count, test_list.binary_count()); + assert_eq!(test_count, test_list.test_count()); + } + + { + let target_runner = with_env( + [( + "CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER", + &format!("{} --ensure-this-arg-is-sent", passthrough_path()), + )], + || TargetRunner::for_target(None), + )? + .unwrap(); + + let test_list = TestList::new(test_bins.clone(), &test_filter, Some(&target_runner))?; + + assert_eq!(bin_count, test_list.binary_count()); + assert_eq!(test_count, test_list.test_count()); + } + + { + // cargo unfortunately doesn't handle relative paths for runner binaries, + // it will just assume they are in PATH if they are not absolute paths, + // and thus makes testing it a bit annoying, so we just punt and rely + // on the tests for parsing the runner in the proper precedence + } + + Ok(()) +} + +#[cfg(all(target_arch = "x86_64", target_os = "linux", target_env = "gnu"))] +#[test] +fn runs_tests() -> Result<()> { + let test_filter = TestFilterBuilder::any(RunIgnored::Default); + let test_bins: Vec<_> = FIXTURE_TARGETS.values().cloned().collect(); + + let target_runner = with_env( + [( + "CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER", + &format!("{} --ensure-this-arg-is-sent", passthrough_path()), + )], + || TargetRunner::for_target(None), + )? + .unwrap(); + + assert_eq!(passthrough_path(), target_runner.binary()); + + let test_list = TestList::new(test_bins, &test_filter, Some(&target_runner))?; + + let config = + NextestConfig::from_sources(&workspace_root(), None).expect("loaded fixture config"); + let profile = config + .profile(NextestConfig::DEFAULT_PROFILE) + .expect("default config is valid"); + + let mut runner = TestRunnerBuilder::default(); + runner.set_target_runner(Some(target_runner)); + let runner = runner.build(&test_list, &profile, SignalHandler::noop()); + + let (instance_statuses, run_stats) = execute_collect(&runner); + + for (name, expected) in &*EXPECTED_TESTS { + let test_binary = FIXTURE_TARGETS + .get(*name) + .unwrap_or_else(|| panic!("unexpected test name {}", name)); + for fixture in expected { + let instance_value = + &instance_statuses[&(test_binary.binary_path.as_path(), fixture.name)]; + let valid = match &instance_value.status { + InstanceStatus::Skipped(_) => fixture.status.is_ignored(), + InstanceStatus::Finished(run_statuses) => { + // This test should not have been retried since retries aren't configured. + assert_eq!( + run_statuses.len(), + 1, + "test {} should have been run exactly once", + fixture.name + ); + let run_status = run_statuses.last_status(); + run_status.result == fixture.status.to_test_status(1) + } + }; + if !valid { + panic!( + "for test {}, mismatch in status: expected {:?}, actual {:?}", + fixture.name, fixture.status, instance_value.status + ); + } + } + } + + assert!(!run_stats.is_success(), "run should be marked failed"); + + Ok(()) +} diff --git a/nextest-runner/tests/target_runner.rs b/nextest-runner/tests/target_runner.rs new file mode 100644 index 00000000000..53ef78b4912 --- /dev/null +++ b/nextest-runner/tests/target_runner.rs @@ -0,0 +1,196 @@ +use camino::Utf8PathBuf; +use nextest_runner::{errors::TargetRunnerError, target_runner::TargetRunner}; +use once_cell::sync::OnceCell; +use std::{env, sync::Mutex}; + +fn env_mutex() -> &'static Mutex<()> { + static MUTEX: OnceCell> = OnceCell::new(); + MUTEX.get_or_init(|| Mutex::new(())) +} + +pub fn with_env( + vars: impl IntoIterator, impl AsRef)>, + func: impl FnOnce() -> Result, TargetRunnerError>, +) -> Result, TargetRunnerError> { + let lock = env_mutex().lock().unwrap(); + + let keys: Vec<_> = vars + .into_iter() + .map(|(key, val)| { + let key = key.into(); + env::set_var(&key, val.as_ref()); + key + }) + .collect(); + + let res = func(); + + for key in keys { + env::remove_var(key); + } + drop(lock); + + res +} + +fn default() -> &'static target_spec::Platform { + static DEF: OnceCell = OnceCell::new(); + DEF.get_or_init(|| target_spec::Platform::current().unwrap()) +} + +#[test] +fn parses_nextest_env() { + let def_runner = with_env( + [( + format!( + "NEXTEST_{}_RUNNER", + default() + .triple_str() + .to_ascii_uppercase() + .replace('-', "_") + ), + "nextest_with_default", + )], + || TargetRunner::for_target(None), + ) + .unwrap() + .unwrap(); + + assert_eq!("nextest_with_default", def_runner.binary()); + assert_eq!(0, def_runner.args().count()); + + let specific_runner = with_env( + [( + "NEXTEST_X86_64_PC_WINDOWS_MSVC_RUNNER", + "nextest_with_specific --arg1 -b", + )], + || TargetRunner::for_target(Some("x86_64-pc-windows-msvc")), + ) + .unwrap() + .unwrap(); + + assert_eq!("nextest_with_specific", specific_runner.binary()); + assert_eq!( + vec!["--arg1", "-b"], + specific_runner.args().collect::>() + ); +} + +#[test] +fn parses_cargo_env() { + let def_runner = with_env( + [( + format!( + "CARGO_TARGET_{}_RUNNER", + default() + .triple_str() + .to_ascii_uppercase() + .replace('-', "_") + ), + "cargo_with_default --arg --arg2", + )], + || TargetRunner::for_target(None), + ) + .unwrap() + .unwrap(); + + assert_eq!("cargo_with_default", def_runner.binary()); + assert_eq!( + vec!["--arg", "--arg2"], + def_runner.args().collect::>() + ); + + let specific_runner = with_env( + [( + "CARGO_TARGET_AARCH64_LINUX_ANDROID_RUNNER", + "cargo_with_specific", + )], + || TargetRunner::for_target(Some("aarch64-linux-android")), + ) + .unwrap() + .unwrap(); + + assert_eq!("cargo_with_specific", specific_runner.binary()); + assert_eq!(0, specific_runner.args().count()); +} + +/// Use fixtures/nextest-test as the root dir +fn root_dir() -> Utf8PathBuf { + Utf8PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .unwrap() + .join("fixtures/nextest-tests") +} + +fn parse_triple(triple: &'static str) -> target_spec::Platform { + target_spec::Platform::new(triple, target_spec::TargetFeatures::Unknown).unwrap() +} + +#[test] +fn parses_cargo_config_exact() { + let windows = parse_triple("x86_64-pc-windows-gnu"); + + let runner = TargetRunner::find_config(windows, false, root_dir()) + .unwrap() + .unwrap(); + + assert_eq!("wine", runner.binary()); + assert_eq!(0, runner.args().count()); +} + +#[test] +fn disregards_non_matching() { + let windows = parse_triple("x86_64-unknown-linux-gnu"); + assert!(TargetRunner::find_config(windows, false, root_dir()) + .unwrap() + .is_none()); +} + +#[test] +fn parses_cargo_config_cfg() { + let android = parse_triple("aarch64-linux-android"); + let runner = TargetRunner::find_config(android, false, root_dir()) + .unwrap() + .unwrap(); + + assert_eq!("android-runner", runner.binary()); + assert_eq!(vec!["-x"], runner.args().collect::>()); + + let linux = parse_triple("x86_64-unknown-linux-musl"); + let runner = TargetRunner::find_config(linux, false, root_dir()) + .unwrap() + .unwrap(); + + assert_eq!("passthrough", runner.binary()); + assert_eq!( + vec!["--ensure-this-arg-is-sent"], + runner.args().collect::>() + ); +} + +#[test] +fn fallsback_to_cargo_config() { + let linux = parse_triple("x86_64-unknown-linux-musl"); + + let runner = with_env( + [ + ( + "NEXTEST_X86_64_UNKNOWN_LINUX_GNU_RUNNER", + "nextest-runner-linux", + ), + ( + "CARGO_TARGET_X86_64_PC_WINDOWS_MSVC_RUNNER", + "cargo-runner-windows", + ), + ], + || TargetRunner::with_root(Some(linux.triple_str()), false, root_dir()), + ) + .unwrap() + .unwrap(); + + assert_eq!("passthrough", runner.binary()); + assert_eq!( + vec!["--ensure-this-arg-is-sent"], + runner.args().collect::>() + ); +} From 7d5f04cf144f3532f63b0ac2731e8a3308226310 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Tue, 22 Feb 2022 08:08:05 +0100 Subject: [PATCH 9/9] Fix test warnings --- nextest-runner/tests/basic.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/nextest-runner/tests/basic.rs b/nextest-runner/tests/basic.rs index 6ceab591eb5..662cc723ef0 100644 --- a/nextest-runner/tests/basic.rs +++ b/nextest-runner/tests/basic.rs @@ -441,9 +441,12 @@ fn execute_collect<'a>( (instance_statuses, run_stats) } +#[cfg(all(target_arch = "x86_64", target_os = "linux", target_env = "gnu"))] mod target_runner; +#[cfg(all(target_arch = "x86_64", target_os = "linux", target_env = "gnu"))] use target_runner::with_env; +#[cfg(all(target_arch = "x86_64", target_os = "linux", target_env = "gnu"))] fn passthrough_path() -> &'static Utf8Path { static PP: once_cell::sync::OnceCell = once_cell::sync::OnceCell::new(); PP.get_or_init(|| {