[lldb/settings] Reset the inferior environment when target.inherit-env is toggled

[friss]

Summary

Allow the target.env-vars property to be recomputed when the
target.inherit-env property changes. This allows to change
the value of the property between runs and get a meaningful
behavior.

[jingham]

If I'm following the logic correctly, if you run a target with inherit-env off, and then do:

env VAR_IN_ENVIRONMENT=NOT_THE_ENVIRONMENTS_VALUE

then turn inherit-env on, we will preserve the value you set it to, not the environment value, because you pass in false for can_replace. That seems right to me. It would be good to test that case explicitly, however.

But if you have inherit-env on and then run the command above, and then turn inherit-env off, your changed value will just get deleted. That seems unexpected. I think you have to compare the values in the else branch and only delete the key if the value is the same as the environment value. Does that sound right?

If so, then it would be good to test that as well...

[friss]

In D76105#1920633, @jingham wrote:

If I'm following the logic correctly, if you run a target with inherit-env off, and then do:

env VAR_IN_ENVIRONMENT=NOT_THE_ENVIRONMENTS_VALUE

then turn inherit-env on, we will preserve the value you set it to, not the environment value, because you pass in false for can_replace. That seems right to me. It would be good to test that case explicitly, however.

But if you have inherit-env on and then run the command above, and then turn inherit-env off, your changed value will just get deleted. That seems unexpected. I think you have to compare the values in the else branch and only delete the key if the value is the same as the environment value. Does that sound right?

Yeah, this shortcoming was pretty obvious while writing the patch. I don't like it very much, it seems like the inherit behavior should be handled closer to the point we launch, Or at least the inherited environment should be stored separately from the one you set manually. Comparing values would solve one class of issues, but what about explicitly setting one variable to the same value it has in your environment. Then you would delete it when changing the inherit-env property. Also not very intuitive.

[labath]

In D76105#1920753, @friss wrote:

Yeah, this shortcoming was pretty obvious while writing the patch. I don't like it very much, it seems like the inherit behavior should be handled closer to the point we launch, Or at least the inherited environment should be stored separately from the one you set manually. Comparing values would solve one class of issues, but what about explicitly setting one variable to the same value it has in your environment. Then you would delete it when changing the inherit-env property. Also not very intuitive.

Yeah, I've been thinking about that too. Is it now too late to do anything about that?

In my ideal world the "env-vars" setting would only hold the values that the user has set, and nothing more. And there would be a separate way to get the effective enviroment used for launches and what not. Maybe something like:

• platform environment => get the current platform's environment -- this is the thing that would get inherited
• process environment => get the effective environment -- if a process is running it's the environment the process was launched with, if it's not yet started, it's the (merged) environment it would be launched with. Or maybe these should be separate commands?

We wouldn't have to add all of these commands, if we don't think they are useful. My main point is about avoiding merging user-provided and inherited variables so early in the process..

One gotcha with this flow is that it becomes hard to unset a specific environment variable but inherit the rest. But maybe that doesn't matter and setting the variable to an empty string is enough? Or we can have a "target populate-environment" command which would fill in the host environment into the "target.env-vars" setting and set "inherit-env=false" (I've seen this kind of flow when launching processes from visual studio (through a gui), and I kind of liked it)

[jingham]

In D76105#1921273, @labath wrote:

In D76105#1920753, @friss wrote:

Yeah, this shortcoming was pretty obvious while writing the patch. I don't like it very much, it seems like the inherit behavior should be handled closer to the point we launch, Or at least the inherited environment should be stored separately from the one you set manually. Comparing values would solve one class of issues, but what about explicitly setting one variable to the same value it has in your environment. Then you would delete it when changing the inherit-env property. Also not very intuitive.

Yeah, I've been thinking about that too. Is it now too late to do anything about that?

In my ideal world the "env-vars" setting would only hold the values that the user has set, and nothing more. And there would be a separate way to get the effective enviroment used for launches and what not. Maybe something like:

• platform environment => get the current platform's environment -- this is the thing that would get inherited
• process environment => get the effective environment -- if a process is running it's the environment the process was launched with, if it's not yet started, it's the (merged) environment it would be launched with. Or maybe these should be separate commands?

This seems like a better set-up to me. I like having "platform environment" because it is useful to see "If I were to launch something, when environment would it get" before mucking with it. Particularly if we get around to fetching the starting environment from the remote target, since that's hard to see otherwise.

It's a little awkward that you use commands to see the original and resultant environments, but a "settings show" to see your changes to the original. You could have "target environment" which be the environment YOU have set in the target (same as env-vars), then it would make sense for process to show the aggregate and platform to show the original. That's a little more symmetrical, but so long as you have to use the setting to change it, I'm not so sure this is valuable.

We wouldn't have to add all of these commands, if we don't think they are useful. My main point is about avoiding merging user-provided and inherited variables so early in the process..

