From 9b8b1bad575a51370e00361946f387d1759db2ba Mon Sep 17 00:00:00 2001 From: JT Date: Thu, 13 May 2021 15:03:49 +1200 Subject: [PATCH] Don't insert PATH variable on Windows (#3422) * Don't insert PATH variable on Windows * Simplify fix * Just centralize the var * Add a message about why we have to workaround the issue --- crates/nu-cli/src/completion/command.rs | 3 ++- .../src/commands/classified/external.rs | 3 ++- crates/nu-data/src/config/nuconfig.rs | 3 ++- crates/nu-engine/Cargo.toml | 1 + crates/nu-engine/src/evaluate/variables.rs | 14 +++++++++++ crates/nu-engine/src/evaluation_context.rs | 23 +++++-------------- crates/nu-test-support/src/lib.rs | 7 +++++- crates/nu-test-support/src/macros.rs | 6 +++-- .../src/playground/nu_process.rs | 2 +- tests/shell/environment/in_sync.rs | 5 +++- 10 files changed, 42 insertions(+), 25 deletions(-) diff --git a/crates/nu-cli/src/completion/command.rs b/crates/nu-cli/src/completion/command.rs index ce0efd7ead..242508407d 100644 --- a/crates/nu-cli/src/completion/command.rs +++ b/crates/nu-cli/src/completion/command.rs @@ -6,6 +6,7 @@ use indexmap::set::IndexSet; use super::matchers::Matcher; use crate::completion::{Completer, CompletionContext, Suggestion}; use nu_engine::EvaluationContext; +use nu_test_support::NATIVE_PATH_ENV_VAR; pub struct CommandCompleter; @@ -121,7 +122,7 @@ fn is_executable(path: &Path) -> bool { // TODO cache these, but watch for changes to PATH fn find_path_executables() -> Option> { - let path_var = std::env::var_os("PATH")?; + let path_var = std::env::var_os(NATIVE_PATH_ENV_VAR)?; let paths: Vec<_> = std::env::split_paths(&path_var).collect(); let mut executables: IndexSet = IndexSet::new(); diff --git a/crates/nu-command/src/commands/classified/external.rs b/crates/nu-command/src/commands/classified/external.rs index b0106d1bd8..79a7b43401 100644 --- a/crates/nu-command/src/commands/classified/external.rs +++ b/crates/nu-command/src/commands/classified/external.rs @@ -1,6 +1,7 @@ use crate::prelude::*; use nu_engine::{evaluate_baseline_expr, BufCodecReader}; use nu_engine::{MaybeTextCodec, StringOrBinary}; +use nu_test_support::NATIVE_PATH_ENV_VAR; use parking_lot::Mutex; use std::io::Write; @@ -518,7 +519,7 @@ fn remove_quotes(argument: &str) -> Option<&str> { fn shell_os_paths() -> Vec { let mut original_paths = vec![]; - if let Some(paths) = std::env::var_os("PATH") { + if let Some(paths) = std::env::var_os(NATIVE_PATH_ENV_VAR) { original_paths = std::env::split_paths(&paths).collect::>(); } diff --git a/crates/nu-data/src/config/nuconfig.rs b/crates/nu-data/src/config/nuconfig.rs index 22bf968ac5..7cecb8e911 100644 --- a/crates/nu-data/src/config/nuconfig.rs +++ b/crates/nu-data/src/config/nuconfig.rs @@ -3,6 +3,7 @@ use indexmap::IndexMap; use nu_errors::ShellError; use nu_protocol::Value; use nu_source::Tag; +use nu_test_support::NATIVE_PATH_ENV_VAR; use std::{fmt::Debug, path::PathBuf}; use super::write; @@ -147,7 +148,7 @@ impl NuConfig { pub fn path(&self) -> Result>, ShellError> { let vars = &self.vars; - if let Some(path) = vars.get("path").or_else(|| vars.get("PATH")) { + if let Some(path) = vars.get("path").or_else(|| vars.get(NATIVE_PATH_ENV_VAR)) { path .table_entries() .map(|p| { diff --git a/crates/nu-engine/Cargo.toml b/crates/nu-engine/Cargo.toml index 05d6d17a8b..9cb39fa832 100644 --- a/crates/nu-engine/Cargo.toml +++ b/crates/nu-engine/Cargo.toml @@ -16,6 +16,7 @@ nu-source = { version = "0.31.1", path = "../nu-source" } nu-stream = { version = "0.31.1", path = "../nu-stream" } nu-value-ext = { version = "0.31.1", path = "../nu-value-ext" } nu-ansi-term = { version = "0.31.1", path = "../nu-ansi-term" } +nu-test-support = { version = "0.31.1", path = "../nu-test-support" } trash = { version = "1.3.0", optional = true } which = { version = "4.0.2", optional = true } diff --git a/crates/nu-engine/src/evaluate/variables.rs b/crates/nu-engine/src/evaluate/variables.rs index df44b6606c..30bc6e1b7a 100644 --- a/crates/nu-engine/src/evaluate/variables.rs +++ b/crates/nu-engine/src/evaluate/variables.rs @@ -16,6 +16,7 @@ pub fn nu( let mut nu_dict = TaggedDictBuilder::new(&tag); let mut dict = TaggedDictBuilder::new(&tag); + for v in env.iter() { if v.0 != "PATH" && v.0 != "Path" { dict.insert_untagged(v.0, UntaggedValue::string(v.1)); @@ -49,6 +50,19 @@ pub fn nu( } } + // A note about environment variables: + // + // Environment variables in Unix platforms are case-sensitive. On Windows, case-sensitivity is context-dependent. + // In DOS, running `SET` will show you the list of environment variables, but their names will be all upper-cased. + // In PowerShell, running `Get-ChildItem Env:` will show you a list of environment variables, and they will match + // the case in the environment variable section of the user configuration + // + // Rust currently returns the DOS-style, all-uppercase environment variables on Windows (as of 1.52) when running + // std::env::vars(), rather than the case-sensitive Environment.GetEnvironmentVariables() of .NET that PowerShell + // uses. + // + // For now, we work around the discrepency as best we can by merging the two into what is shown to the user as the + // 'path' column of `$nu` let mut table = vec![]; for v in env.iter() { if v.0 == "PATH" || v.0 == "Path" { diff --git a/crates/nu-engine/src/evaluation_context.rs b/crates/nu-engine/src/evaluation_context.rs index b00d6b1c77..c0a6ed5057 100644 --- a/crates/nu-engine/src/evaluation_context.rs +++ b/crates/nu-engine/src/evaluation_context.rs @@ -11,6 +11,7 @@ use nu_errors::ShellError; use nu_protocol::{hir, ConfigPath}; use nu_source::{Span, Tag}; use nu_stream::InputStream; +use nu_test_support::NATIVE_PATH_ENV_VAR; use parking_lot::Mutex; use std::sync::atomic::AtomicBool; use std::{path::Path, sync::Arc}; @@ -112,7 +113,7 @@ impl EvaluationContext { let env_vars = self.scope.get_env_vars(); for (var, val) in env_vars { - if var == "PATH" || var == "Path" || var == "path" { + if var == NATIVE_PATH_ENV_VAR { std::env::set_var(var, val); break; } @@ -174,13 +175,7 @@ impl EvaluationContext { let joined_paths = cfg_paths .map(|mut cfg_paths| { //existing paths are prepended to path - let env_paths = if let Some(env_paths) = self.scope.get_env("PATH") { - Some(env_paths) - } else if let Some(env_paths) = self.scope.get_env("Path") { - Some(env_paths) - } else { - self.scope.get_env("path") - }; + let env_paths = self.scope.get_env(NATIVE_PATH_ENV_VAR); if let Some(env_paths) = env_paths { let mut env_paths = std::env::split_paths(&env_paths).collect::>(); @@ -210,7 +205,7 @@ impl EvaluationContext { self.scope.enter_scope_with_tag(tag); self.scope.add_env(cfg.env_map()); if let Some(path) = joined_paths { - self.scope.add_env_var("PATH", path); + self.scope.add_env_var(NATIVE_PATH_ENV_VAR, path); } self.scope.set_exit_scripts(exit_scripts); @@ -243,13 +238,7 @@ impl EvaluationContext { let joined_paths = cfg_paths .map(|mut cfg_paths| { //existing paths are prepended to path - let env_paths = if let Some(env_paths) = self.scope.get_env("PATH") { - Some(env_paths) - } else if let Some(env_paths) = self.scope.get_env("Path") { - Some(env_paths) - } else { - self.scope.get_env("path") - }; + let env_paths = self.scope.get_env(NATIVE_PATH_ENV_VAR); if let Some(env_paths) = env_paths { let mut env_paths = std::env::split_paths(&env_paths).collect::>(); @@ -279,7 +268,7 @@ impl EvaluationContext { frame.env = cfg.env_map(); if let Some(path) = joined_paths { - frame.env.insert("PATH".to_string(), path); + frame.env.insert(NATIVE_PATH_ENV_VAR.to_string(), path); } frame.exitscripts = exit_scripts; diff --git a/crates/nu-test-support/src/lib.rs b/crates/nu-test-support/src/lib.rs index 611ad4d80d..3925b2d095 100644 --- a/crates/nu-test-support/src/lib.rs +++ b/crates/nu-test-support/src/lib.rs @@ -9,6 +9,11 @@ pub struct Outcome { pub err: String, } +#[cfg(windows)] +pub const NATIVE_PATH_ENV_VAR: &str = "Path"; +#[cfg(not(windows))] +pub const NATIVE_PATH_ENV_VAR: &str = "PATH"; + impl Outcome { pub fn new(out: String, err: String) -> Outcome { Outcome { out, err } @@ -29,7 +34,7 @@ pub fn pipeline(commands: &str) -> String { pub fn shell_os_paths() -> Vec { let mut original_paths = vec![]; - if let Some(paths) = std::env::var_os("PATH") { + if let Some(paths) = std::env::var_os(NATIVE_PATH_ENV_VAR) { original_paths = std::env::split_paths(&paths).collect::>(); } diff --git a/crates/nu-test-support/src/macros.rs b/crates/nu-test-support/src/macros.rs index 759a710f94..d97f984abe 100644 --- a/crates/nu-test-support/src/macros.rs +++ b/crates/nu-test-support/src/macros.rs @@ -18,6 +18,7 @@ macro_rules! nu { pub use std::error::Error; pub use std::io::prelude::*; pub use std::process::{Command, Stdio}; + pub use $crate::NATIVE_PATH_ENV_VAR; let commands = &*format!( " @@ -46,7 +47,7 @@ macro_rules! nu { }; let mut process = match Command::new($crate::fs::executable_path()) - .env("PATH", paths_joined) + .env(NATIVE_PATH_ENV_VAR, paths_joined) .arg("--skip-plugins") .arg("--no-history") .arg("--config-file") @@ -98,6 +99,7 @@ macro_rules! nu_with_plugins { pub use std::error::Error; pub use std::io::prelude::*; pub use std::process::{Command, Stdio}; + pub use crate::NATIVE_PATH_ENV_VAR; let commands = &*format!( " @@ -126,7 +128,7 @@ macro_rules! nu_with_plugins { }; let mut process = match Command::new($crate::fs::executable_path()) - .env("PATH", paths_joined) + .env(NATIVE_PATH_ENV_VAR, paths_joined) .stdout(Stdio::piped()) .stdin(Stdio::piped()) .stderr(Stdio::piped()) diff --git a/crates/nu-test-support/src/playground/nu_process.rs b/crates/nu-test-support/src/playground/nu_process.rs index 4504562161..30e18141e3 100644 --- a/crates/nu-test-support/src/playground/nu_process.rs +++ b/crates/nu-test-support/src/playground/nu_process.rs @@ -99,7 +99,7 @@ impl NuProcess { Err(_) => panic!("Couldn't join paths for PATH var."), }; - command.env("PATH", paths_joined); + command.env(crate::NATIVE_PATH_ENV_VAR, paths_joined); for env_var in &self.environment_vars { command.env(&env_var.name, &env_var.value); diff --git a/tests/shell/environment/in_sync.rs b/tests/shell/environment/in_sync.rs index 98503fcb88..2f2138ab72 100644 --- a/tests/shell/environment/in_sync.rs +++ b/tests/shell/environment/in_sync.rs @@ -103,7 +103,10 @@ fn inherited_environment_path_values_not_present_in_configuration_should_pick_up "#, )]) .with_config(&file) - .with_env("PATH", &PathBuf::from("/path/to/be/added").display_path()); + .with_env( + nu_test_support::NATIVE_PATH_ENV_VAR, + &PathBuf::from("/path/to/be/added").display_path(), + ); assert_that!( nu.pipeline("echo $nu.path | str collect '-'"),