From 11beb258a44e6303665fe96862ac84209dfd940d Mon Sep 17 00:00:00 2001 From: William Woodall Date: Fri, 15 Sep 2023 16:59:43 -0700 Subject: [PATCH 01/12] add split_arguments argument for ExecuteLocal Signed-off-by: William Woodall --- launch/launch/actions/execute_local.py | 50 +++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/launch/launch/actions/execute_local.py b/launch/launch/actions/execute_local.py index 487ae905c..5be9ac5cd 100644 --- a/launch/launch/actions/execute_local.py +++ b/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 @@ -82,6 +83,8 @@ def __init__( *, process_description: Executable, shell: bool = False, + split_arguments: SomeSubstitutionsType = LaunchConfiguration( + 'split_arguments', default=False), sigterm_timeout: SomeSubstitutionsType = LaunchConfiguration( 'sigterm_timeout', default=5), sigkill_timeout: SomeSubstitutionsType = LaunchConfiguration( @@ -150,6 +153,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 +209,7 @@ def __init__( super().__init__(**kwargs) self.__process_description = process_description self.__shell = shell + 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 +256,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.""" @@ -547,14 +574,27 @@ async def __execute_process(self, context: LaunchContext) -> None: raise RuntimeError('process_event_args unexpectedly None') cmd = process_event_args['cmd'] + if evaluate_condition_expression(context, self.__split_arguments): + if self.__shell: + self.__logger.debug( + "Ignoring 'split_arguments=True' because 'shell=True'." + ) + else: + expanded_cmd = [] + for token in cmd: + expanded_cmd.extend(shlex.split(token)) + cmd = expanded_cmd 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: From 4922f167240a6e3c09a7212006a5cb8e83406434 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Tue, 19 Sep 2023 17:44:08 -0700 Subject: [PATCH 02/12] add tests for split_arguments argument in ExecuteLocal Signed-off-by: William Woodall --- launch/launch/actions/execute_local.py | 13 +++-- launch/test/launch/test_execute_process.py | 67 ++++++++++++++++++++++ 2 files changed, 76 insertions(+), 4 deletions(-) diff --git a/launch/launch/actions/execute_local.py b/launch/launch/actions/execute_local.py index 5be9ac5cd..f861271c9 100644 --- a/launch/launch/actions/execute_local.py +++ b/launch/launch/actions/execute_local.py @@ -83,8 +83,11 @@ def __init__( *, process_description: Executable, shell: bool = False, - split_arguments: SomeSubstitutionsType = LaunchConfiguration( - 'split_arguments', default=False), + split_arguments: Union[ + SomeSubstitutionsType, + bool + ] = LaunchConfiguration( + 'split_arguments', default='False'), sigterm_timeout: SomeSubstitutionsType = LaunchConfiguration( 'sigterm_timeout', default=5), sigkill_timeout: SomeSubstitutionsType = LaunchConfiguration( @@ -209,6 +212,8 @@ 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) @@ -633,8 +638,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) diff --git a/launch/test/launch/test_execute_process.py b/launch/test/launch/test_execute_process.py index fe6408fbe..11367351b 100644 --- a/launch/test/launch/test_execute_process.py +++ b/launch/test/launch/test_execute_process.py @@ -318,3 +318,70 @@ def test_execute_process_prefix_filter_override_in_launch_file(): test_process.execute(lc) assert 'echo' in test_process.process_details['cmd'] and \ 'time' not in test_process.process_details['cmd'] + + +preamble = [sys.executable, os.path.join(os.path.dirname(__file__), 'argv_echo.py')] + + +@pytest.mark.parametrize('test_parameters', [ + # This case will result in 2 arguments, keeping the --some-arg and "some string" together. + { + 'cmd': preamble + ['--some-arg "some string"'], + 'shell': False, + 'split_arguments': False, + 'expected_number_of_args': 2, + }, + # This case will split the --some-arg and "some string" up, resulting in 3 args. + { + 'cmd': preamble + ['--some-arg "some string"'], + 'shell': False, + 'split_arguments': True, + 'expected_number_of_args': 3, + }, + # This case the split_arguments is ignored, due to shell=True, + # and produces again 3 arguments, not 4. + { + 'cmd': preamble + ['--some-arg "some string"'], + 'shell': True, + 'split_arguments': True, + 'expected_number_of_args': 3, + }, + # This is the "normal" shell=True behavior. + { + 'cmd': preamble + ['--some-arg "some string"'], + 'shell': True, + 'split_arguments': False, + 'expected_number_of_args': 3, + }, + # Test single argument for cmd (still a list), which will require shell=True. + { + 'cmd': [' '.join(preamble + ['--some-arg "some string"'])], + 'shell': True, + 'split_arguments': False, + 'expected_number_of_args': 3, + }, + # This case also ignores split_arguments. + { + 'cmd': [' '.join(preamble + ['--some-arg "some string"'])], + 'shell': True, + 'split_arguments': True, + 'expected_number_of_args': 3, + }, +]) +def test_execute_process_split_arguments(test_parameters): + """Test the use of the split_arguments option.""" + execute_process_action = ExecuteProcess( + cmd=test_parameters['cmd'], + output='screen', + shell=test_parameters['shell'], + split_arguments=test_parameters['split_arguments'], + ) + + ld = LaunchDescription([ + execute_process_action, + ]) + ls = LaunchService() + ls.include_launch_description(ld) + assert 0 == ls.run(shutdown_when_idle=True) + assert execute_process_action.return_code == test_parameters['expected_number_of_args'], \ + execute_process_action.process_details['cmd'] From e45bed9f5a185f0e9013f991bb7e3a10ac4b8e90 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Tue, 19 Sep 2023 18:57:02 -0700 Subject: [PATCH 03/12] add missing test executable Signed-off-by: William Woodall --- launch/test/launch/argv_echo.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 launch/test/launch/argv_echo.py diff --git a/launch/test/launch/argv_echo.py b/launch/test/launch/argv_echo.py new file mode 100644 index 000000000..ac6b3915c --- /dev/null +++ b/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) From 95abdc29144ceb127c230be3e976d2332995abc5 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Tue, 19 Sep 2023 19:08:11 -0700 Subject: [PATCH 04/12] fixups Signed-off-by: William Woodall --- launch/launch/actions/execute_local.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/launch/launch/actions/execute_local.py b/launch/launch/actions/execute_local.py index f861271c9..0203d8c44 100644 --- a/launch/launch/actions/execute_local.py +++ b/launch/launch/actions/execute_local.py @@ -84,10 +84,9 @@ def __init__( process_description: Executable, shell: bool = False, split_arguments: Union[ - SomeSubstitutionsType, - bool - ] = LaunchConfiguration( - 'split_arguments', default='False'), + bool, + SomeSubstitutionsType + ] = LaunchConfiguration('split_arguments', default='False'), sigterm_timeout: SomeSubstitutionsType = LaunchConfiguration( 'sigterm_timeout', default=5), sigkill_timeout: SomeSubstitutionsType = LaunchConfiguration( @@ -581,20 +580,21 @@ async def __execute_process(self, context: LaunchContext) -> None: cmd = process_event_args['cmd'] if evaluate_condition_expression(context, self.__split_arguments): if self.__shell: - self.__logger.debug( - "Ignoring 'split_arguments=True' because 'shell=True'." - ) + self.__logger.debug("Ignoring 'split_arguments=True' because 'shell=True'.") else: + self.__logger.debug("Splitting arguments because 'split_arguments=True'.") expanded_cmd = [] for token in cmd: expanded_cmd.extend(shlex.split(token)) cmd = expanded_cmd + # Also update process_event_args so they reflect the splitting. + process_event_args['cmd'] = cmd cwd = process_event_args['cwd'] env = process_event_args['env'] if self.__log_cmd: self.__logger.info( "process details: cmd=['{}'], cwd='{}', shell='{}', custom_env?={}".format( - "', ' ".join(cmd), + "', '".join(cmd), cwd, 'True' if self.__shell else 'False', 'True' if env is not None else 'False', From 82069ed8d24ca04ec9b01b92443cbba11a52c918 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Tue, 19 Sep 2023 19:09:38 -0700 Subject: [PATCH 05/12] working on xml tests for split_arguments, not working yet Signed-off-by: William Woodall --- launch/launch/actions/execute_process.py | 19 +++++++++++- launch/test/launch/test_execute_process.py | 30 +++++++++++++++++-- launch_xml/test/launch_xml/test_executable.py | 20 +++++++++++++ 3 files changed, 65 insertions(+), 4 deletions(-) diff --git a/launch/launch/actions/execute_process.py b/launch/launch/actions/execute_process.py index c780af7be..4e3fae76c 100644 --- a/launch/launch/actions/execute_process.py +++ b/launch/launch/actions/execute_process.py @@ -21,6 +21,8 @@ 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 @@ -262,7 +264,12 @@ def _append_arg(): arg = [] for sub in parser.parse_substitution(cmd): if isinstance(sub, TextSubstitution): - tokens = shlex.split(sub.text) + try: + tokens = shlex.split(sub.text) + except: + logger = launch.logging.get_logger(cls.__name__) + logger.error(f"Failed to parse token '{sub.text}' of cmd '{cmd}'") + raise if not tokens: # String with just spaces. # Appending args allow splitting two substitutions @@ -397,11 +404,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 diff --git a/launch/test/launch/test_execute_process.py b/launch/test/launch/test_execute_process.py index 11367351b..30d434dd2 100644 --- a/launch/test/launch/test_execute_process.py +++ b/launch/test/launch/test_execute_process.py @@ -29,6 +29,7 @@ from launch.actions.register_event_handler import RegisterEventHandler from launch.actions.shutdown_action import Shutdown from launch.actions.timer_action import TimerAction +from launch.conditions import evaluate_condition_expression from launch.event_handlers.on_process_start import OnProcessStart from launch.events.shutdown import Shutdown as ShutdownEvent from launch.substitutions.launch_configuration import LaunchConfiguration @@ -377,11 +378,34 @@ def test_execute_process_split_arguments(test_parameters): split_arguments=test_parameters['split_arguments'], ) - ld = LaunchDescription([ - execute_process_action, - ]) + ld = LaunchDescription([execute_process_action]) ls = LaunchService() ls.include_launch_description(ld) assert 0 == ls.run(shutdown_when_idle=True) assert execute_process_action.return_code == test_parameters['expected_number_of_args'], \ execute_process_action.process_details['cmd'] + + +def test_execute_process_split_arguments_override_in_launch_file(): + execute_process_args = { + 'cmd': preamble + ['--some-arg "some string"'], + 'output': 'screen', + 'shell': False, + 'log_cmd': True, + } + execute_process_action1 = ExecuteProcess(**execute_process_args) + execute_process_action2 = ExecuteProcess(**execute_process_args) + + ld = LaunchDescription([ + # Control to test the default. + execute_process_action1, + # Change default with LaunchConfiguration, test again. + SetLaunchConfiguration('split_arguments', 'True'), + execute_process_action2, + ]) + ls = LaunchService() + ls.include_launch_description(ld) + assert 0 == ls.run(shutdown_when_idle=True) + + assert execute_process_action1.return_code == 2, execute_process_action1.process_details['cmd'] + assert execute_process_action2.return_code == 3, execute_process_action2.process_details['cmd'] diff --git a/launch_xml/test/launch_xml/test_executable.py b/launch_xml/test/launch_xml/test_executable.py index 878556ea3..a8512abc6 100644 --- a/launch_xml/test/launch_xml/test_executable.py +++ b/launch_xml/test/launch_xml/test_executable.py @@ -16,6 +16,7 @@ import io from pathlib import Path +import sys import textwrap from launch import LaunchService @@ -67,6 +68,25 @@ def test_executable_wrong_subtag(): assert '`executable`' in str(excinfo.value) assert 'whats_this' in str(excinfo.value) + # +split_arguments_example = f""" + + + +""" + + +def test_executable_with_split_arguments(): + """Parse node xml example.""" + root_entity, parser = Parser.load(io.StringIO(split_arguments_example)) + ld = parser.parse_description(root_entity) + ls = LaunchService() + ls.include_launch_description(ld) + assert 0 == ls.run() + + executable = ld.entities[0] + assert False, executable.get_stdout() + def test_executable_on_exit(): xml_file = \ From 6e5c32dcb831868370e880445ca37c3d4cb32f3b Mon Sep 17 00:00:00 2001 From: William Woodall Date: Wed, 20 Sep 2023 16:38:15 -0700 Subject: [PATCH 06/12] move logic out of async fn to avoid race, and fixups Signed-off-by: William Woodall --- launch/launch/actions/execute_local.py | 23 +++++++++++----------- launch/launch/actions/execute_process.py | 12 ++++++----- launch/test/launch/test_execute_process.py | 1 - 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/launch/launch/actions/execute_local.py b/launch/launch/actions/execute_local.py index 0203d8c44..2089066e3 100644 --- a/launch/launch/actions/execute_local.py +++ b/launch/launch/actions/execute_local.py @@ -578,17 +578,6 @@ async def __execute_process(self, context: LaunchContext) -> None: raise RuntimeError('process_event_args unexpectedly None') cmd = process_event_args['cmd'] - 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 = [] - for token in cmd: - expanded_cmd.extend(shlex.split(token)) - cmd = expanded_cmd - # Also update process_event_args so they reflect the splitting. - process_event_args['cmd'] = cmd cwd = process_event_args['cwd'] env = process_event_args['env'] if self.__log_cmd: @@ -759,6 +748,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)) + # 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: diff --git a/launch/launch/actions/execute_process.py b/launch/launch/actions/execute_process.py index 4e3fae76c..5eba900ca 100644 --- a/launch/launch/actions/execute_process.py +++ b/launch/launch/actions/execute_process.py @@ -262,19 +262,21 @@ def _append_arg(): nonlocal arg result_args.append(arg) arg = [] + for sub in parser.parse_substitution(cmd): if isinstance(sub, TextSubstitution): try: tokens = shlex.split(sub.text) - except: + except Exception: logger = launch.logging.get_logger(cls.__name__) logger.error(f"Failed to parse token '{sub.text}' of cmd '{cmd}'") raise - if not tokens: - # String with just spaces. + 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(): diff --git a/launch/test/launch/test_execute_process.py b/launch/test/launch/test_execute_process.py index 30d434dd2..e8b4327d6 100644 --- a/launch/test/launch/test_execute_process.py +++ b/launch/test/launch/test_execute_process.py @@ -29,7 +29,6 @@ from launch.actions.register_event_handler import RegisterEventHandler from launch.actions.shutdown_action import Shutdown from launch.actions.timer_action import TimerAction -from launch.conditions import evaluate_condition_expression from launch.event_handlers.on_process_start import OnProcessStart from launch.events.shutdown import Shutdown as ShutdownEvent from launch.substitutions.launch_configuration import LaunchConfiguration From 74f5f4c04e3b186876597164c343e84fec5bd4fc Mon Sep 17 00:00:00 2001 From: William Woodall Date: Wed, 20 Sep 2023 16:38:54 -0700 Subject: [PATCH 07/12] add yaml and xml tests for split_arguments Signed-off-by: William Woodall --- launch_xml/test/launch_xml/arg_echo.py | 28 ++++++++++++++++++ launch_xml/test/launch_xml/test_executable.py | 18 +++++++----- launch_yaml/test/launch_yaml/arg_echo.py | 28 ++++++++++++++++++ .../test/launch_yaml/test_executable.py | 29 +++++++++++++++++++ 4 files changed, 96 insertions(+), 7 deletions(-) create mode 100644 launch_xml/test/launch_xml/arg_echo.py create mode 100644 launch_yaml/test/launch_yaml/arg_echo.py diff --git a/launch_xml/test/launch_xml/arg_echo.py b/launch_xml/test/launch_xml/arg_echo.py new file mode 100644 index 000000000..978e780b2 --- /dev/null +++ b/launch_xml/test/launch_xml/arg_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_executable.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) diff --git a/launch_xml/test/launch_xml/test_executable.py b/launch_xml/test/launch_xml/test_executable.py index a8512abc6..342f2490e 100644 --- a/launch_xml/test/launch_xml/test_executable.py +++ b/launch_xml/test/launch_xml/test_executable.py @@ -15,6 +15,7 @@ """Test parsing an executable action.""" import io +import os from pathlib import Path import sys import textwrap @@ -68,25 +69,28 @@ def test_executable_wrong_subtag(): assert '`executable`' in str(excinfo.value) assert 'whats_this' in str(excinfo.value) - # -split_arguments_example = f""" + +split_arguments_example1 = f""" - + + """ def test_executable_with_split_arguments(): """Parse node xml example.""" - root_entity, parser = Parser.load(io.StringIO(split_arguments_example)) + root_entity, parser = Parser.load(io.StringIO(split_arguments_example1)) ld = parser.parse_description(root_entity) ls = LaunchService() ls.include_launch_description(ld) assert 0 == ls.run() - executable = ld.entities[0] - assert False, executable.get_stdout() - def test_executable_on_exit(): xml_file = \ diff --git a/launch_yaml/test/launch_yaml/arg_echo.py b/launch_yaml/test/launch_yaml/arg_echo.py new file mode 100644 index 000000000..978e780b2 --- /dev/null +++ b/launch_yaml/test/launch_yaml/arg_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_executable.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) diff --git a/launch_yaml/test/launch_yaml/test_executable.py b/launch_yaml/test/launch_yaml/test_executable.py index a4e1a9bf2..24da21ca3 100644 --- a/launch_yaml/test/launch_yaml/test_executable.py +++ b/launch_yaml/test/launch_yaml/test_executable.py @@ -15,6 +15,8 @@ """Test parsing an executable action.""" import io +import os +import sys import textwrap from launch import LaunchService @@ -82,5 +84,32 @@ def test_executable_on_exit(): assert isinstance(sub_entities[0], Shutdown) +split_arguments_example1 = f""" +launch: +- let: + name: args + value: '--some-arg "some string"' +- executable: + cmd: {sys.executable} {os.path.join(os.path.dirname(__file__), 'arg_echo.py')} $(var args) + log_cmd: True + shell: False + split_arguments: True + output: screen +""" + + +def test_executable_with_split_arguments(): + """Parse node xml example.""" + root_entity, parser = Parser.load(io.StringIO(split_arguments_example1)) + ld = parser.parse_description(root_entity) + ls = LaunchService() + ls.include_launch_description(ld) + assert 0 == ls.run() + + executable = ld.entities[-1] + # expect a return code that matches the number of arguments after the executable + assert executable.return_code == 3 + + if __name__ == '__main__': test_executable() From 1b6c759c7c588bae3d1683549d19e6e827f85d11 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Thu, 21 Sep 2023 15:19:05 -0700 Subject: [PATCH 08/12] fixup yaml indent in test example Signed-off-by: William Woodall --- launch_yaml/test/launch_yaml/test_executable.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/launch_yaml/test/launch_yaml/test_executable.py b/launch_yaml/test/launch_yaml/test_executable.py index 24da21ca3..f2048b8b0 100644 --- a/launch_yaml/test/launch_yaml/test_executable.py +++ b/launch_yaml/test/launch_yaml/test_executable.py @@ -86,10 +86,10 @@ def test_executable_on_exit(): split_arguments_example1 = f""" launch: -- let: - name: args - value: '--some-arg "some string"' -- executable: + - let: + name: args + value: '--some-arg "some string"' + - executable: cmd: {sys.executable} {os.path.join(os.path.dirname(__file__), 'arg_echo.py')} $(var args) log_cmd: True shell: False From fae8e5c0d8d432574f183014e7980e4039534b79 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Tue, 26 Sep 2023 16:18:09 -0700 Subject: [PATCH 09/12] conditionally set the shlex.split() mode for Windows Signed-off-by: William Woodall --- launch/launch/actions/execute_local.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/launch/launch/actions/execute_local.py b/launch/launch/actions/execute_local.py index 2089066e3..320fd4d09 100644 --- a/launch/launch/actions/execute_local.py +++ b/launch/launch/actions/execute_local.py @@ -74,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.""" @@ -357,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'" @@ -757,7 +759,7 @@ def execute(self, context: LaunchContext) -> Optional[List[LaunchDescriptionEnti expanded_cmd = [] assert self.__process_event_args is not None for token in self.__process_event_args['cmd']: - expanded_cmd.extend(shlex.split(token)) + expanded_cmd.extend(shlex.split(token), posix=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)) From 1b7fb0d2ceb10cb3cb1d489f3d715c67a14c29fd Mon Sep 17 00:00:00 2001 From: William Woodall Date: Tue, 26 Sep 2023 16:31:41 -0700 Subject: [PATCH 10/12] fixup typo Signed-off-by: William Woodall --- launch/launch/actions/execute_local.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/launch/launch/actions/execute_local.py b/launch/launch/actions/execute_local.py index 320fd4d09..6ff7563e6 100644 --- a/launch/launch/actions/execute_local.py +++ b/launch/launch/actions/execute_local.py @@ -759,7 +759,7 @@ def execute(self, context: LaunchContext) -> Optional[List[LaunchDescriptionEnti 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=g_is_windows) + expanded_cmd.extend(shlex.split(token, posix=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)) From 66d408b7a4cf12e8187e17a13d043d856b4ca6ac Mon Sep 17 00:00:00 2001 From: William Woodall Date: Wed, 27 Sep 2023 15:42:58 -0700 Subject: [PATCH 11/12] fix posix logic and add test Signed-off-by: William Woodall --- launch/launch/actions/execute_local.py | 2 +- launch/test/launch/test_execute_process.py | 39 ++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/launch/launch/actions/execute_local.py b/launch/launch/actions/execute_local.py index 6ff7563e6..146cca0ae 100644 --- a/launch/launch/actions/execute_local.py +++ b/launch/launch/actions/execute_local.py @@ -759,7 +759,7 @@ def execute(self, context: LaunchContext) -> Optional[List[LaunchDescriptionEnti 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=g_is_windows)) + 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)) diff --git a/launch/test/launch/test_execute_process.py b/launch/test/launch/test_execute_process.py index e8b4327d6..264710496 100644 --- a/launch/test/launch/test_execute_process.py +++ b/launch/test/launch/test_execute_process.py @@ -24,12 +24,15 @@ from launch import LaunchService from launch.actions import SetLaunchConfiguration from launch.actions.emit_event import EmitEvent +from launch.actions.execute_local import g_is_windows from launch.actions.execute_process import ExecuteProcess from launch.actions.opaque_function import OpaqueFunction from launch.actions.register_event_handler import RegisterEventHandler from launch.actions.shutdown_action import Shutdown from launch.actions.timer_action import TimerAction +from launch.event_handlers.on_process_io import OnProcessIO from launch.event_handlers.on_process_start import OnProcessStart +from launch.events.process import ProcessIO from launch.events.shutdown import Shutdown as ShutdownEvent from launch.substitutions.launch_configuration import LaunchConfiguration @@ -408,3 +411,39 @@ def test_execute_process_split_arguments_override_in_launch_file(): assert execute_process_action1.return_code == 2, execute_process_action1.process_details['cmd'] assert execute_process_action2.return_code == 3, execute_process_action2.process_details['cmd'] + + +def test_execute_process_split_arguments_with_windows_like_pathsep(): + # On POSIX platforms the `\` will be removed, but not on windows. + path = b'C:\\some\\path' + execute_process_args = { + 'cmd': preamble + [f'--some-arg {path.decode()}'], + 'output': 'screen', + 'shell': False, + 'split_arguments': True, + 'log_cmd': True, + } + execute_process_action1 = ExecuteProcess(**execute_process_args) + + did_see_path = False + + def on_stdout(event: ProcessIO): + nonlocal did_see_path + if event.from_stdout and path in event.text: + did_see_path = True + + event_handler = OnProcessIO( + target_action=execute_process_action1, + on_stdout=on_stdout, + ) + + ld = LaunchDescription([ + RegisterEventHandler(event_handler), + execute_process_action1, + ]) + ls = LaunchService() + ls.include_launch_description(ld) + assert 0 == ls.run(shutdown_when_idle=True) + + assert execute_process_action1.return_code == 3, execute_process_action1.process_details['cmd'] + assert did_see_path == g_is_windows From 03fed58be3887e8fccd8a1521621a036d7b1f6bd Mon Sep 17 00:00:00 2001 From: William Woodall Date: Mon, 22 Jan 2024 09:50:28 -0800 Subject: [PATCH 12/12] fixup tests Signed-off-by: William Woodall --- launch/launch/actions/execute_process.py | 5 ++- launch/launch/descriptions/executable.py | 6 ++- launch/launch/substitutions/command.py | 9 ++-- launch_xml/test/launch_xml/executable.xml | 5 --- launch_xml/test/launch_xml/test_executable.py | 43 ++++++++++++------- 5 files changed, 40 insertions(+), 28 deletions(-) delete mode 100644 launch_xml/test/launch_xml/executable.xml diff --git a/launch/launch/actions/execute_process.py b/launch/launch/actions/execute_process.py index 5eba900ca..ce511d9d1 100644 --- a/launch/launch/actions/execute_process.py +++ b/launch/launch/actions/execute_process.py @@ -14,6 +14,7 @@ """Module for the ExecuteProcess action.""" +import platform import shlex from typing import Dict from typing import Iterable @@ -34,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): @@ -266,7 +269,7 @@ def _append_arg(): for sub in parser.parse_substitution(cmd): if isinstance(sub, TextSubstitution): try: - tokens = shlex.split(sub.text) + 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}'") diff --git a/launch/launch/descriptions/executable.py b/launch/launch/descriptions/executable.py index 3d1d0640b..930801921 100644 --- a/launch/launch/descriptions/executable.py +++ b/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) diff --git a/launch/launch/substitutions/command.py b/launch/launch/substitutions/command.py index 0aed7a9d6..6d0ddc840 100644 --- a/launch/launch/substitutions/command.py +++ b/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( diff --git a/launch_xml/test/launch_xml/executable.xml b/launch_xml/test/launch_xml/executable.xml deleted file mode 100644 index 3195dd1ad..000000000 --- a/launch_xml/test/launch_xml/executable.xml +++ /dev/null @@ -1,5 +0,0 @@ - - - - - diff --git a/launch_xml/test/launch_xml/test_executable.py b/launch_xml/test/launch_xml/test_executable.py index 342f2490e..73b638760 100644 --- a/launch_xml/test/launch_xml/test_executable.py +++ b/launch_xml/test/launch_xml/test_executable.py @@ -16,9 +16,7 @@ import io import os -from pathlib import Path import sys -import textwrap from launch import LaunchService from launch.actions import Shutdown @@ -26,15 +24,27 @@ import pytest +test_executable_xml = f""" + + + + + +""" + def test_executable(): """Parse node xml example.""" - xml_file = str(Path(__file__).parent / 'executable.xml') - root_entity, parser = Parser.load(xml_file) + root_entity, parser = Parser.load(io.StringIO(test_executable_xml)) ld = parser.parse_description(root_entity) executable = ld.entities[0] cmd = [i[0].perform(None) for i in executable.cmd] - assert cmd == ['ls', '-l', '-a', '-s'] + assert cmd == [sys.executable, '--version'] assert executable.cwd[0].perform(None) == '/' assert executable.name[0].perform(None) == 'my_ls' assert executable.shell is True @@ -50,20 +60,21 @@ def test_executable(): ls = LaunchService() ls.include_launch_description(ld) assert 0 == ls.run() + assert executable.return_code == 0 + + +test_executable_wrong_subtag_xml = """ + + + + + + +""" def test_executable_wrong_subtag(): - xml_file = \ - """\ - - - - - - - """ # noqa, line too long - xml_file = textwrap.dedent(xml_file) - root_entity, parser = Parser.load(io.StringIO(xml_file)) + root_entity, parser = Parser.load(io.StringIO(test_executable_wrong_subtag_xml)) with pytest.raises(ValueError) as excinfo: parser.parse_description(root_entity) assert '`executable`' in str(excinfo.value)