diff --git a/crates/turborepo-lib/src/cli/mod.rs b/crates/turborepo-lib/src/cli/mod.rs index b857bd34b8f6c..14b112ddb0ccd 100644 --- a/crates/turborepo-lib/src/cli/mod.rs +++ b/crates/turborepo-lib/src/cli/mod.rs @@ -1,4 +1,10 @@ -use std::{backtrace::Backtrace, env, fmt, fmt::Display, io, mem, process}; +use std::{ + backtrace::Backtrace, + env, + ffi::OsString, + fmt::{self, Display}, + io, mem, process, +}; use biome_deserialize_macros::Deserializable; use camino::{Utf8Path, Utf8PathBuf}; @@ -329,52 +335,9 @@ pub enum LinkTarget { } impl Args { - pub fn new() -> Self { - // We always pass --single-package in from the shim. - // We need to omit it, and then add it in for run. - let arg_separator_position = env::args_os().position(|input_token| input_token == "--"); - - let single_package_position = - env::args_os().position(|input_token| input_token == "--single-package"); - - let is_single_package = match (arg_separator_position, single_package_position) { - (_, None) => false, - (None, Some(_)) => true, - (Some(arg_separator_position), Some(single_package_position)) => { - single_package_position < arg_separator_position - } - }; - - // Clap supports arbitrary iterators as input. - // We can remove all instances of --single-package - let single_package_free = std::env::args_os() - .enumerate() - .filter(|(index, input_token)| { - arg_separator_position - .is_some_and(|arg_separator_position| index > &arg_separator_position) - || input_token != "--single-package" - }) - .map(|(_, input_token)| input_token); - - let mut clap_args = match Args::try_parse_from(single_package_free) { - Ok(mut args) => { - // And then only add them back in when we're in `run`. - // The value can appear in two places in the struct. - // We defensively attempt to set both. - if let Some(ref mut execution_args) = args.execution_args { - execution_args.single_package = is_single_package - } - - if let Some(Command::Run { - run_args: _, - ref mut execution_args, - }) = args.command - { - execution_args.single_package = is_single_package; - } - - args - } + pub fn new(os_args: Vec) -> Self { + let clap_args = match Args::parse(os_args) { + Ok(args) => args, // Don't use error logger when displaying help text Err(e) if matches!( @@ -409,10 +372,6 @@ impl Args { process::exit(0); } - if env::var("TEST_RUN").is_ok() { - clap_args.test_run = true; - } - if let Some(run_args) = clap_args.run_args() { if run_args.no_cache { warn!( @@ -437,6 +396,33 @@ impl Args { clap_args } + fn parse(os_args: Vec) -> Result { + let (is_single_package, single_package_free) = Self::remove_single_package(os_args); + let mut args = Args::try_parse_from(single_package_free)?; + // And then only add them back in when we're in `run`. + // The value can appear in two places in the struct. + // We defensively attempt to set both. + if let Some(ref mut execution_args) = args.execution_args { + execution_args.single_package = is_single_package + } + + if let Some(Command::Run { + run_args: _, + ref mut execution_args, + }) = args.command + { + execution_args.single_package = is_single_package; + } + + if env::var("TEST_RUN").is_ok() { + args.test_run = true; + } + + args.validate()?; + + Ok(args) + } + pub fn track(&self, tel: &GenericEventBuilder) { // track usage only track_usage!(tel, self.skip_infer, |val| val); @@ -493,6 +479,69 @@ impl Args { self.execution_args.as_ref() } } + + fn remove_single_package(args: Vec) -> (bool, impl Iterator) { + // We always pass --single-package in from the shim. + // We need to omit it, and then add it in for run. + let arg_separator_position = args.iter().position(|input_token| input_token == "--"); + + let single_package_position = args + .iter() + .position(|input_token| input_token == "--single-package"); + + let is_single_package = match (arg_separator_position, single_package_position) { + (_, None) => false, + (None, Some(_)) => true, + (Some(arg_separator_position), Some(single_package_position)) => { + single_package_position < arg_separator_position + } + }; + + // Clap supports arbitrary iterators as input. + // We can remove all instances of --single-package + let single_package_free = args + .into_iter() + .enumerate() + .filter(move |(index, input_token)| { + arg_separator_position + .is_some_and(|arg_separator_position| index > &arg_separator_position) + || input_token != "--single-package" + }) + .map(|(_, input_token)| input_token); + + (is_single_package, single_package_free) + } + + fn validate(&self) -> Result<(), clap::Error> { + if self.run_args.is_some() + && !matches!( + self.command, + None | Some(Command::Run { .. }) | Some(Command::Config) + ) + { + let mut cmd = Self::command(); + Err(cmd.error( + clap::error::ErrorKind::UnknownArgument, + "Cannot use run arguments outside of run command", + )) + } else if self.execution_args.is_some() && matches!(self.command, Some(Command::Watch(_))) { + let mut cmd = Self::command(); + Err(cmd.error( + clap::error::ErrorKind::ArgumentConflict, + "Cannot use watch arguments before `watch` subcommand", + )) + } else if matches!(self.command, Some(Command::Run { .. })) + && (self.run_args.is_some() || self.execution_args.is_some()) + { + let mut cmd = Self::command(); + Err(cmd.error( + clap::error::ErrorKind::ArgumentConflict, + "Cannot use run arguments before `run` subcommand", + )) + } else { + Ok(()) + } + } } /// Defines the subcommands for CLI @@ -1071,7 +1120,7 @@ pub async fn run( color_config: ColorConfig, ) -> Result { // TODO: remove mutability from this function - let mut cli_args = Args::new(); + let mut cli_args = Args::new(env::args_os().collect()); let version = get_version(); // track telemetry handle to close at the end of the run @@ -1462,10 +1511,11 @@ pub async fn run( #[cfg(test)] mod test { - use std::assert_matches::assert_matches; + use std::{assert_matches::assert_matches, ffi::OsString}; use camino::Utf8PathBuf; use clap::Parser; + use insta::assert_snapshot; use itertools::Itertools; use pretty_assertions::assert_eq; @@ -2725,4 +2775,126 @@ mod test { assert!(Args::try_parse_from(["turbo", "build", "--filter", "foo", "--affected"]).is_err(),); assert!(Args::try_parse_from(["turbo", "ls", "--filter", "foo", "--affected"]).is_err(),); } + + struct SinglePackageTestCase { + args: &'static [&'static str], + expected_is_single: bool, + expected: &'static [&'static str], + } + + impl SinglePackageTestCase { + pub const fn new(args: &'static [&'static str; N]) -> Self { + let args = args.as_slice(); + Self { + args, + expected_is_single: false, + expected: args, + } + } + + pub const fn expected( + mut self, + expected: &'static [&'static str; N], + ) -> Self { + self.expected_is_single = true; + self.expected = expected.as_slice(); + self + } + + pub fn os_args(&self) -> Vec { + self.args.iter().map(|s| OsString::from(*s)).collect() + } + + pub fn assert_actual(&self, actual: (bool, impl Iterator)) { + let (is_single, args) = actual; + assert_eq!(is_single, self.expected_is_single); + assert_eq!( + args.map(|s| s.to_str().unwrap().to_string()) + .collect::>() + .as_slice(), + self.expected + ); + } + } + + const NO_SINGLE_PKG: SinglePackageTestCase = + SinglePackageTestCase::new(&["turbo", "--version"]); + const SINGLE_PKG_AFTER_PASS: SinglePackageTestCase = + SinglePackageTestCase::new(&["turbo", "--", "--single-package"]); + const SINGLE_PKG: SinglePackageTestCase = + SinglePackageTestCase::new(&["turbo", "--single-package", "run", "build"]) + .expected(&["turbo", "run", "build"]); + const SINGLE_PKG_BEFORE_AFTER: SinglePackageTestCase = SinglePackageTestCase::new(&[ + "turbo", + "--single-package", + "run", + "build", + "--", + "--single-package", + ]) + .expected(&["turbo", "run", "build", "--", "--single-package"]); + + #[test_case::test_case(NO_SINGLE_PKG)] + #[test_case::test_case(SINGLE_PKG_AFTER_PASS)] + #[test_case::test_case(SINGLE_PKG)] + #[test_case::test_case(SINGLE_PKG_BEFORE_AFTER)] + fn test_single_package_removal(test: SinglePackageTestCase) { + let os_args = test.os_args(); + let actual = Args::remove_single_package(os_args); + test.assert_actual(actual); + } + + #[test] + fn test_set_single_package() { + let inferred_run = Args::parse( + ["turbo", "--single-package", "build"] + .iter() + .map(|s| OsString::from(*s)) + .collect(), + ) + .unwrap(); + let explicit_run = Args::parse( + ["turbo", "run", "--single-package", "build"] + .iter() + .map(|s| OsString::from(*s)) + .collect(), + ) + .unwrap(); + assert!(inferred_run + .execution_args + .as_ref() + .map_or(false, |e| e.single_package)); + assert!(explicit_run + .command + .as_ref() + .and_then(|cmd| if let Command::Run { execution_args, .. } = cmd { + Some(execution_args.single_package) + } else { + None + }) + .unwrap_or(false)); + } + + #[test_case::test_case(&["turbo", "watch", "build", "--no-daemon"]; "after watch")] + #[test_case::test_case(&["turbo", "--no-daemon", "watch", "build"]; "before watch")] + fn test_no_run_args_outside_of_run(args: &[&str]) { + let os_args = args.iter().map(|s| OsString::from(*s)).collect(); + let err = Args::parse(os_args).unwrap_err(); + assert_snapshot!(args.join("-").as_str(), err); + } + + #[test_case::test_case(&["turbo", "--filter=foo", "run", "build"], false; "execution args")] + #[test_case::test_case(&["turbo", "--no-daemon", "run", "build"], false; "run args")] + #[test_case::test_case(&["turbo", "build", "run"], true; "task")] + #[test_case::test_case(&["turbo", "--filter=web", "watch", "build"], false; "execution before watch")] + fn test_no_run_args_before_run(args: &[&str], is_okay: bool) { + let os_args = args.iter().map(|s| OsString::from(*s)).collect(); + let cli = Args::parse(os_args); + if is_okay { + cli.unwrap(); + } else { + let err = cli.unwrap_err(); + assert_snapshot!(args.join("-").as_str(), err); + } + } } diff --git a/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---filter=foo-run-build.snap b/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---filter=foo-run-build.snap new file mode 100644 index 0000000000000..e2b8f0018f0e2 --- /dev/null +++ b/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---filter=foo-run-build.snap @@ -0,0 +1,9 @@ +--- +source: crates/turborepo-lib/src/cli/mod.rs +expression: err +--- +error: Cannot use run arguments before `run` subcommand + +Usage: turbo [OPTIONS] [COMMAND] + +For more information, try '--help'. diff --git a/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---filter=web-watch-build.snap b/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---filter=web-watch-build.snap new file mode 100644 index 0000000000000..22101e293eda0 --- /dev/null +++ b/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---filter=web-watch-build.snap @@ -0,0 +1,9 @@ +--- +source: crates/turborepo-lib/src/cli/mod.rs +expression: err +--- +error: Cannot use watch arguments before `watch` subcommand + +Usage: turbo [OPTIONS] [COMMAND] + +For more information, try '--help'. diff --git a/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---no-daemon-run-build.snap b/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---no-daemon-run-build.snap new file mode 100644 index 0000000000000..e2b8f0018f0e2 --- /dev/null +++ b/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---no-daemon-run-build.snap @@ -0,0 +1,9 @@ +--- +source: crates/turborepo-lib/src/cli/mod.rs +expression: err +--- +error: Cannot use run arguments before `run` subcommand + +Usage: turbo [OPTIONS] [COMMAND] + +For more information, try '--help'. diff --git a/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---no-daemon-watch-build.snap b/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---no-daemon-watch-build.snap new file mode 100644 index 0000000000000..fccc5c2f000f7 --- /dev/null +++ b/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---no-daemon-watch-build.snap @@ -0,0 +1,9 @@ +--- +source: crates/turborepo-lib/src/cli/mod.rs +expression: err +--- +error: Cannot use run arguments outside of run command + +Usage: turbo [OPTIONS] [COMMAND] + +For more information, try '--help'. diff --git a/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo-watch-build---no-daemon.snap b/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo-watch-build---no-daemon.snap new file mode 100644 index 0000000000000..724f3e9a57ce2 --- /dev/null +++ b/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo-watch-build---no-daemon.snap @@ -0,0 +1,12 @@ +--- +source: crates/turborepo-lib/src/cli/mod.rs +expression: err +--- +error: unexpected argument '--no-daemon' found + + tip: a similar argument exists: '--no-update-notifier' + tip: to pass '--no-daemon' as a value, use '-- --no-daemon' + +Usage: turbo watch --no-update-notifier <--cache-dir |--concurrency |--continue|--single-package|--framework-inference []|--global-deps |--env-mode []|--filter |--affected|--output-logs |--log-order |--only|--pkg-inference-root |--log-prefix |TASKS|PASS_THROUGH_ARGS> + +For more information, try '--help'.