Page MenuHomePhabricator

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

Authored by friss on Mar 12 2020, 3:35 PM.

Details

Reviewers
labath
jingham
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.

Diff Detail

Unit TestsFailed

TimeTest
220 mslibunwind.libunwind::frameheadercache_test.pass.cpp
Command: ['/usr/bin/clang++', '-o', '/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libunwind/test/Output/frameheadercache_test.pass.cpp.o', '-x', 'c++', '/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libunwind/test/frameheadercache_test.pass.cpp', '-c', '-v', '-ftemplate-depth=270', '-Werror=thread-safety', '-DLIBUNWIND_NO_TIMER', '-funwind-tables', '-std=c++2a', '-I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libunwind/include', '-D__STDC_FORMAT_MACROS', '-D__STDC_LIMIT_MACROS', '-D__STDC_CONSTANT_MACROS', '-I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libunwind/../libcxx/test/support', '-DLIBCXX_FILESYSTEM_STATIC_TEST_ROOT="/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/std/input.output/filesystems/Inputs/static_test_env"', '-DLIBCXX_FILESYSTEM_DYNAMIC_TEST_ROOT="/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libunwind/test/filesystem/Output/dynamic_env"', '-DLIBCXX_FILESYSTEM_DYNAMIC_TEST_HELPER="/usr/bin/python /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libunwind/../libcxx/test/support/filesystem_dynamic_test_helper.py"', '-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER', '-Wall', '-Wextra', '-Werror', '-Wuser-defined-warnings', '-Wshadow', '-Wno-unused-command-line-argument', '-Wno-attributes', '-Wno-pessimizing-move', '-Wno-c++11-extensions', '-Wno-user-defined-literals', '-Wno-noexcept-type', '-Wsign-compare', '-Wunused-variable', '-Wunused-parameter', '-Wunreachable-code', '-c'] Exit Code: 1 Standard Error:

Event Timeline

friss created this revision.Mar 12 2020, 3:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2020, 3:35 PM

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 added a comment.Mar 12 2020, 5:07 PM

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.

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)

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.