From b202951c1dcf2e37ac3d81e02535dc6cb3623eca Mon Sep 17 00:00:00 2001 From: Jonathan Turner Date: Mon, 15 Feb 2021 20:14:16 +1300 Subject: [PATCH] fix prompts on startup (#3056) * fix prompts on startup * Try again --- crates/nu-cli/src/cli.rs | 6 +- crates/nu-cli/src/env/environment_syncer.rs | 258 ++++---------------- crates/nu-command/src/commands/with_env.rs | 10 +- crates/nu-command/src/script.rs | 1 - crates/nu-engine/src/evaluate/scope.rs | 3 +- tests/shell/pipeline/commands/internal.rs | 45 ++++ 6 files changed, 109 insertions(+), 214 deletions(-) diff --git a/crates/nu-cli/src/cli.rs b/crates/nu-cli/src/cli.rs index 123ab7d515..381a00a1fa 100644 --- a/crates/nu-cli/src/cli.rs +++ b/crates/nu-cli/src/cli.rs @@ -29,7 +29,7 @@ use rustyline::{self, error::ReadlineError}; use crate::EnvironmentSyncer; use nu_errors::ShellError; use nu_parser::ParserScope; -use nu_protocol::{UntaggedValue, Value}; +use nu_protocol::{hir::ExternalRedirection, UntaggedValue, Value}; use std::error::Error; use std::iter::Iterator; @@ -158,7 +158,9 @@ pub async fn cli(mut context: EvaluationContext) -> Result<(), Box> { let prompt_line = prompt.as_string()?; context.scope.enter_scope(); - let (prompt_block, err) = nu_parser::parse(&prompt_line, 0, &context.scope); + let (mut prompt_block, err) = nu_parser::parse(&prompt_line, 0, &context.scope); + + prompt_block.set_redirect(ExternalRedirection::Stdout); if err.is_some() { context.scope.exit_scope(); diff --git a/crates/nu-cli/src/env/environment_syncer.rs b/crates/nu-cli/src/env/environment_syncer.rs index ba0909792e..97e656ec1f 100644 --- a/crates/nu-cli/src/env/environment_syncer.rs +++ b/crates/nu-cli/src/env/environment_syncer.rs @@ -74,63 +74,50 @@ impl EnvironmentSyncer { } pub fn sync_env_vars(&mut self, ctx: &mut EvaluationContext) { - let mut environment = self.env.lock(); + let nu_env_vars = ctx.scope.get_env_vars(); - 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))); - } - } - - 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), - ) - }); + 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); } } } } + + let nu_env_vars = ctx.scope.get_env_vars(); + + 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); + } + } } pub fn sync_path_vars(&mut self, ctx: &mut EvaluationContext) { let mut environment = self.env.lock(); - if environment.path().is_some() { - let native_paths = ctx.with_host(|host| host.env_get(std::ffi::OsString::from("PATH"))); + let native_paths = ctx.with_host(|host| host.env_get(std::ffi::OsString::from("PATH"))); - 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(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(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.with_host(|host| { - host.env_set(std::ffi::OsString::from("PATH"), paths_ready); - }); - } + if let Ok(paths_ready) = prepared { + ctx.scope + .add_env_var("PATH", paths_ready.to_string_lossy().to_string()); } } } @@ -156,7 +143,6 @@ 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; @@ -214,25 +200,6 @@ 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. @@ -246,26 +213,12 @@ mod tests { actual.reload(); actual.sync_env_vars(&mut ctx); - expected.insert("USER".to_string(), "NUNO".to_string()); + 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"); - { - 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)); - } - } + let result = env_vars.get("USER").unwrap(); + assert_eq!(result, "NUNO"); }); Ok(()) @@ -325,48 +278,12 @@ mod tests { // Nu sees the missing "USER" variable and accounts for it. actual.sync_env_vars(&mut ctx); - // 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 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"); - 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)); - } + let result = env_vars.get("USER").unwrap(); + assert_eq!(result, "NUNO"); }); Ok(()) } @@ -376,12 +293,6 @@ 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", @@ -410,37 +321,10 @@ mod tests { actual.load_environment(); actual.sync_env_vars(&mut ctx); - 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."); + let env_vars = ctx.scope.get_env_vars(); + let result = env_vars.get("SHELL").unwrap(); - 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)); - } + assert_eq!(result, "/usr/bin/you_already_made_the_nu_choice"); }); Ok(()) @@ -502,32 +386,10 @@ mod tests { // Nu sees the missing "/path/to/be/added" and accounts for it. actual.sync_path_vars(&mut ctx); - 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."); + let env_vars = ctx.scope.get_env_vars(); + let paths = env_vars.get("PATH").unwrap(); - 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); + assert_eq!(paths, &expected); }); Ok(()) @@ -576,32 +438,10 @@ mod tests { actual.load_environment(); actual.sync_path_vars(&mut ctx); - 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."); + let env_vars = ctx.scope.get_env_vars(); + let paths = env_vars.get("PATH").unwrap(); - 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); + 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 2447b8743b..c1db4fdd18 100644 --- a/crates/nu-command/src/commands/with_env.rs +++ b/crates/nu-command/src/commands/with_env.rs @@ -69,9 +69,17 @@ impl WholeStreamCommand for WithEnv { } async fn with_env(raw_args: CommandArgs) -> Result { + let redirection = raw_args.call_info.args.external_redirection; let context = EvaluationContext::from_args(&raw_args); - let (WithEnvArgs { variable, block }, input) = raw_args.process().await?; + let ( + WithEnvArgs { + variable, + mut block, + }, + input, + ) = 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 6b58508b7d..363d57ef6d 100644 --- a/crates/nu-command/src/script.rs +++ b/crates/nu-command/src/script.rs @@ -186,7 +186,6 @@ pub async fn process_script( ctx.scope.add_env(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-engine/src/evaluate/scope.rs b/crates/nu-engine/src/evaluate/scope.rs index bbc3408d56..ff67d97867 100644 --- a/crates/nu-engine/src/evaluate/scope.rs +++ b/crates/nu-engine/src/evaluate/scope.rs @@ -176,7 +176,8 @@ impl Scope { pub fn add_env_var(&self, name: impl Into, value: String) { if let Some(frame) = self.frames.lock().last_mut() { - frame.env.insert(name.into(), value); + let name = name.into(); + frame.env.insert(name, value); } } diff --git a/tests/shell/pipeline/commands/internal.rs b/tests/shell/pipeline/commands/internal.rs index 31833c8b8c..df9d642e2e 100644 --- a/tests/shell/pipeline/commands/internal.rs +++ b/tests/shell/pipeline/commands/internal.rs @@ -220,6 +220,51 @@ fn autoenv() { }) } +// #[cfg(feature = "which")] +// #[test] +// fn nu_let_env_overwrites() { +// Playground::setup("syncs_env_test_1", |dirs, sandbox| { +// sandbox.with_files(vec![FileWithContent( +// "configuration.toml", +// r#" +// [env] +// SHELL = "/usr/bin/you_already_made_the_nu_choice" +// "#, +// )]); + +// let mut file = dirs.test().clone(); +// file.push("configuration.toml"); + +// let fake_config = FakeConfig::new(&file); +// let mut actual = EnvironmentSyncer::new(); +// actual.set_config(Box::new(fake_config)); + +// // Here, the environment variables from the current session +// // are cleared since we will load and set them from the +// // configuration file (if any) +// actual.clear_env_vars(&mut ctx); + +// // Nu loads the environment variables from the configuration file (if any) +// actual.load_environment(); + +// // By this point, Nu has already loaded the environment variables +// // stored in the configuration file. Before continuing we check +// // if any new environment variables have been added from the ones loaded +// // in the configuration file. +// // +// // Nu sees the missing "USER" variable and accounts for it. +// actual.sync_env_vars(&mut ctx); + +// let actual = nu!( +// cwd: dirs.test(), +// r#"let-env SHELL = bob +// echo $nu.env.SHELL +// "# +// ); +// assert!(actual.out.ends_with("set_in_foo")) +// }); +// } + #[test] fn invocation_properly_redirects() { let actual = nu!(