From 5173127fdc8e1fe9927ba67154ad3596152348a1 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Tue, 29 Aug 2023 10:01:58 +0200 Subject: [PATCH] fix: simplify quoting logic --- Cargo.lock | 1 - Cargo.toml | 1 - src/cli/run.rs | 42 +++++------------- src/cli/task.rs | 30 ++++++++----- src/task/mod.rs | 105 ++++++++++++++++++++++++++++++++++++++++++++ tests/task_tests.rs | 13 ++++-- 6 files changed, 143 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7ac4af574..68cb6c058 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2090,7 +2090,6 @@ dependencies = [ "serde_json", "serde_spanned", "serde_with", - "shlex", "spdx", "strsim", "tempfile", diff --git a/Cargo.toml b/Cargo.toml index 3ed439784..7ac767df8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -48,7 +48,6 @@ serde = "1.0.171" serde_json = "1.0.103" serde_spanned = "0.6.3" serde_with = { version = "3.1.0", features = ["indexmap"] } -shlex = "1.1.0" spdx = "0.10.2" strsim = "0.10.0" tempfile = "3.6.0" diff --git a/src/cli/run.rs b/src/cli/run.rs index ec485f5cf..a04190bc5 100644 --- a/src/cli/run.rs +++ b/src/cli/run.rs @@ -12,7 +12,7 @@ use rattler_conda_types::Platform; use crate::prefix::Prefix; use crate::progress::await_in_progress; use crate::project::environment::get_metadata_env; -use crate::task::{CmdArgs, Execute, Task}; +use crate::task::{quote_arguments, CmdArgs, Execute, Task}; use crate::{environment::get_up_to_date_prefix, Project}; use rattler_shell::{ activation::{ActivationVariables, Activator, PathModificationBehaviour}, @@ -78,10 +78,11 @@ pub fn order_tasks( .unwrap_or_else(|| { ( None, - Task::Execute(Execute { - cmd: CmdArgs::Multiple(tasks), + Execute { + cmd: CmdArgs::from(tasks), depends_on: vec![], - }), + } + .into(), Vec::new(), ) }); @@ -100,11 +101,7 @@ pub fn order_tasks( while let Some((task, additional_args)) = s1.pop_back() { // Get the dependencies of the command - let depends_on = match &task { - Task::Execute(process) => process.depends_on.as_slice(), - Task::Alias(alias) => &alias.depends_on, - _ => &[], - }; + let depends_on = task.depends_on(); // Locate the dependencies in the project and add them to the stack for dependency in depends_on.iter() { @@ -132,23 +129,12 @@ pub fn order_tasks( pub async fn create_script(task: Task, args: Vec) -> miette::Result { // Construct the script from the task - let task = match task { - Task::Execute(Execute { - cmd: CmdArgs::Single(cmd), - .. - }) - | Task::Plain(cmd) => cmd, - Task::Execute(Execute { - cmd: CmdArgs::Multiple(args), - .. - }) => quote_arguments(args), - _ => { - return Err(miette::miette!("No command given")); - } - }; + let task = task + .as_single_command() + .ok_or_else(|| miette::miette!("the task does not provide a runnable command"))?; // Append the command line arguments - let cli_args = quote_arguments(args); + let cli_args = quote_arguments(args.iter().map(|arg| arg.as_str())); let full_script = format!("{task} {cli_args}"); // Parse the shell command @@ -193,14 +179,6 @@ pub async fn execute_script_with_output( } } -fn quote_arguments(args: impl IntoIterator>) -> String { - args.into_iter() - // surround all the additional arguments in double quotes and santize any command - // substitution - .map(|a| format!("\"{}\"", a.as_ref().replace('"', "\\\""))) - .join(" ") -} - /// CLI entry point for `pixi run` /// When running the sigints are ignored and child can react to them. As it pleases. pub async fn execute(args: Args) -> miette::Result<()> { diff --git a/src/cli/task.rs b/src/cli/task.rs index 3378117c3..46817c9c6 100644 --- a/src/cli/task.rs +++ b/src/cli/task.rs @@ -1,4 +1,4 @@ -use crate::task::{Alias, CmdArgs, Execute, Task}; +use crate::task::{quote, Alias, CmdArgs, Execute, Task}; use crate::Project; use clap::Parser; use itertools::Itertools; @@ -74,19 +74,25 @@ impl From for Task { fn from(value: AddArgs) -> Self { let depends_on = value.depends_on.unwrap_or_default(); + // Convert the arguments into a single string representation + let cmd_args = if value.commands.len() == 1 { + value.commands.into_iter().next().unwrap() + } else { + // Simply concatenate all arguments + value + .commands + .into_iter() + .map(|arg| quote(&arg).into_owned()) + .join(" ") + }; + + // Depending on whether the task should have depends_on or not we create a Plain or complex + // command. if depends_on.is_empty() { - Self::Plain(if value.commands.len() == 1 { - value.commands[0].clone() - } else { - shlex::join(value.commands.iter().map(AsRef::as_ref)) - }) + Self::Plain(cmd_args) } else { Self::Execute(Execute { - cmd: CmdArgs::Single(if value.commands.len() == 1 { - value.commands[0].clone() - } else { - shlex::join(value.commands.iter().map(AsRef::as_ref)) - }), + cmd: CmdArgs::Single(cmd_args), depends_on, }) } @@ -175,7 +181,7 @@ pub fn execute(args: Args) -> miette::Result<()> { project.remove_task(name, platform)?; eprintln!( "{}Removed task {} ", - console::style(console::Emoji("❌ ", "X")).yellow(), + console::style(console::Emoji("✔ ", "+")).green(), console::style(&name).bold(), ); } diff --git a/src/task/mod.rs b/src/task/mod.rs index 53e0517ac..275c3b9f9 100644 --- a/src/task/mod.rs +++ b/src/task/mod.rs @@ -1,6 +1,7 @@ use itertools::Itertools; use serde::Deserialize; use serde_with::{formats::PreferMany, serde_as, OneOrMany}; +use std::borrow::Cow; use std::fmt::{Display, Formatter}; /// Represents different types of scripts @@ -13,6 +14,7 @@ pub enum Task { } impl Task { + /// Returns the names of the task that this task depends on pub fn depends_on(&self) -> &[String] { match self { Task::Plain(_) => &[], @@ -52,6 +54,24 @@ impl Task { Task::Alias(_) => false, } } + + /// Returns the command to execute. + pub fn as_command(&self) -> Option { + match self { + Task::Plain(cmd) => Some(CmdArgs::Single(cmd.clone())), + Task::Execute(exe) => Some(exe.cmd.clone()), + Task::Alias(_) => None, + } + } + + /// Returns the command to execute as a single string. + pub fn as_single_command(&self) -> Option> { + match self { + Task::Plain(cmd) => Some(Cow::Borrowed(cmd)), + Task::Execute(exe) => Some(exe.cmd.as_single()), + Task::Alias(_) => None, + } + } } /// A command script executes a single command from the environment @@ -68,6 +88,12 @@ pub struct Execute { pub depends_on: Vec, } +impl From for Task { + fn from(value: Execute) -> Self { + Task::Execute(value) + } +} + #[derive(Debug, Clone, Deserialize)] #[serde(untagged)] pub enum CmdArgs { @@ -75,6 +101,36 @@ pub enum CmdArgs { Multiple(Vec), } +impl From> for CmdArgs { + fn from(value: Vec) -> Self { + CmdArgs::Multiple(value) + } +} + +impl From for CmdArgs { + fn from(value: String) -> Self { + CmdArgs::Single(value) + } +} + +impl CmdArgs { + /// Returns a single string representation of the command arguments. + pub fn as_single(&self) -> Cow { + match self { + CmdArgs::Single(cmd) => Cow::Borrowed(cmd), + CmdArgs::Multiple(args) => Cow::Owned(args.iter().map(|arg| quote(arg)).join(" ")), + } + } + + /// Returns a single string representation of the command arguments. + pub fn into_single(self) -> String { + match self { + CmdArgs::Single(cmd) => cmd, + CmdArgs::Multiple(args) => args.iter().map(|arg| quote(arg)).join(" "), + } + } +} + #[derive(Debug, Clone, Deserialize)] #[serde(deny_unknown_fields)] #[serde_as] @@ -114,3 +170,52 @@ impl Display for Task { } } } + +/// Quotes a string argument if it requires quotes to be able to be properly represented in our +/// shell implementation. +pub fn quote(in_str: &str) -> Cow { + if in_str.is_empty() { + "\"\"".into() + } else if in_str + .bytes() + .any(|c| matches!(c as char, '\t' | '\r' | '\n' | ' ')) + { + let mut out: Vec = Vec::new(); + out.push(b'"'); + for c in in_str.bytes() { + match c as char { + '"' | '\\' => out.push(b'\\'), + _ => (), + } + out.push(c); + } + out.push(b'"'); + unsafe { String::from_utf8_unchecked(out) }.into() + } else { + in_str.into() + } +} + +/// Quotes multiple string arguments and joins them together to form a single string. +pub fn quote_arguments<'a>(args: impl IntoIterator) -> String { + args.into_iter().map(quote).join(" ") +} + +#[cfg(test)] +mod test { + use super::quote; + + #[test] + fn test_quote() { + assert_eq!(quote("foobar"), "foobar"); + assert_eq!(quote("foo bar"), "\"foo bar\""); + assert_eq!(quote("\""), "\""); + assert_eq!(quote("foo \" bar"), "\"foo \\\" bar\""); + assert_eq!(quote(""), "\"\""); + assert_eq!(quote("$PATH"), "$PATH"); + assert_eq!( + quote("PATH=\"$PATH;build/Debug\""), + "PATH=\"$PATH;build/Debug\"" + ); + } +} diff --git a/tests/task_tests.rs b/tests/task_tests.rs index e72571359..beecf1eb5 100644 --- a/tests/task_tests.rs +++ b/tests/task_tests.rs @@ -45,9 +45,16 @@ pub async fn add_command_types() { .unwrap(); let project = pixi.project().unwrap(); - let task = project.manifest.tasks.get("test2").unwrap(); - assert!(matches!(task, Task::Execute(cmd) if matches!(cmd.cmd, CmdArgs::Single(_)))); - assert!(matches!(task, Task::Execute(cmd) if !cmd.depends_on.is_empty())); + let task2 = project.manifest.tasks.get("test2").unwrap(); + let task = project.manifest.tasks.get("test").unwrap(); + assert!(matches!(task2, Task::Execute(cmd) if matches!(cmd.cmd, CmdArgs::Single(_)))); + assert!(matches!(task2, Task::Execute(cmd) if !cmd.depends_on.is_empty())); + + assert_eq!(task.as_single_command().as_deref(), Some("echo hello")); + assert_eq!( + task2.as_single_command().as_deref(), + Some("\"echo hello\" \"echo bonjour\"") + ); // Create an alias pixi.tasks()