Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixup environment leakage for Command #740

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

haudren-woven
Copy link
Contributor

The Command substitution was not using the context's environment to launch itself, and defaults to the parent process environment. This caused bugs, as described in #715, which I re-state below.

Fixing this is very simple, just pass the environment of the context to the subprocess spawned in Command.

How to reproduce? I couldn't quite come up with a very simple unit test yet, but the below procedure should work.

First a simple executable:

import os

print(os.environ.get("FOO", "There is no foo"))

Place it somewhere on your path, I named it print_env.py.

Then the following launch file:

import launch

def generate_launch_description():
    ld = launch.LaunchDescription()

    ld.add_action(
        launch.actions.ExecuteProcess(
            cmd=["print_env.py"], additional_env={"FOO": "BAR"}, output="screen"
        )
    )

    ld.add_action(launch.actions.ExecuteProcess(cmd=["print_env.py"], output="screen"))

    ld.add_action(
        launch.actions.ExecuteProcess(
            cmd=["print_env.py"],
            output="screen",
            additional_env={
                "FOO": launch.substitutions.Command(
                    [
                        "print_env.py",
                    ]
                )
            },
        )
    )

    return ld

This launches three "printenv" processes:

  • The first one has additional_env set to FOO=BAR
  • The second one has nothing set
  • The third one sets the FOO value to the contents of the environment variable, using a substitutions.Command

So I expect:

  • BAR
  • There is no foo
  • There is no foo

What I get:

  • BAR
  • There is no foo
  • BAR !

The `Command` substitution was not using the context's environment to
launch itself, and defaults to the parent process environment. This
caused bugs, as described in ros2#715, which I re-state below.

Fixing this is very simple, just pass the environment of the context to
the subprocess spawned in Command.

How to reproduce? I couldn't quite come up with a very simple unit test
yet, but the below procedure should work.

First a simple executable:
```
import os

print(os.environ.get("FOO", "There is no foo"))
```

Place it somewhere on your path, I named it print_env.py.

Then the following launch file:
```
import launch

def generate_launch_description():
    ld = launch.LaunchDescription()

    ld.add_action(
        launch.actions.ExecuteProcess(
            cmd=["print_env.py"], additional_env={"FOO": "BAR"}, output="screen"
        )
    )

    ld.add_action(launch.actions.ExecuteProcess(cmd=["print_env.py"], output="screen"))

    ld.add_action(
        launch.actions.ExecuteProcess(
            cmd=["print_env.py"],
            output="screen",
            additional_env={
                "FOO": launch.substitutions.Command(
                    [
                        "print_env.py",
                    ]
                )
            },
        )
    )

    return ld
```
This launches three "printenv" processes:

* The first one has additional_env set to FOO=BAR
* The second one has nothing set
* The third one sets the FOO value to the contents of the environment variable,
  using a substitutions.Command

So I expect:

* BAR
* There is no foo
* There is no foo

What I get:

* BAR
* There is no foo
* BAR !
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants