ros2 / launch

Tools for launching multiple processes and for writing tests involving multiple processes.
Apache License 2.0
126 stars 141 forks source link

Support scoping environment variables #601

Closed jacobperron closed 2 years ago

jacobperron commented 2 years ago

Resolves #597

Wherever actions were previously using os.environ, now get environment variables from the launch context. There are a few exceptions where os.environ is still used since a launch context is not available (e.g. logging directory and overriding launch process output).

Maintain a stack of environment variables in the launch context, similar to launch configurations. Add new actions PushEnvironment and PopEnvironment to interact with the stack. Push and pop the environment inside the GroupAction to support scoping.

Add a new ResetEnvironment action that sets the contexts environment back to it's initial state (os.environ). Return ResetEnvironment as part of GroupAction when forwarding=False.

jacobperron commented 2 years ago

Testing packages above launch:

jacobperron commented 2 years ago

I'm also interested in any objections to backporting this change (which was my plan). I think it should be fine, but I imagine there may be consequences I can't think of since we're changing how launch interacts with os.environ.

jacobperron commented 2 years ago

This change breaks a test in tracetools_launch that was assuming launch would set os.environ, https://ci.ros2.org/job/ci_linux-aarch64/10905/testReport/tracetools_launch.tracetools_launch.test.tracetools_launch.test_ld_preload_action/TestLdPreloadAction/test_action/

While it's easy to fix the test, it's an example of how this change may break assumptions if we backport it.

jacobperron commented 2 years ago

Another test that breaks because of this change, see https://github.com/ros2/rcutils/pull/354

I'll see if I can modify this PR so that these tests don't break.

jacobperron commented 2 years ago

Okay, with 3d2a695, we're now operating directly on os.environ so we don't need #354 or any changes to ros2_tracing.

In retrospect, I think this is much simpler and technically we don't need the context.environment abstraction (though I kind of like it). Let me know what you think.

jacobperron commented 2 years ago
wjwwood commented 2 years ago

Can we discuss this at the next ROS 2 meeting and/or a dedicated meeting?

jacobperron commented 2 years ago

@hidmic @wjwwood To try and summarize our offline discussion:

Please provide any corrections to my summary or add important points I may have missed.

jacobperron commented 2 years ago
Blast545 commented 2 years ago

:man_farmer: I think this PR introduced some test regressions in the repeated jobs buildfarm:

launch.test.launch.actions.test_append_environment_variable.test_append_environment_variable_execute[2-3] launch.test.launch.actions.test_append_environment_variable.test_append_environment_variable_execute[3-3] launch.test.launch.actions.test_group_action.test_group_action_execute[2-3] launch.test.launch.actions.test_group_action.test_group_action_execute[3-3] launch.test.launch.actions.test_set_and_unset_environment_variable.test_set_and_unset_environment_variable_execute[2-3] launch.test.launch.actions.test_set_and_unset_environment_variable.test_set_and_unset_environment_variable_execute[3-3]

First time appearing: https://ci.ros2.org/view/nightly/job/nightly_linux_repeated/2602/ Failing constantly since then. AFAIK @sloretz is looking into this.

jacobperron commented 2 years ago

Backport PRs are open: