Fix parsing of cmd line arguments in XML and yaml file (#379)

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

launch/launch/actions/execute_process.py

@@ -245,7 +245,7 @@ def _parse_cmdline(
         cls,
         cmd: Text,
         parser: Parser
-    ):
+    ) -> List[SomeSubstitutionsType]:
         """
         Parse text apt for command line execution.
 
@@ -254,16 +254,52 @@ def _parse_cmdline(
            list again as a `TextSubstitution`.
         :returns: a list of command line arguments.
         """
-        args = []
-        for arg in parser.parse_substitution(cmd):
-            if isinstance(arg, TextSubstitution):
-                text = arg.text
-                text = shlex.split(text)
-                text = [TextSubstitution(text=item) for item in text]
-                args.extend(text)
+        result_args = []
+        arg = []
+
+        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:
+                    # Sting with just spaces.
+                    # Appending args allow splitting two substitutions
+                    # separated by a space.
+                    # e.g.: `$(subst1 asd) $(subst2 bsd)` will be two separate arguments.
+                    _append_arg()
+                    continue
+                if sub.text[0].isspace():
+                    # Needed for splitting from the previous argument
+                    # e.g.: `$(find-exec bsd) asd`
+                    # It splits `asd` from the path of `bsd` executable.
+                    if len(arg) != 0:
+                        _append_arg()
+                arg.append(TextSubstitution(text=tokens[0]))
+                if len(tokens) > 1:
+                    # Needed to split the first argument when more than one token.
+                    # e.g. `$(find-pkg-prefix csd)/asd bsd`
+                    # will split `$(find-pkg-prefix csd)/asd` from `bsd`.
+                    _append_arg()
+                    arg.append(TextSubstitution(text=tokens[-1]))
+                if len(tokens) > 2:
+                    # If there are more than two tokens, just add all the middle tokens to
+                    # `result_args`.
+                    # e.g. `$(find-pkg-prefix csd)/asd bsd dsd xsd`
+                    # 'bsd' 'dsd' will be added.
+                    result_args.extend([TextSubstitution(text=x)] for x in tokens[1:-1])
+                if sub.text[-1].isspace():
+                    # Allows splitting from next argument.
+                    # e.g. `exec $(find-some-file)`
+                    # Will split `exec` argument from the result of `find-some-file` substitution.
+                    _append_arg()
             else:
