From 39da3917e56b2bbd6b034676122abb4f7b14765c Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Wed, 19 Jun 2024 10:55:21 -0400 Subject: [PATCH] Improve handling of command arguments in `uv run` and `uv tool run` (#4404) Closes https://github.com/astral-sh/uv/issues/4390 We no longer require `--` to disambiguate child command options that overlap with uv options. --- crates/uv/src/cli.rs | 40 ++++++++++++++----- crates/uv/src/commands/project/run.rs | 23 +++++------ crates/uv/src/commands/tool/run.rs | 38 +++++++++++------- crates/uv/src/main.rs | 6 +-- crates/uv/src/settings.rs | 29 ++++++-------- crates/uv/tests/common/mod.rs | 37 +++++++++++++++++- crates/uv/tests/run.rs | 55 +++++++++++++++++++++++++++ crates/uv/tests/tool_run.rs | 55 +++++++++++++++++++++++++++ 8 files changed, 226 insertions(+), 57 deletions(-) create mode 100644 crates/uv/tests/tool_run.rs diff --git a/crates/uv/src/cli.rs b/crates/uv/src/cli.rs index 1f26792ac..2de34d32a 100644 --- a/crates/uv/src/cli.rs +++ b/crates/uv/src/cli.rs @@ -1,4 +1,5 @@ use std::ffi::OsString; +use std::ops::Deref; use std::path::PathBuf; use std::str::FromStr; @@ -1457,6 +1458,31 @@ pub(crate) struct VenvArgs { pub(crate) compat_args: compat::VenvCompatArgs, } +#[derive(Parser, Debug, Clone)] +pub(crate) enum ExternalCommand { + #[command(external_subcommand)] + Cmd(Vec), +} + +impl Deref for ExternalCommand { + type Target = Vec; + + fn deref(&self) -> &Self::Target { + match self { + Self::Cmd(cmd) => cmd, + } + } +} + +impl ExternalCommand { + pub(crate) fn split(&self) -> (Option<&OsString>, &[OsString]) { + match self.as_slice() { + [] => (None, &[]), + [cmd, args @ ..] => (Some(cmd), args), + } + } +} + #[derive(Args)] #[allow(clippy::struct_excessive_bools)] pub(crate) struct RunArgs { @@ -1482,11 +1508,8 @@ pub(crate) struct RunArgs { pub(crate) no_dev: bool, /// The command to run. - pub(crate) target: Option, - - /// The arguments to the command. - #[arg(allow_hyphen_values = true)] - pub(crate) args: Vec, + #[command(subcommand)] + pub(crate) command: ExternalCommand, /// Run with the given packages installed. #[arg(long)] @@ -1684,11 +1707,8 @@ pub(crate) enum ToolCommand { #[allow(clippy::struct_excessive_bools)] pub(crate) struct ToolRunArgs { /// The command to run. - pub(crate) target: String, - - /// The arguments to the command. - #[arg(allow_hyphen_values = true)] - pub(crate) args: Vec, + #[command(subcommand)] + pub(crate) command: ExternalCommand, /// Use the given package to provide the command. /// diff --git a/crates/uv/src/commands/project/run.rs b/crates/uv/src/commands/project/run.rs index 74ad03824..04acb5ec1 100644 --- a/crates/uv/src/commands/project/run.rs +++ b/crates/uv/src/commands/project/run.rs @@ -15,6 +15,7 @@ use uv_requirements::RequirementsSource; use uv_toolchain::{PythonEnvironment, SystemPython, Toolchain, ToolchainRequest}; use uv_warnings::warn_user; +use crate::cli::ExternalCommand; use crate::commands::pip::operations::Modifications; use crate::commands::{project, ExitStatus}; use crate::printer::Printer; @@ -25,8 +26,7 @@ use crate::settings::ResolverInstallerSettings; pub(crate) async fn run( extras: ExtrasSpecification, dev: bool, - target: Option, - mut args: Vec, + command: ExternalCommand, requirements: Vec, python: Option, package: Option, @@ -179,25 +179,25 @@ pub(crate) async fn run( ) }; - // Construct the command - let command = if let Some(target) = target { + let (target, args) = command.split(); + let (command, prefix_args) = if let Some(target) = target { let target_path = PathBuf::from(&target); if target_path .extension() .map_or(false, |ext| ext.eq_ignore_ascii_case("py")) && target_path.exists() { - args.insert(0, target_path.as_os_str().into()); - "python".to_string() + (OsString::from("python"), vec![target_path]) } else { - target + (target.clone(), vec![]) } } else { - "python".to_string() + (OsString::from("python"), vec![]) }; let mut process = Command::new(&command); - process.args(&args); + process.args(prefix_args); + process.args(args); // Construct the `PATH` environment variable. let new_path = std::env::join_paths( @@ -250,12 +250,13 @@ pub(crate) async fn run( // TODO(zanieb): Throw a nicer error message if the command is not found let space = if args.is_empty() { "" } else { " " }; debug!( - "Running `{command}{space}{}`", + "Running `{}{space}{}`", + command.to_string_lossy(), args.iter().map(|arg| arg.to_string_lossy()).join(" ") ); let mut handle = process .spawn() - .with_context(|| format!("Failed to spawn: `{command}`"))?; + .with_context(|| format!("Failed to spawn: `{}`", command.to_string_lossy()))?; let status = handle.wait().await.context("Child process disappeared")?; // Exit based on the result of the command diff --git a/crates/uv/src/commands/tool/run.rs b/crates/uv/src/commands/tool/run.rs index af48d8aa6..70acc4e65 100644 --- a/crates/uv/src/commands/tool/run.rs +++ b/crates/uv/src/commands/tool/run.rs @@ -1,4 +1,3 @@ -use std::ffi::OsString; use std::path::PathBuf; use anyhow::{Context, Result}; @@ -13,6 +12,7 @@ use uv_requirements::RequirementsSource; use uv_toolchain::{PythonEnvironment, SystemPython, Toolchain, ToolchainRequest}; use uv_warnings::warn_user; +use crate::cli::ExternalCommand; use crate::commands::project::update_environment; use crate::commands::ExitStatus; use crate::printer::Printer; @@ -21,8 +21,7 @@ use crate::settings::ResolverInstallerSettings; /// Run a command. #[allow(clippy::too_many_arguments)] pub(crate) async fn run( - target: String, - args: Vec, + command: ExternalCommand, python: Option, from: Option, with: Vec, @@ -39,12 +38,24 @@ pub(crate) async fn run( warn_user!("`uv tool run` is experimental and may change without warning."); } - let requirements = [RequirementsSource::from_package( - from.unwrap_or_else(|| target.clone()), - )] - .into_iter() - .chain(with.into_iter().map(RequirementsSource::from_package)) - .collect::>(); + let (target, args) = command.split(); + let Some(target) = target else { + return Err(anyhow::anyhow!("No tool command provided")); + }; + + let from = if let Some(from) = from { + from + } else { + let Some(target) = target.to_str() else { + return Err(anyhow::anyhow!("Tool command could not be parsed as UTF-8 string. Use `--from` to specify the package name.")); + }; + target.to_string() + }; + + let requirements = [RequirementsSource::from_package(from)] + .into_iter() + .chain(with.into_iter().map(RequirementsSource::from_package)) + .collect::>(); // TODO(zanieb): When implementing project-level tools, discover the project and check if it has the tool. // TODO(zanieb): Determine if we should layer on top of the project environment if it is present. @@ -92,8 +103,8 @@ pub(crate) async fn run( let command = target; // Construct the command - let mut process = Command::new(&command); - process.args(&args); + let mut process = Command::new(command); + process.args(args); // Construct the `PATH` environment variable. let new_path = std::env::join_paths( @@ -133,12 +144,13 @@ pub(crate) async fn run( // TODO(zanieb): Throw a nicer error message if the command is not found let space = if args.is_empty() { "" } else { " " }; debug!( - "Running `{command}{space}{}`", + "Running `{}{space}{}`", + command.to_string_lossy(), args.iter().map(|arg| arg.to_string_lossy()).join(" ") ); let mut handle = process .spawn() - .with_context(|| format!("Failed to spawn: `{command}`"))?; + .with_context(|| format!("Failed to spawn: `{}`", command.to_string_lossy()))?; let status = handle.wait().await.context("Child process disappeared")?; // Exit based on the result of the command diff --git a/crates/uv/src/main.rs b/crates/uv/src/main.rs index ea0f53f63..8db437aba 100644 --- a/crates/uv/src/main.rs +++ b/crates/uv/src/main.rs @@ -619,8 +619,7 @@ async fn run() -> Result { commands::run( args.extras, args.dev, - args.target, - args.args, + args.command, requirements, args.python, args.package, @@ -745,8 +744,7 @@ async fn run() -> Result { let cache = cache.init()?.with_refresh(args.refresh); commands::run_tool( - args.target, - args.args, + args.command, args.python, args.from, args.with, diff --git a/crates/uv/src/settings.rs b/crates/uv/src/settings.rs index abf9449b9..d73fe1874 100644 --- a/crates/uv/src/settings.rs +++ b/crates/uv/src/settings.rs @@ -1,5 +1,4 @@ use std::env::VarError; -use std::ffi::OsString; use std::num::NonZeroUsize; use std::path::PathBuf; use std::process; @@ -26,11 +25,11 @@ use uv_settings::{ use uv_toolchain::{Prefix, PythonVersion, Target}; use crate::cli::{ - AddArgs, BuildArgs, ColorChoice, GlobalArgs, IndexArgs, InstallerArgs, LockArgs, Maybe, - PipCheckArgs, PipCompileArgs, PipFreezeArgs, PipInstallArgs, PipListArgs, PipShowArgs, - PipSyncArgs, PipUninstallArgs, RefreshArgs, RemoveArgs, ResolverArgs, ResolverInstallerArgs, - RunArgs, SyncArgs, ToolRunArgs, ToolchainFindArgs, ToolchainInstallArgs, ToolchainListArgs, - VenvArgs, + AddArgs, BuildArgs, ColorChoice, ExternalCommand, GlobalArgs, IndexArgs, InstallerArgs, + LockArgs, Maybe, PipCheckArgs, PipCompileArgs, PipFreezeArgs, PipInstallArgs, PipListArgs, + PipShowArgs, PipSyncArgs, PipUninstallArgs, RefreshArgs, RemoveArgs, ResolverArgs, + ResolverInstallerArgs, RunArgs, SyncArgs, ToolRunArgs, ToolchainFindArgs, ToolchainInstallArgs, + ToolchainListArgs, VenvArgs, }; use crate::commands::pip::operations::Modifications; use crate::commands::ListFormat; @@ -123,8 +122,7 @@ impl CacheSettings { pub(crate) struct RunSettings { pub(crate) extras: ExtrasSpecification, pub(crate) dev: bool, - pub(crate) target: Option, - pub(crate) args: Vec, + pub(crate) command: ExternalCommand, pub(crate) with: Vec, pub(crate) python: Option, pub(crate) package: Option, @@ -142,8 +140,7 @@ impl RunSettings { no_all_extras, dev, no_dev, - target, - args, + command, with, installer, build, @@ -158,8 +155,7 @@ impl RunSettings { extra.unwrap_or_default(), ), dev: flag(dev, no_dev).unwrap_or(true), - target, - args, + command, with, python, package, @@ -176,8 +172,7 @@ impl RunSettings { #[allow(clippy::struct_excessive_bools)] #[derive(Debug, Clone)] pub(crate) struct ToolRunSettings { - pub(crate) target: String, - pub(crate) args: Vec, + pub(crate) command: ExternalCommand, pub(crate) from: Option, pub(crate) with: Vec, pub(crate) python: Option, @@ -190,8 +185,7 @@ impl ToolRunSettings { #[allow(clippy::needless_pass_by_value)] pub(crate) fn resolve(args: ToolRunArgs, filesystem: Option) -> Self { let ToolRunArgs { - target, - args, + command, from, with, installer, @@ -201,8 +195,7 @@ impl ToolRunSettings { } = args; Self { - target, - args, + command, from, with, python, diff --git a/crates/uv/tests/common/mod.rs b/crates/uv/tests/common/mod.rs index 9c3822ae7..ea6eff3ca 100644 --- a/crates/uv/tests/common/mod.rs +++ b/crates/uv/tests/common/mod.rs @@ -44,7 +44,7 @@ pub const INSTA_FILTERS: &[(&str, &str)] = &[ (r"uv.exe", "uv"), // uv version display ( - r"uv \d+\.\d+\.\d+( \(.*\))?", + r"uv(-.*)? \d+\.\d+\.\d+( \(.*\))?", r"uv [VERSION] ([COMMIT] DATE)", ), // The exact message is host language dependent @@ -417,6 +417,41 @@ impl TestContext { command } + /// Create a `uv tool run` command with options shared across scenarios. + pub fn tool_run(&self) -> std::process::Command { + let mut command = self.tool_run_without_exclude_newer(); + command.arg("--exclude-newer").arg(EXCLUDE_NEWER); + command + } + + /// Create a `uv tool run` command with no `--exclude-newer` option. + /// + /// One should avoid using this in tests to the extent possible because + /// it can result in tests failing when the index state changes. Therefore, + /// if you use this, there should be some other kind of mitigation in place. + /// For example, pinning package versions. + pub fn tool_run_without_exclude_newer(&self) -> std::process::Command { + let mut command = std::process::Command::new(get_bin()); + command + .arg("tool") + .arg("run") + .arg("--cache-dir") + .arg(self.cache_dir.path()) + .env("VIRTUAL_ENV", self.venv.as_os_str()) + .env("UV_NO_WRAP", "1") + .env("UV_TOOLCHAIN_DIR", "") + .env("UV_TEST_PYTHON_PATH", &self.python_path()) + .current_dir(&self.temp_dir); + + if cfg!(all(windows, debug_assertions)) { + // TODO(konstin): Reduce stack usage in debug mode enough that the tests pass with the + // default windows stack of 1MB + command.env("UV_STACK_SIZE", (4 * 1024 * 1024).to_string()); + } + + command + } + /// Create a `uv add` command for the given requirements. pub fn add(&self, reqs: &[&str]) -> std::process::Command { let mut command = std::process::Command::new(get_bin()); diff --git a/crates/uv/tests/run.rs b/crates/uv/tests/run.rs index 454836767..746144728 100644 --- a/crates/uv/tests/run.rs +++ b/crates/uv/tests/run.rs @@ -116,3 +116,58 @@ fn run_with_python_version() -> Result<()> { Ok(()) } + +#[test] +fn run_args() -> Result<()> { + let context = TestContext::new("3.12"); + + let pyproject_toml = context.temp_dir.child("pyproject.toml"); + pyproject_toml.write_str(indoc! { r#" + [project] + name = "foo" + version = "1.0.0" + requires-python = ">=3.8" + dependencies = [] + "# + })?; + + // We treat arguments before the command as uv arguments + uv_snapshot!(context.filters(), context.run().arg("--version").arg("python"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + uv [VERSION] ([COMMIT] DATE) + + ----- stderr ----- + "###); + + // We don't treat arguments after the command as uv arguments + uv_snapshot!(context.filters(), context.run().arg("python").arg("--version"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + Python 3.12.[X] + + ----- stderr ----- + warning: `uv run` is experimental and may change without warning. + Resolved 1 package in [TIME] + Prepared 1 package in [TIME] + Installed 1 package in [TIME] + + foo==1.0.0 (from file://[TEMP_DIR]/) + "###); + + // Can use `--` to separate uv arguments from the command arguments. + uv_snapshot!(context.filters(), context.run().arg("--").arg("python").arg("--version"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + Python 3.12.[X] + + ----- stderr ----- + warning: `uv run` is experimental and may change without warning. + Resolved 1 package in [TIME] + Audited 1 package in [TIME] + "###); + + Ok(()) +} diff --git a/crates/uv/tests/tool_run.rs b/crates/uv/tests/tool_run.rs new file mode 100644 index 000000000..03e91fd4e --- /dev/null +++ b/crates/uv/tests/tool_run.rs @@ -0,0 +1,55 @@ +#![cfg(all(feature = "python", feature = "pypi"))] + +use common::{uv_snapshot, TestContext}; + +mod common; + +#[test] +fn tool_run_args() { + let context = TestContext::new("3.12"); + + // We treat arguments before the command as uv arguments + uv_snapshot!(context.filters(), context.tool_run().arg("--version").arg("pytest"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + uv [VERSION] ([COMMIT] DATE) + + ----- stderr ----- + "###); + + // We don't treat arguments after the command as uv arguments + uv_snapshot!(context.filters(), context.tool_run().arg("pytest").arg("--version"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + pytest 8.1.1 + + ----- stderr ----- + warning: `uv tool run` is experimental and may change without warning. + Resolved 4 packages in [TIME] + Prepared 4 packages in [TIME] + Installed 4 packages in [TIME] + + iniconfig==2.0.0 + + packaging==24.0 + + pluggy==1.4.0 + + pytest==8.1.1 + "###); + + // Can use `--` to separate uv arguments from the command arguments. + uv_snapshot!(context.filters(), context.tool_run().arg("--").arg("pytest").arg("--version"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + pytest 8.1.1 + + ----- stderr ----- + warning: `uv tool run` is experimental and may change without warning. + Resolved 4 packages in [TIME] + Installed 4 packages in [TIME] + + iniconfig==2.0.0 + + packaging==24.0 + + pluggy==1.4.0 + + pytest==8.1.1 + "###); +}