From 9e1e2a432067ab80bbfce504fe4eb5a07a73f43a Mon Sep 17 00:00:00 2001 From: Eugeen Sablin Date: Tue, 12 Sep 2023 14:03:41 +0200 Subject: [PATCH] provide env to commands and try to start provided path (#10302) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes #8551 # Description Use `open::commands` function to get list of command available for starting given path. run commands directly, providing environment, until one of them is successful. example of output if start was not successful: ``` ~\code\nushell> start ..\nustart\a.myext 09/12/2023 01:37:55 PM Error: nu::shell::external_command × External command failed ╭─[entry #1:1:1] 1 │ start ..\nustart\a.myext · ─────────┬──────── · ╰── No command found to start with this path ╰──── help: Try different path or install appropriate command Command `cmd /c start "" "..\nustart\a.myext"` failed with exit code: 1 ``` # User-Facing Changes `start` command now provides environment to the external command. This is how it worked in `nu 0.72`, see linked issue. # Tests + Formatting `start` command didn't have any tests and this PR does not add any. Integration-level tests will require setup specific to OS and potentially change global environment on testing machine. For unit-level test it is possible to test `try_commands` function. But is still runs external commands, and robust test will require apriori knowledge which commands are necessary successful to run and which are not. --- crates/nu-command/src/filesystem/start.rs | 78 +++++++++++++++++++++-- 1 file changed, 72 insertions(+), 6 deletions(-) diff --git a/crates/nu-command/src/filesystem/start.rs b/crates/nu-command/src/filesystem/start.rs index 790315a94a..5e96ffa568 100644 --- a/crates/nu-command/src/filesystem/start.rs +++ b/crates/nu-command/src/filesystem/start.rs @@ -1,11 +1,15 @@ +use itertools::Itertools; +use nu_engine::env_to_strings; use nu_engine::CallExt; use nu_path::canonicalize_with; use nu_protocol::ast::Call; use nu_protocol::engine::{Command, EngineState, Stack}; use nu_protocol::{ - Category, Example, PipelineData, ShellError, Signature, Spanned, SyntaxShape, Type, + Category, Example, PipelineData, ShellError, Signature, Span, Spanned, SyntaxShape, Type, }; +use std::ffi::{OsStr, OsString}; use std::path::Path; +use std::process::Stdio; #[derive(Clone)] pub struct Start; @@ -46,7 +50,7 @@ impl Command for Start { // only check if file exists in current current directory let file_path = Path::new(path_no_whitespace); if file_path.exists() { - open::that(path_no_whitespace)?; + open_path(path_no_whitespace, engine_state, stack, path.span)?; } else if file_path.starts_with("https://") || file_path.starts_with("http://") { let url = url::Url::parse(&path.item).map_err(|_| { ShellError::GenericError( @@ -57,13 +61,13 @@ impl Command for Start { Vec::new(), ) })?; - open::that(url.as_str())?; + open_path(url.as_str(), engine_state, stack, path.span)?; } else { // try to distinguish between file not found and opening url without prefix - if let Ok(path) = + if let Ok(canon_path) = canonicalize_with(path_no_whitespace, std::env::current_dir()?.as_path()) { - open::that(path)?; + open_path(canon_path, engine_state, stack, path.span)?; } else { // open crate does not allow opening URL without prefix let path_with_prefix = Path::new("https://").join(&path.item); @@ -83,7 +87,7 @@ impl Command for Start { let ext = Path::new(&domain).extension().and_then(|s| s.to_str()); if let Some(url_ext) = ext { if common_domains.contains(&url_ext) { - open::that(url.as_str())?; + open_path(url.as_str(), engine_state, stack, path.span)?; } } } @@ -130,3 +134,65 @@ impl Command for Start { ] } } + +fn open_path( + path: impl AsRef, + engine_state: &EngineState, + stack: &Stack, + span: Span, +) -> Result<(), ShellError> { + try_commands(open::commands(path), engine_state, stack, span) +} + +fn try_commands( + commands: Vec, + engine_state: &EngineState, + stack: &Stack, + span: Span, +) -> Result<(), ShellError> { + let env_vars_str = env_to_strings(engine_state, stack)?; + commands + .into_iter() + .map(|mut cmd| { + let status = cmd + .envs(&env_vars_str) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status(); + match status { + Ok(status) if status.success() => Ok(()), + Ok(status) => Err(format!( + "\nCommand `{}` failed with {}", + format_command(&cmd), + status + )), + Err(err) => Err(format!( + "\nCommand `{}` failed with {}", + format_command(&cmd), + err + )), + } + }) + .take_while_inclusive(|result| result.is_err()) + .fold(Err("".to_string()), |combined_result, next_result| { + combined_result.or_else(|combined_message| { + next_result.map_err(|next_message| combined_message + &next_message) + }) + }) + .map_err(|message| ShellError::ExternalCommand { + label: "No command found to start with this path".to_string(), + help: "Try different path or install appropriate command\n".to_string() + &message, + span, + }) +} + +fn format_command(command: &std::process::Command) -> String { + let parts_iter = std::iter::repeat(command.get_program()) + .take(1) + .chain(command.get_args()); + Itertools::intersperse(parts_iter, " ".as_ref()) + .collect::() + .to_string_lossy() + .into_owned() +}