Yes, I agree there is value in showing these things separately.

One gotcha with this flow is that it becomes hard to unset a specific environment variable but inherit the rest. But maybe that doesn't matter and setting the variable to an empty string is enough? Or we can have a "target populate-environment" command which would fill in the host environment into the "target.env-vars" setting and set "inherit-env=false" (I've seen this kind of flow when launching processes from visual studio (through a gui), and I kind of liked it)

Setting them to an empty string won't do, I think. There are too many uses of environment variables that just check that the variable exists, and don't care about the value.

As we think about a better way of doing this, I'd like to make it possible to fetch the environment from a remote system. Filling in the host environment for a remote session is not the right thing to do. Filling it with the connected platform's environment is what actually makes some sense.

But if we do it that way you won't have a valid environment till you've connected to some remote platform. That would mean the model where you populate the environment and then excise unwanted entries can only happen properly after targets get created and connected to a platform. That makes the populate then excise model less workable.

This makes me think that maybe settings are not a rich enough interface for environment variables. Maybe it would be better to have "target environment {set/show/remove}" where set tracks with the env-vars, and remove means remove these entries from the composite environment. Then "platform env" and "process env" would be show only.

lldb/source/Target/Target.cpp

const Property *GetPropertyAtIndex(const ExecutionContext *exe_ctx,
     }
     return ProtectedGetPropertyAtIndex(idx);
   }
 
   lldb::TargetSP GetTargetSP() { return m_target->shared_from_this(); }
 
 protected:
   void GetHostEnvironmentIfNeeded() const {
-    if (!m_got_host_env) {
-      if (m_target) {
-        m_got_host_env = true;
-        const uint32_t idx = ePropertyInheritEnv;
-        if (GetPropertyAtIndexAsBoolean(
-                nullptr, idx, g_target_properties[idx].default_uint_value != 0)) {
-          PlatformSP platform_sp(m_target->GetPlatform());
-          if (platform_sp) {
-            Environment env = platform_sp->GetEnvironment();
-            OptionValueDictionary *env_dict =
-                GetPropertyAtIndexAsOptionValueDictionary(nullptr,
-                                                          ePropertyEnvVars);
-            if (env_dict) {
-              const bool can_replace = false;
-              for (const auto &KV : env) {
-                // Don't allow existing keys to be replaced with ones we get
-                // from the platform environment
-                env_dict->SetValueForKey(
-                    ConstString(KV.first()),
-                    OptionValueSP(new OptionValueString(KV.second.c_str())),
-                    can_replace);
-              }
-            }
-          }
+    if (!m_target)
+      return;
+
+    bool should_inherit = GetPropertyAtIndexAsBoolean(
+        nullptr, idx, g_target_properties[idx].default_uint_value != 0);
+
+    if (!m_got_host_env || should_inherit != m_host_env_inherited) {
+      m_got_host_env = true;
+        m_host_env_inherited = should_inherit;
+
+        assert(env_dict && "The target.env-vars property doesn't exist.");
+        if (should_inherit) {
+          const bool can_replace = false;
+          for (const auto &KV : env)
+        } else {
+          for (const auto &KV : env)
+            env_dict->DeleteValueForKey(ConstString(KV.first()));
         }
       }
     }
   }
   Target *m_target;
   mutable bool m_got_host_env;
+  mutable bool m_host_env_inherited;
 };
 
 // TargetProperties
 #define LLDB_PROPERTIES_experimental
 #include "TargetProperties.inc"
 
 enum {
 #define LLDB_PROPERTIES_experimental

lldb/test/API/commands/settings/TestSettings.py

def test_pass_host_env_vars(self):
 
         self.expect(
             output,
             exe=False,
             substrs=[
                 "The host environment variable 'MY_HOST_ENV_VAR1' successfully passed.",
                 "The host environment variable 'MY_HOST_ENV_VAR2' successfully passed."])
 
+        self.runCmd('settings set target.inherit-env false')
+
+        self.addTearDownHook(unset_env_variables)
+        self.runCmd("process launch --working-dir '{0}'".format(self.get_process_working_directory()),
+                RUN_SUCCEEDED)
+
+        # Read the output file produced by running the program.
+        output = lldbutil.read_file_from_process_wd(self, "output1.txt")
+
+        self.expect(
+            output,
+            matching=False,
+            exe=False,
+            substrs=[
+                "The host environment variable 'MY_HOST_ENV_VAR1' successfully passed.",
+                "The host environment variable 'MY_HOST_ENV_VAR2' successfully passed."])
+
     @skipIfDarwinEmbedded   # <rdar://problem/34446098> debugserver on ios etc can't write files
     def test_set_error_output_path(self):
         """Test that setting target.error/output-path for the launched process works."""
         self.build()
 
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)