-                args.append(arg)
-        return args
+                arg.append(sub)
+        if arg:
+            result_args.append(arg)
+        return result_args
 
     @classmethod
     def parse(

launch/test/launch/frontend/test_substitutions.py

@@ -14,12 +14,17 @@
 
 """Test the default substitution interpolator."""
 
+from typing import List
+from typing import Text
+
 from launch import LaunchContext
+from launch import SomeSubstitutionsType
+from launch import Substitution
+from launch.actions import ExecuteProcess
 from launch.frontend.expose import expose_substitution
 from launch.frontend.parse_substitution import parse_substitution
 from launch.substitutions import EnvironmentVariable
 from launch.substitutions import PythonExpression
-from launch.substitutions import TextSubstitution
 from launch.substitutions import ThisLaunchFileDir
 
 
@@ -47,13 +52,26 @@ def test_text_only():
     assert subst[0].perform(None) == '10e4'
 
 
+def perform_substitutions_without_context(subs: List[Substitution]):
+    return ''.join([sub.perform(None) for sub in subs])
+
+
+class CustomSubstitution(Substitution):
+
+    def __init__(self, text):
+        self.__text = text
+
+    def perform(self, context):
+        return self.__text
+
+
 @expose_substitution('test')
 def parse_test_substitution(data):
     if not data or len(data) > 1:
         raise RuntimeError()
     kwargs = {}
-    kwargs['text'] = ''.join([i.perform(None) for i in data[0]])
-    return TextSubstitution, kwargs
+    kwargs['text'] = perform_substitutions_without_context(data[0])
+    return CustomSubstitution, kwargs
 
 
 def test_text_with_embedded_substitutions():
@@ -150,19 +168,19 @@ def test_env_subst():
     assert len(subst) == 1
     env = subst[0]
     assert isinstance(env, EnvironmentVariable)
-    assert 'asd' == ''.join([x.perform(None) for x in env.name])
-    assert 'bsd' == ''.join([x.perform(None) for x in env.default_value])
+    assert 'asd' == perform_substitutions_without_context(env.name)
+    assert 'bsd' == perform_substitutions_without_context(env.default_value)
     subst = parse_substitution("$(env asd '')")
     assert len(subst) == 1
     env = subst[0]
     assert isinstance(env, EnvironmentVariable)
-    assert 'asd' == ''.join([x.perform(None) for x in env.name])
-    assert '' == ''.join([x.perform(None) for x in env.default_value])
+    assert 'asd' == perform_substitutions_without_context(env.name)
+    assert '' == perform_substitutions_without_context(env.default_value)
     subst = parse_substitution('$(env asd)')
     assert len(subst) == 1
     env = subst[0]
     assert isinstance(env, EnvironmentVariable)
-    assert 'asd' == ''.join([x.perform(None) for x in env.name])
+    assert 'asd' == perform_substitutions_without_context(env.name)
     assert env.default_value is None
 
 
@@ -172,3 +190,58 @@ def test_eval_subst():
     expr = subst[0]
     assert isinstance(expr, PythonExpression)
     assert 'asdbsd' == expr.perform(LaunchContext())
+
+
+def expand_cmd_subs(cmd_subs: List[SomeSubstitutionsType]):
+    return [perform_substitutions_without_context(x) for x in cmd_subs]
+
+
+class MockParser:
+
+    def parse_substitution(self, value: Text) -> SomeSubstitutionsType:
+        return parse_substitution(value)
+
+
+def test_execute_process_parse_cmd_line():
+    """Test ExecuteProcess._parse_cmd_line."""
+    parser = MockParser()
+
+    cmd_text: Text = '$(test path)/a/b/c asd csd $(test asd)/bsd/csd'
+    cmd_subs: List[SomeSubstitutionsType] = ExecuteProcess._parse_cmdline(cmd_text, parser)
+    cmd_performed: List[Text] = expand_cmd_subs(cmd_subs)
+    assert cmd_performed == ['path/a/b/c', 'asd', 'csd', 'asd/bsd/csd']
+
+    cmd_text = '$(test exec) asd $(test bsd) csd'
+    cmd_subs = ExecuteProcess._parse_cmdline(cmd_text, parser)
+    cmd_performed = expand_cmd_subs(cmd_subs)
+    assert cmd_performed == ['exec', 'asd', 'bsd', 'csd']
+
+    cmd_text = '$(test exec) prefix/$(test bsd)'
+    cmd_subs = ExecuteProcess._parse_cmdline(cmd_text, parser)
+    cmd_performed = expand_cmd_subs(cmd_subs)
+    assert cmd_performed == ['exec', 'prefix/bsd']
+
+    cmd_text = '$(test exec) prefix/$(test bsd) '
+    cmd_subs = ExecuteProcess._parse_cmdline(cmd_text, parser)
+    cmd_performed = expand_cmd_subs(cmd_subs)
+    assert cmd_performed == ['exec', 'prefix/bsd']
+
+    cmd_text = '$(test exec) asd prefix/$(test bsd) '
+    cmd_subs = ExecuteProcess._parse_cmdline(cmd_text, parser)
+    cmd_performed = expand_cmd_subs(cmd_subs)
+    assert cmd_performed == ['exec', 'asd', 'prefix/bsd']
+
+    cmd_text = 'exec asd prefix/bsd '
+    cmd_subs = ExecuteProcess._parse_cmdline(cmd_text, parser)
+    cmd_performed = expand_cmd_subs(cmd_subs)
+    assert cmd_performed == ['exec', 'asd', 'prefix/bsd']
+
+    cmd_text = '$(test foo)$(test bar)'
+    cmd_subs = ExecuteProcess._parse_cmdline(cmd_text, parser)
+    cmd_performed = expand_cmd_subs(cmd_subs)
+    assert cmd_performed == ['foobar']
+
+    cmd_text = '$(test that) $(test this)'
+    cmd_subs = ExecuteProcess._parse_cmdline(cmd_text, parser)
+    cmd_performed = expand_cmd_subs(cmd_subs)
+    assert cmd_performed == ['that', 'this']