Skip to content

Commit

Permalink
fix: simplify quoting logic (#313)
Browse files Browse the repository at this point in the history
baszalmstra authored Sep 11, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent 40b479e commit 26c2af1
Showing 6 changed files with 143 additions and 49 deletions.
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
@@ -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"
42 changes: 10 additions & 32 deletions src/cli/run.rs
Original file line number Diff line number Diff line change
@@ -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<String>) -> miette::Result<SequentialList> {
// 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<Item = impl AsRef<str>>) -> 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<()> {
30 changes: 18 additions & 12 deletions src/cli/task.rs
Original file line number Diff line number Diff line change
@@ -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<AddArgs> 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(),
);
}
105 changes: 105 additions & 0 deletions src/task/mod.rs
Original file line number Diff line number Diff line change
@@ -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<CmdArgs> {
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<Cow<str>> {
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,13 +88,49 @@ pub struct Execute {
pub depends_on: Vec<String>,
}

impl From<Execute> for Task {
fn from(value: Execute) -> Self {
Task::Execute(value)
}
}

#[derive(Debug, Clone, Deserialize)]
#[serde(untagged)]
pub enum CmdArgs {
Single(String),
Multiple(Vec<String>),
}

impl From<Vec<String>> for CmdArgs {
fn from(value: Vec<String>) -> Self {
CmdArgs::Multiple(value)
}
}

impl From<String> 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<str> {
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<str> {
if in_str.is_empty() {
"\"\"".into()
} else if in_str
.bytes()
.any(|c| matches!(c as char, '\t' | '\r' | '\n' | ' '))
{
let mut out: Vec<u8> = 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<Item = &'a str>) -> 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\""
);
}
}
13 changes: 10 additions & 3 deletions tests/task_tests.rs
Original file line number Diff line number Diff line change
@@ -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()

0 comments on commit 26c2af1

Please sign in to comment.