[Lldb-commits] [PATCH] D76105: [lldb/settings] Reset the inferior environment when target.inherit-env is toggled

Fri Mar 13 10:11:59 PDT 2020

jingham added a comment.

In D76105#1921273 <https://reviews.llvm.org/D76105#1921273>, @labath wrote:

> In D76105#1920753 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76105/new/

https://reviews.llvm.org/D76105