Keep the environment properly set. (#3072)

* Revert "fix prompts on startup (#3056)"

This reverts commit b202951c1d.

* Ensure environment variables synced with global internal Nu Scope.

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 environment.env().is_some() {
-        if let Some(variables) = environment.env() {
+            for (name, value) in ctx.with_host(|host| host.vars()) {
-            for var in variables.row_entries() {
+                if name != "path" && name != "PATH" {
-                if let Ok(string) = var.1.as_string() {
+                    // account for new env vars present in the current session
-                    if var.0 != "path" && var.0 != "PATH" && !nu_env_vars.contains_key(var.0) {
+                    // that aren't loaded from config.
-                        ctx.scope.add_env_var(var.0, string);
+                    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()) {
+                        ctx.scope.add_env_var_to_base(var.0, string);
-            if name != "path" && name != "PATH" && !nu_env_vars.contains_key(&name) {
+                    }
-                ctx.scope.add_env_var(name, value);
+                }
             }
         }
     }
@@ -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 {
+            if let Some(native_paths) = native_paths {
-            for path in std::env::split_paths(&native_paths) {
+                environment.add_path(native_paths);
-                environment.add_path(path.as_os_str().to_os_string());
+
+                ctx.with_host(|host| {
+                    host.env_rm(std::ffi::OsString::from("PATH"));
+                });
             }
-        }
 
-        if let Some(new_paths) = environment.path() {
+            if let Some(new_paths) = environment.path() {
-            let prepared = std::env::join_paths(
+                let prepared = std::env::join_paths(
-                new_paths
+                    new_paths
-                    .table_entries()
+                        .table_entries()
-                    .map(|p| p.as_string())
+                        .map(|p| p.as_string())
-                    .filter_map(Result::ok),
+                        .filter_map(Result::ok),
-            );
+                );
 
-            if let Ok(paths_ready) = prepared {
+                if let Ok(paths_ready) = prepared {
-                ctx.scope
+                    ctx.with_host(|host| {
-                    .add_env_var("PATH", paths_ready.to_string_lossy().to_string());
+                        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();
+            expected.insert("USER".to_string(), "NUNO".to_string());
-            let result = env_vars.get("SHELL").unwrap();
-            assert_eq!(result, "/usr/bin/you_already_made_the_nu_choice");
 
-            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();
+            // Confirms session environment variables are replaced from Nu configuration file
-            let result = env_vars.get("SHELL").unwrap();
+            // including the newer one accounted for.
-            assert_eq!(result, "/usr/bin/you_already_made_the_nu_choice");
+            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();
+                let var_shell = test_host
-            assert_eq!(result, "NUNO");
+                    .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();
+            ctx.with_host(|test_host| {
-            let result = env_vars.get("SHELL").unwrap();
+                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();
+            ctx.with_host(|test_host| {
-            let paths = env_vars.get("PATH").unwrap();
+                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::<Vec<_>>(),
+            )
+            .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();
+            ctx.with_host(|test_host| {
-            let paths = env_vars.get("PATH").unwrap();
+                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::<Vec<_>>(),
+            )
+            .expect("Couldn't join paths.")
+            .into_string()
+            .expect("Couldn't convert to string.");
+
+            assert_eq!(paths, expected);
         });
 
         Ok(())

crates/nu-command/src/commands/with_env.rs

@@ -80,6 +80,7 @@ async fn with_env(raw_args: CommandArgs) -> Result<OutputStream, ShellError> {
     ) = raw_args.process().await?;
 
     block.block.set_redirect(redirection);
+
     let mut env = IndexMap::new();
 
     match &variable.value {

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

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
     }
 }

crates/nu-engine/src/evaluate/scope.rs

@@ -176,8 +176,7 @@ impl Scope {
 
     pub fn add_env_var(&self, name: impl Into<String>, value: String) {
         if let Some(frame) = self.frames.lock().last_mut() {
-            let name = name.into();
+            frame.env.insert(name.into(), value);
-            frame.env.insert(name, value);
         }
     }
 
@@ -192,6 +191,12 @@ impl Scope {
             frame.env.extend(env_vars)
         }
     }
+
+    pub fn add_env_var_to_base(&self, name: impl Into<String>, value: String) {
+        if let Some(frame) = self.frames.lock().first_mut() {
+            frame.env.insert(name.into(), value);
+        }
+    }
 }
 
 impl ParserScope for Scope {

crates/nu-engine/src/evaluate/variables.rs

@@ -21,10 +21,11 @@ pub fn nu(env: &IndexMap<String, String>, tag: impl Into<Tag>) -> Result<Value,
     nu_dict.insert_value("config", UntaggedValue::row(config).into_value(&tag));
 
     let mut table = vec![];
-    let path = std::env::var_os("PATH");
+    for v in env.iter() {
-    if let Some(paths) = path {
+        if v.0 == "PATH" || v.0 == "Path" {
-        for path in std::env::split_paths(&paths) {
+            for path in std::env::split_paths(&v.1) {
-            table.push(UntaggedValue::filepath(path).into_value(&tag));
+                table.push(UntaggedValue::filepath(path).into_value(&tag));
+            }
         }
     }
     nu_dict.insert_value("path", UntaggedValue::table(&table).into_value(&tag));