Change `cmd` parameter in `ExecuteProcess`

[ivanpauno]

Feature request

Feature description

The current format of cmd is a little confusing. e.g.:

ExecuteProcess(['ls -las'])  # fails
ExecuteProcess(['ls -las'], shell=True)  # Ok
ExecuteProcess(['ls', '-las'])  # Ok
ExecuteProcess(['ls', '-las'], shell=True)  # Ok

That is maybe not of much interest, but the following case:

ExecuteProcess(['command', EnvironmentVariable('USER_ARGUMENTS')])

It's not way of specifying more than one argument in that environment variable, e.g.:

export USER_ARGUMENTS="-opt1 -opt2 -opt3"

That will make the program crash. The same problem applies for any substitution. I don't see a possible workaround for that (without forcing shell=True).

My idea is to change the cmd type from:

cmd: Iterable[SomeSubstitutionsType]

to:

cmd: SomeSubstitutionsType

And use shlex.split after the substitutions are performed.

For backwards compatibility, we could accept both:

cmd: Union[SomeSubstitutionsType, Iterable[SomeSubstitutionsType]]

I realize about the problem while working in the launch frontend: https://github.com/ros2/launch/pull/226#discussion_r293036243.

[ivanpauno]

@hidmic FYI

[wjwwood]

That solution seems reasonable to me.

[hidmic]

There's a bit of an issue with this change. We're essentially forcing shell-like parsing on otherwise plain command line arguments. And thus, something like cmd=['ls', 'my/subdir/with spaces/'] would stop working the way it currently does. Same goes for prefixes, where we already are shlex.split'ing them. We probably need a flag for the user to explicitly opt-in (or opt-out) this parsing behavior.

[ivanpauno]

There's a bit of an issue with this change. We're essentially forcing shell-like parsing on otherwise plain command line arguments. And thus, something like cmd=['ls', 'my/subdir/with spaces/'] would stop working the way it currently does. Same goes for prefixes, where we already are shlex.split'ing them. We probably need a flag for the user to explicitly opt-in (or opt-out) this parsing behavior.

I see. So, though it's true that it's not 100% backwards compatible, I think that the change is still reasonable and won't affect much users (and the ones affected should be able to fix the problem easily).

[hidmic]

~We should probably also support a single string then e.g. cmd='ls "my/subdir/with spaces/"'. WDYT?~ That's the original suggestion, nvm.