From 08e7d0dfb6453374b5b792a7ba4ae90556c4f687 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20N=2E=20Robalino?= Date: Wed, 17 Feb 2021 21:56:14 -0500 Subject: [PATCH] Keep the environment properly set. (#3072) * Revert "fix prompts on startup (#3056)" This reverts commit b202951c1dcf2e37ac3d81e02535dc6cb3623eca. * Ensure environment variables synced with global internal Nu Scope. --- crates/nu-cli/src/env/environment_syncer.rs | 262 ++++++++++++++++---- crates/nu-command/src/commands/with_env.rs | 1 + crates/nu-command/src/script.rs | 1 + crates/nu-data/src/config/nuconfig.rs | 4 + crates/nu-engine/src/evaluate/scope.rs | 9 +- crates/nu-engine/src/evaluate/variables.rs | 9 +- 6 files changed, 233 insertions(+), 53 deletions(-) diff --git a/crates/nu-cli/src/env/environment_syncer.rs b/crates/nu-cli/src/env/environment_syncer.rs index 97e656ec1..c51cb998d 100644 --- a/crates/nu-cli/src/env/environment_syncer.rs +++ b/crates/nu-cli/src/env/environment_syncer.rs @@ -74,24 +74,34 @@ impl EnvironmentSyncer { } pub fn sync_env_vars(&mut self, ctx: &mut EvaluationContext) { - let nu_env_vars = ctx.scope.get_env_vars(); + let mut environment = self.env.lock(); - let environment = self.env.lock(); - if let Some(variables) = environment.env() { - for var in variables.row_entries() { - if let Ok(string) = var.1.as_string() { - if var.0 != "path" && var.0 != "PATH" && !nu_env_vars.contains_key(var.0) { - ctx.scope.add_env_var(var.0, string); - } + if environment.env().is_some() { + for (name, value) in ctx.with_host(|host| host.vars()) { + if name != "path" && name != "PATH" { + // account for new env vars present in the current session + // that aren't loaded from config. + environment.add_env(&name, &value); + + // clear the env var from the session + // we are about to replace them + ctx.with_host(|host| host.env_rm(std::ffi::OsString::from(name))); } } - } - let nu_env_vars = ctx.scope.get_env_vars(); + if let Some(variables) = environment.env() { + for var in variables.row_entries() { + if let Ok(string) = var.1.as_string() { + ctx.with_host(|host| { + host.env_set( + std::ffi::OsString::from(var.0), + std::ffi::OsString::from(&string), + ) + }); - for (name, value) in ctx.with_host(|host| host.vars()) { - if name != "path" && name != "PATH" && !nu_env_vars.contains_key(&name) { - ctx.scope.add_env_var(name, value); + ctx.scope.add_env_var_to_base(var.0, string); + } + } } } } @@ -99,25 +109,36 @@ impl EnvironmentSyncer { pub fn sync_path_vars(&mut self, ctx: &mut EvaluationContext) { let mut environment = self.env.lock(); - let native_paths = ctx.with_host(|host| host.env_get(std::ffi::OsString::from("PATH"))); + if environment.path().is_some() { + let native_paths = ctx.with_host(|host| host.env_get(std::ffi::OsString::from("PATH"))); - if let Some(native_paths) = native_paths { - for path in std::env::split_paths(&native_paths) { - environment.add_path(path.as_os_str().to_os_string()); + if let Some(native_paths) = native_paths { + environment.add_path(native_paths); + + ctx.with_host(|host| { + host.env_rm(std::ffi::OsString::from("PATH")); + }); } - } - if let Some(new_paths) = environment.path() { - let prepared = std::env::join_paths( - new_paths - .table_entries() - .map(|p| p.as_string()) - .filter_map(Result::ok), - ); + if let Some(new_paths) = environment.path() { + let prepared = std::env::join_paths( + new_paths + .table_entries() + .map(|p| p.as_string()) + .filter_map(Result::ok), + ); - if let Ok(paths_ready) = prepared { - ctx.scope - .add_env_var("PATH", paths_ready.to_string_lossy().to_string()); + if let Ok(paths_ready) = prepared { + ctx.with_host(|host| { + host.env_set( + std::ffi::OsString::from("PATH"), + std::ffi::OsString::from(&paths_ready), + ); + }); + + ctx.scope + .add_env_var_to_base("PATH", paths_ready.to_string_lossy().to_string()); + } } } } @@ -143,6 +164,7 @@ mod tests { use indexmap::IndexMap; use nu_data::config::tests::FakeConfig; use nu_engine::basic_evaluation_context; + use nu_engine::Env; use nu_errors::ShellError; use nu_test_support::fs::Stub::FileWithContent; use nu_test_support::playground::Playground; @@ -200,6 +222,25 @@ mod tests { actual.load_environment(); actual.sync_env_vars(&mut ctx); + { + let environment = actual.env.lock(); + let mut vars = IndexMap::new(); + environment + .env() + .expect("No variables in the environment.") + .row_entries() + .for_each(|(name, value)| { + vars.insert( + name.to_string(), + value.as_string().expect("Couldn't convert to string"), + ); + }); + + for k in expected.keys() { + assert!(vars.contains_key(k)); + } + } + assert!(!actual.did_config_change()); // Replacing the newer configuration file to the existing one. @@ -213,12 +254,26 @@ mod tests { actual.reload(); actual.sync_env_vars(&mut ctx); - let env_vars = ctx.scope.get_env_vars(); - let result = env_vars.get("SHELL").unwrap(); - assert_eq!(result, "/usr/bin/you_already_made_the_nu_choice"); + expected.insert("USER".to_string(), "NUNO".to_string()); - let result = env_vars.get("USER").unwrap(); - assert_eq!(result, "NUNO"); + { + let environment = actual.env.lock(); + let mut vars = IndexMap::new(); + environment + .env() + .expect("No variables in the environment.") + .row_entries() + .for_each(|(name, value)| { + vars.insert( + name.to_string(), + value.as_string().expect("Couldn't convert to string"), + ); + }); + + for k in expected.keys() { + assert!(vars.contains_key(k)); + } + } }); Ok(()) @@ -278,12 +333,48 @@ mod tests { // Nu sees the missing "USER" variable and accounts for it. actual.sync_env_vars(&mut ctx); - let env_vars = ctx.scope.get_env_vars(); - let result = env_vars.get("SHELL").unwrap(); - assert_eq!(result, "/usr/bin/you_already_made_the_nu_choice"); + // Confirms session environment variables are replaced from Nu configuration file + // including the newer one accounted for. + ctx.with_host(|test_host| { + let var_user = test_host + .env_get(std::ffi::OsString::from("USER")) + .expect("Couldn't get USER var from host.") + .into_string() + .expect("Couldn't convert to string."); - let result = env_vars.get("USER").unwrap(); - assert_eq!(result, "NUNO"); + let var_shell = test_host + .env_get(std::ffi::OsString::from("SHELL")) + .expect("Couldn't get SHELL var from host.") + .into_string() + .expect("Couldn't convert to string."); + + let mut found = IndexMap::new(); + found.insert("SHELL".to_string(), var_shell); + found.insert("USER".to_string(), var_user); + + for k in found.keys() { + assert!(expected.contains_key(k)); + } + }); + + // Now confirm in-memory environment variables synced appropriately + // including the newer one accounted for. + let environment = actual.env.lock(); + + let mut vars = IndexMap::new(); + environment + .env() + .expect("No variables in the environment.") + .row_entries() + .for_each(|(name, value)| { + vars.insert( + name.to_string(), + value.as_string().expect("Couldn't convert to string"), + ); + }); + for k in expected.keys() { + assert!(vars.contains_key(k)); + } }); Ok(()) } @@ -293,6 +384,12 @@ mod tests { let mut ctx = basic_evaluation_context()?; ctx.host = Arc::new(Mutex::new(Box::new(nu_engine::FakeHost::new()))); + let mut expected = IndexMap::new(); + expected.insert( + "SHELL".to_string(), + "/usr/bin/you_already_made_the_nu_choice".to_string(), + ); + Playground::setup("syncs_env_test_2", |dirs, sandbox| { sandbox.with_files(vec![FileWithContent( "configuration.toml", @@ -321,10 +418,37 @@ mod tests { actual.load_environment(); actual.sync_env_vars(&mut ctx); - let env_vars = ctx.scope.get_env_vars(); - let result = env_vars.get("SHELL").unwrap(); + ctx.with_host(|test_host| { + let var_shell = test_host + .env_get(std::ffi::OsString::from("SHELL")) + .expect("Couldn't get SHELL var from host.") + .into_string() + .expect("Couldn't convert to string."); - assert_eq!(result, "/usr/bin/you_already_made_the_nu_choice"); + let mut found = IndexMap::new(); + found.insert("SHELL".to_string(), var_shell); + + for k in found.keys() { + assert!(expected.contains_key(k)); + } + }); + + let environment = actual.env.lock(); + + let mut vars = IndexMap::new(); + environment + .env() + .expect("No variables in the environment.") + .row_entries() + .for_each(|(name, value)| { + vars.insert( + name.to_string(), + value.as_string().expect("couldn't convert to string"), + ); + }); + for k in expected.keys() { + assert!(vars.contains_key(k)); + } }); Ok(()) @@ -386,10 +510,32 @@ mod tests { // Nu sees the missing "/path/to/be/added" and accounts for it. actual.sync_path_vars(&mut ctx); - let env_vars = ctx.scope.get_env_vars(); - let paths = env_vars.get("PATH").unwrap(); + ctx.with_host(|test_host| { + let actual = test_host + .env_get(std::ffi::OsString::from("PATH")) + .expect("Couldn't get PATH var from host.") + .into_string() + .expect("Couldn't convert to string."); - assert_eq!(paths, &expected); + assert_eq!(actual, expected); + }); + + let environment = actual.env.lock(); + + let paths = std::env::join_paths( + &environment + .path() + .expect("No path variable in the environment.") + .table_entries() + .map(|value| value.as_string().expect("Couldn't convert to string")) + .map(PathBuf::from) + .collect::>(), + ) + .expect("Couldn't join paths.") + .into_string() + .expect("Couldn't convert to string."); + + assert_eq!(paths, expected); }); Ok(()) @@ -438,10 +584,32 @@ mod tests { actual.load_environment(); actual.sync_path_vars(&mut ctx); - let env_vars = ctx.scope.get_env_vars(); - let paths = env_vars.get("PATH").unwrap(); + ctx.with_host(|test_host| { + let actual = test_host + .env_get(std::ffi::OsString::from("PATH")) + .expect("Couldn't get PATH var from host.") + .into_string() + .expect("Couldn't convert to string."); - assert_eq!(paths, &expected); + assert_eq!(actual, expected); + }); + + let environment = actual.env.lock(); + + let paths = std::env::join_paths( + &environment + .path() + .expect("No path variable in the environment.") + .table_entries() + .map(|value| value.as_string().expect("Couldn't convert to string")) + .map(PathBuf::from) + .collect::>(), + ) + .expect("Couldn't join paths.") + .into_string() + .expect("Couldn't convert to string."); + + assert_eq!(paths, expected); }); Ok(()) diff --git a/crates/nu-command/src/commands/with_env.rs b/crates/nu-command/src/commands/with_env.rs index c1db4fdd1..044b0266f 100644 --- a/crates/nu-command/src/commands/with_env.rs +++ b/crates/nu-command/src/commands/with_env.rs @@ -80,6 +80,7 @@ async fn with_env(raw_args: CommandArgs) -> Result { ) = raw_args.process().await?; block.block.set_redirect(redirection); + let mut env = IndexMap::new(); match &variable.value { diff --git a/crates/nu-command/src/script.rs b/crates/nu-command/src/script.rs index a768f1cee..fb416f675 100644 --- a/crates/nu-command/src/script.rs +++ b/crates/nu-command/src/script.rs @@ -186,6 +186,7 @@ pub async fn process_script( ctx.scope.add_env_to_base(env); let result = run_block(&block, ctx, input_stream).await; + match result { Ok(input) => { // Running a pipeline gives us back a stream that we can then diff --git a/crates/nu-data/src/config/nuconfig.rs b/crates/nu-data/src/config/nuconfig.rs index 44fb48ffb..e37669a58 100644 --- a/crates/nu-data/src/config/nuconfig.rs +++ b/crates/nu-data/src/config/nuconfig.rs @@ -127,6 +127,10 @@ impl NuConfig { return Some(env_vars.clone()); } + if let Some(env_vars) = vars.get("PATH") { + return Some(env_vars.clone()); + } + None } } diff --git a/crates/nu-engine/src/evaluate/scope.rs b/crates/nu-engine/src/evaluate/scope.rs index e02958085..c5a9fd380 100644 --- a/crates/nu-engine/src/evaluate/scope.rs +++ b/crates/nu-engine/src/evaluate/scope.rs @@ -176,8 +176,7 @@ impl Scope { pub fn add_env_var(&self, name: impl Into, value: String) { if let Some(frame) = self.frames.lock().last_mut() { - let name = name.into(); - frame.env.insert(name, value); + frame.env.insert(name.into(), value); } } @@ -192,6 +191,12 @@ impl Scope { frame.env.extend(env_vars) } } + + pub fn add_env_var_to_base(&self, name: impl Into, value: String) { + if let Some(frame) = self.frames.lock().first_mut() { + frame.env.insert(name.into(), value); + } + } } impl ParserScope for Scope { diff --git a/crates/nu-engine/src/evaluate/variables.rs b/crates/nu-engine/src/evaluate/variables.rs index e9f0a1170..2df871d6e 100644 --- a/crates/nu-engine/src/evaluate/variables.rs +++ b/crates/nu-engine/src/evaluate/variables.rs @@ -21,10 +21,11 @@ pub fn nu(env: &IndexMap, tag: impl Into) -> Result