Add option to split arguments with shlex.split in ExecuteProcess

launch/launch/actions/execute_local.py

@@ -19,6 +19,7 @@
 import logging
 import os
 import platform
+import shlex
 import signal
 import traceback
 from typing import Any  # noqa: F401
@@ -73,6 +74,8 @@
 from ..utilities.type_utils import normalize_typed_substitution
 from ..utilities.type_utils import perform_typed_substitution
 
+g_is_windows = 'win' in platform.system().lower()
+
 
 class ExecuteLocal(Action):
     """Action that begins executing a process on the local system and sets up event handlers."""
@@ -82,6 +85,10 @@ def __init__(
         *,
         process_description: Executable,
         shell: bool = False,
+        split_arguments: Union[
+            bool,
+            SomeSubstitutionsType
+        ] = LaunchConfiguration('split_arguments', default='False'),
         sigterm_timeout: SomeSubstitutionsType = LaunchConfiguration(
             'sigterm_timeout', default=5),
         sigkill_timeout: SomeSubstitutionsType = LaunchConfiguration(
@@ -150,6 +157,24 @@ def __init__(
         :param: process_description the `launch.descriptions.Executable` to execute
             as a local process
         :param: shell if True, a shell is used to execute the cmd
+        :param: split_arguments if True, and shell=False, the arguments are
+            split by whitespace as if parsed by a shell, e.g. like shlex.split()
+            from Python's standard library.
+            Useful if the arguments need to be split, e.g. if a substitution
+            evaluates to multiple whitespace separated arguments but shell=True
+            cannot be used.
+            Usually it does not make sense to split the arguments if shell=True
+            because the shell will split them again, e.g. the single
+            substitution '--opt1 --opt2 "Hello world"' would become ['--opt1',
+            '--opt2', '"Hello World"'] if split_arguments=True, and then become
+            ['--opt1', '--opt2', 'Hello', 'World'] because of shell=True, which
+            is likely not what was originally intended.
+            Therefore, when both shell=True and split_arguments=True, the
+            arguments will not be split before executing, depending on the
+            shell to split the arguments instead.
+            If not explicitly passed, the LaunchConfiguration called
+            'split_arguments' will be used as the default, and if that
+            LaunchConfiguration is not set, the default will be False.
         :param: sigterm_timeout time until shutdown should escalate to SIGTERM,
             as a string or a list of strings and Substitutions to be resolved
             at runtime, defaults to the LaunchConfiguration called
@@ -188,6 +213,9 @@ def __init__(
         super().__init__(**kwargs)
         self.__process_description = process_description
         self.__shell = shell
+        if isinstance(split_arguments, bool):
+            split_arguments = 'True' if split_arguments else 'False'
+        self.__split_arguments = normalize_to_list_of_substitutions(split_arguments)
         self.__sigterm_timeout = normalize_to_list_of_substitutions(sigterm_timeout)
         self.__sigkill_timeout = normalize_to_list_of_substitutions(sigkill_timeout)
         self.__emulate_tty = emulate_tty
@@ -234,6 +262,11 @@ def shell(self):
         """Getter for shell."""
         return self.__shell
 
+    @property
+    def split_arguments(self):
+        """Getter for split_arguments."""
+        return self.__split_arguments
+
     @property
     def emulate_tty(self):
         """Getter for emulate_tty."""
@@ -326,7 +359,7 @@ def __on_signal_process_event(
                     typed_event.signal_name, self.process_details['name']),
             )
             return None
-        if platform.system() == 'Windows' and typed_event.signal_name == 'SIGINT':
+        if g_is_windows and typed_event.signal_name == 'SIGINT':
             # TODO(wjwwood): remove this when/if SIGINT is fixed on Windows
             self.__logger.warning(
                 "'SIGINT' sent to process[{}] not supported on Windows, escalating to 'SIGTERM'"
@@ -550,11 +583,14 @@ async def __execute_process(self, context: LaunchContext) -> None:
         cwd = process_event_args['cwd']
         env = process_event_args['env']
         if self.__log_cmd:
-            self.__logger.info("process details: cmd='{}', cwd='{}', custom_env?={}".format(
-                ' '.join(filter(lambda part: part.strip(), cmd)),
-                cwd,
-                'True' if env is not None else 'False'
-            ))
+            self.__logger.info(
+                "process details: cmd=['{}'], cwd='{}', shell='{}', custom_env?={}".format(
+                    "', '".join(cmd),
+                    cwd,
+                    'True' if self.__shell else 'False',
+                    'True' if env is not None else 'False',
+                )
+            )
 
         emulate_tty = self.__emulate_tty
         if 'emulate_tty' in context.launch_configurations:
@@ -593,8 +629,8 @@ async def __execute_process(self, context: LaunchContext) -> None:
         if returncode == 0:
             self.__logger.info('process has finished cleanly [pid {}]'.format(pid))
         else:
-            self.__logger.error("process has died [pid {}, exit code {}, cmd '{}'].".format(
-                pid, returncode, ' '.join(filter(lambda part: part.strip(), cmd))
+            self.__logger.error("process has died [pid {}, exit code {}, cmd ['{}']].".format(
+                pid, returncode, "', '".join(cmd)
             ))
         await context.emit_event(
                 ProcessExited(returncode=returncode, **process_event_args)
@@ -714,6 +750,18 @@ def execute(self, context: LaunchContext) -> Optional[List[LaunchDescriptionEnti
             else:
                 self.__stdout_logger, self.__stderr_logger = \
                     launch.logging.get_output_loggers(name, self.__output)
+            # Update the cmd to respect split_arguments option.
+            if evaluate_condition_expression(context, self.__split_arguments):
+                if self.__shell:
+                    self.__logger.debug("Ignoring 'split_arguments=True' because 'shell=True'.")
+                else:
+                    self.__logger.debug("Splitting arguments because 'split_arguments=True'.")
+                    expanded_cmd = []
+                    assert self.__process_event_args is not None
+                    for token in self.__process_event_args['cmd']:
+                        expanded_cmd.extend(shlex.split(token, posix=(not g_is_windows)))
+                    # Also update self.__process_event_args['cmd'] so it reflects the splitting.
+                    self.__process_event_args['cmd'] = expanded_cmd
             context.asyncio_loop.create_task(self.__execute_process(context))
         except Exception:
             for event_handler in event_handlers:

launch/launch/actions/execute_process.py

@@ -14,13 +14,16 @@
 
 """Module for the ExecuteProcess action."""
 
+import platform
 import shlex
 from typing import Dict
 from typing import Iterable
 from typing import List
 from typing import Optional
 from typing import Text
 
+import launch.logging
+
 from .execute_local import ExecuteLocal
 from .shutdown_action import Shutdown
 from ..descriptions import Executable
@@ -32,6 +35,8 @@
 from ..substitution import Substitution
 from ..substitutions import TextSubstitution
 
+g_is_windows = 'win' in platform.system().lower()
+
 
 @expose_action('executable')
 class ExecuteProcess(ExecuteLocal):
@@ -260,14 +265,21 @@ def _append_arg():
             nonlocal arg
             result_args.append(arg)
             arg = []
+
         for sub in parser.parse_substitution(cmd):
             if isinstance(sub, TextSubstitution):
-                tokens = shlex.split(sub.text)
-                if not tokens:
-                    # String with just spaces.
+                try:
+                    tokens = shlex.split(sub.text, posix=(not g_is_windows))
+                except Exception:
+                    logger = launch.logging.get_logger(cls.__name__)
+                    logger.error(f"Failed to parse token '{sub.text}' of cmd '{cmd}'")
+                    raise
+                if len(tokens) == 0:
+                    # String with just spaces produces an empty list.
                     # Appending args allow splitting two substitutions
-                    # separated by a space.
-                    # e.g.: `$(subst1 asd) $(subst2 bsd)` will be two separate arguments.
+                    # separated by some amount of whitespace.
+                    # e.g.: `$(subst1 asd) $(subst2 bsd)` will be two separate arguments,
+                    # first `$(subst1 asd)`, then this case ` `, then the `$(subst2 bsd)`.
                     _append_arg()
                     continue
                 if sub.text[0].isspace():
@@ -397,11 +409,21 @@ def parse(
             if shell is not None:
                 kwargs['shell'] = shell
 
+        if 'split_arguments' not in ignore:
+            split_arguments = entity.get_attr('split_arguments', data_type=bool, optional=True)
+            if split_arguments is not None:
+                kwargs['split_arguments'] = split_arguments
+
         if 'emulate_tty' not in ignore:
             emulate_tty = entity.get_attr('emulate_tty', data_type=bool, optional=True)
             if emulate_tty is not None:
                 kwargs['emulate_tty'] = emulate_tty
 
+        if 'log_cmd' not in ignore:
+            log_cmd = entity.get_attr('log_cmd', data_type=bool, optional=True)
+            if log_cmd is not None:
+                kwargs['log_cmd'] = log_cmd
+
         if 'additional_env' not in ignore:
             # Conditions won't be allowed in the `env` tag.
             # If that feature is needed, `set_enviroment_variable` and

launch/launch/descriptions/executable.py

@@ -18,6 +18,7 @@
 """Module for a description of an Executable."""
 
 import os
+import platform
 import re
 import shlex
 import threading
@@ -37,6 +38,7 @@
 
 _executable_process_counter_lock = threading.Lock()
 _executable_process_counter = 0  # in Python3, this number is unbounded (no rollover)
+g_is_windows = 'win' in platform.system().lower()
 
 
 class Executable:
@@ -179,7 +181,9 @@ def prepare(self, context: LaunchContext, action: Action):
             # Apply if filter regex matches (empty regex matches all strings)
             should_apply_prefix = re.match(prefix_filter, os.path.basename(cmd[0])) is not None
         if should_apply_prefix:
-            cmd = shlex.split(perform_substitutions(context, self.__prefix)) + cmd
+            cmd = shlex.split(
+                perform_substitutions(context, self.__prefix), posix=(not g_is_windows)
+            ) + cmd
         self.__final_cmd = cmd
         name = os.path.basename(cmd[0]) if self.__name is None \
             else perform_substitutions(context, self.__name)

launch/launch/substitutions/command.py

@@ -14,7 +14,7 @@
 
 """Module for the Command substitution."""
 
-import os
+import platform
 import shlex
 import subprocess
 from typing import List
@@ -30,6 +30,8 @@
 from ..some_substitutions_type import SomeSubstitutionsType
 from ..substitution import Substitution
 
+g_is_windows = 'win' in platform.system().lower()
+
 
 @expose_substitution('command')
 class Command(Substitution):
@@ -94,10 +96,7 @@ def perform(self, context: LaunchContext) -> Text:
         from ..utilities import perform_substitutions  # import here to avoid loop
         command_str = perform_substitutions(context, self.command)
         command: Union[str, List[str]]
-        if os.name != 'nt':
-            command = shlex.split(command_str)
-        else:
-            command = command_str
+        command = shlex.split(command_str, posix=(not g_is_windows))
         on_stderr = perform_substitutions(context, self.on_stderr)
         if on_stderr not in ('fail', 'ignore', 'warn', 'capture'):
             raise SubstitutionFailure(

launch/test/launch/argv_echo.py

@@ -0,0 +1,28 @@
+# Copyright 2023 Open Source Robotics Foundation, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Used from test_execute_process.py and others."""
+
+
+import sys
+
+
+def print_argv(argv):
+    joined_args = "', '".join(argv)
+    print(f"['{joined_args}']")
+    sys.exit(len(argv))
+
+
+if __name__ == '__main__':
+    print_argv(sys.argv)