Refactor arguments from nodes, address feedback

Distro A; OPSEC #4584

Signed-off-by: Roger Strain <[email protected]>

launch_ros/launch_ros/actions/lifecycle_node.py

@@ -142,7 +142,7 @@ def execute(self, context: launch.LaunchContext) -> Optional[List[Action]]:
 
         Delegated to :meth:`launch.actions.ExecuteProcess.execute`.
         """
-        self._perform_substitutions(context)  # ensure self.node_name is expanded
+        self.prepare(context)  # ensure self.node_name is expanded
         if '<node_name_unspecified>' in self.node_name:
             raise RuntimeError('node_name unexpectedly incomplete for lifecycle node')
         node = get_ros_node(context)

launch_ros/launch_ros/actions/node.py

@@ -14,6 +14,7 @@
 
 """Module for the Node action."""
 
+from typing import Dict
 from typing import Iterable
 from typing import List
 from typing import Optional
@@ -51,6 +52,7 @@ def __init__(
         parameters: Optional[SomeParameters] = None,
         remappings: Optional[SomeRemapRules] = None,
         arguments: Optional[Iterable[SomeSubstitutionsType]] = None,
+        additional_env: Optional[Dict[SomeSubstitutionsType, SomeSubstitutionsType]] = None,
         **kwargs
     ) -> None:
         """
@@ -112,16 +114,20 @@ def __init__(
         :param: remappings ordered list of 'to' and 'from' string pairs to be
             passed to the node as ROS remapping rules
         :param: arguments list of extra arguments for the node
+        :param additional_env: Dictionary of environment variables to be added. If env was
+            None, they are added to the current environment. If not, env is updated with
+            additional_env.
         """
         self.__node_desc = NodeDescription(node_name=name, node_namespace=namespace,
-                                           parameters=parameters, remappings=remappings,
-                                           arguments=arguments)
+                                           parameters=parameters, remappings=remappings)
+        ros_exec_kwargs = {'additional_env': additional_env, 'arguments': arguments}
         self.__ros_exec = RosExecutable(package=package, executable=executable,
-                                        nodes=[self.__node_desc])
+                                        nodes=[self.__node_desc], **ros_exec_kwargs)
         super().__init__(process_description=self.__ros_exec, **kwargs)
 
-    def _perform_substitutions(self, lc: LaunchContext):
-        self.__node_desc.prepare(lc, self.__ros_exec)
+    def prepare(self, context: LaunchContext):
+        self.__node_desc.prepare(context, self.__ros_exec, self)
+        super().prepare(context)
 
     def is_node_name_fully_specified(self):
         return self.__node_desc.is_node_name_fully_specified()
@@ -193,6 +199,10 @@ def get_nested_dictionary_from_nested_key_value_pairs(params):
     def parse(cls, entity: Entity, parser: Parser):
         """Parse node."""
         # See parse method of `ExecuteProcess`
+        # Note: This class originally was a direct descendant of ExecuteProcess,
+        # but has been refactored to better divide the concept of a node vs an
+        # executable process. This class remains as a compatibility layer, and
+        # must hand off parsing duties to its original ancestor.
         _, kwargs = ExecuteProcess.parse(entity, parser, ignore=['cmd'])
         args = entity.get_attr('args', optional=True)
         if args is not None:

launch_ros/launch_ros/descriptions/node.py

@@ -35,6 +35,7 @@
 from typing import Tuple
 from typing import Union
 
+from launch import Action
 from launch import LaunchContext
 from launch import SomeSubstitutionsType
 from launch.descriptions import Executable
@@ -75,7 +76,6 @@ def __init__(
         node_namespace: Optional[SomeSubstitutionsType] = None,
         parameters: Optional[SomeParameters] = None,
         remappings: Optional[SomeRemapRules] = None,
-        arguments: Optional[Iterable[SomeSubstitutionsType]] = None,
         traits: Optional[Iterable[NodeTrait]] = None,
         **kwargs
     ) -> None:
@@ -121,7 +121,6 @@ def __init__(
             or dictionaries of parameters.
         :param: remappings ordered list of 'to' and 'from' string pairs to be
             passed to the node as ROS remapping rules
-        :param: arguments list of extra arguments for the node
         :param: traits list of special traits of the node
         """
         if parameters is not None:
@@ -134,7 +133,6 @@ def __init__(
         self.__node_namespace = node_namespace
         self.__parameters = [] if parameters is None else normalized_params
         self.__remappings = [] if remappings is None else list(normalize_remap_rules(remappings))
-        self.__arguments = arguments
         self.__traits = traits
 
         self.__expanded_node_name = self.UNSPECIFIED_NODE_NAME
@@ -169,11 +167,6 @@ def remappings(self):
         """Getter for remappings."""
         return self.__remappings
 
-    @property
-    def arguments(self):
-        """Getter for arguments."""
-        return self.__arguments
-
     @property
     def traits(self):
         """Getter for traits."""
@@ -217,14 +210,22 @@ def _get_parameter_rule(self, param: 'Parameter', context: LaunchContext):
         name, value = param.evaluate(context)
         return f'{name}:={yaml.dump(value)}'
 
-    def prepare(self, context: LaunchContext, executable: Executable) -> None:
+    def prepare(self, context: LaunchContext, executable: Executable, action: Action) -> None:
+        self._perform_substitutions(context, executable.cmd)
+
+        # Prepare any traits which may be defined for this node
+        if self.__traits is not None:
+            for trait in self.__traits:
+                trait.prepare(self, context, action)
+
+    def _perform_substitutions(self, context: LaunchContext, cmd: List) -> None:
         try:
             if self.__substitutions_performed:
                 # This function may have already been called by a subclass' `execute`, for example.
                 return
             self.__substitutions_performed = True
             cmd_ext = ['--ros-args']  # Prepend ros specific arguments with --ros-args flag
-            executable.cmd.extend([normalize_to_list_of_substitutions(x) for x in cmd_ext])
+            cmd.extend([normalize_to_list_of_substitutions(x) for x in cmd_ext])
             if self.__node_name is not None:
                 self.__expanded_node_name = perform_substitutions(
                     context, normalize_to_list_of_substitutions(self.__node_name))
@@ -240,7 +241,7 @@ def prepare(self, context: LaunchContext, executable: Executable) -> None:
             if expanded_node_namespace is not None:
                 self.__expanded_node_namespace = expanded_node_namespace
                 cmd_ext = ['-r', LocalSubstitution("ros_specific_arguments['ns']")]
-                executable.cmd.extend([normalize_to_list_of_substitutions(x) for x in cmd_ext])
+                cmd.extend([normalize_to_list_of_substitutions(x) for x in cmd_ext])
                 validate_namespace(self.__expanded_node_namespace)
         except Exception:
             self.__logger.error(
@@ -262,7 +263,7 @@ def prepare(self, context: LaunchContext, executable: Executable) -> None:
             param_file_path = self._create_params_file_from_dict(global_params)
             self.__expanded_parameter_arguments.append((param_file_path, True))
             cmd_ext = ['--params-file', f'{param_file_path}']
-            executable.cmd.extend([normalize_to_list_of_substitutions(x) for x in cmd_ext])
+            cmd.extend([normalize_to_list_of_substitutions(x) for x in cmd_ext])
             assert os.path.isfile(param_file_path)
         # expand parameters too
         if self.__parameters is not None:
@@ -287,7 +288,7 @@ def prepare(self, context: LaunchContext, executable: Executable) -> None:
                     continue
                 self.__expanded_parameter_arguments.append((param_argument, is_file))
                 cmd_ext = ['--params-file' if is_file else '-p', f'{param_argument}']
-                executable.cmd.extend([normalize_to_list_of_substitutions(x) for x in cmd_ext])
+                cmd.extend([normalize_to_list_of_substitutions(x) for x in cmd_ext])
         # expand remappings too
         global_remaps = context.launch_configurations.get('ros_remaps', None)
         if global_remaps or self.__remappings:
@@ -303,7 +304,7 @@ def prepare(self, context: LaunchContext, executable: Executable) -> None:
             cmd_ext = []
             for src, dst in self.__expanded_remappings:
                 cmd_ext.extend(['-r', f'{src}:={dst}'])
-            executable.cmd.extend([normalize_to_list_of_substitutions(x) for x in cmd_ext])
+            cmd.extend([normalize_to_list_of_substitutions(x) for x in cmd_ext])
         # Prepare the ros_specific_arguments list and add it to the context so that the
         # LocalSubstitution placeholders added to the the cmd can be expanded using the contents.
         ros_specific_arguments: Dict[str, Union[str, List[str]]] = {}
@@ -321,4 +322,4 @@ def prepare(self, context: LaunchContext, executable: Executable) -> None:
                 execute_process_logger.warning(
                     'there are now at least {} nodes with the name {} created within this '
                     'launch context'.format(node_name_count, self.node_name)
-                )
+                )

launch_ros/launch_ros/descriptions/node_trait.py

@@ -22,9 +22,14 @@
 
 """Module for a description of a NodeTrait."""
 
+from typing import TYPE_CHECKING
+
 from launch import Action
 from launch import LaunchContext
 
+if TYPE_CHECKING:
+    from . import Node
+
 
 class NodeTrait:
     """Describes a trait of a node."""
@@ -40,5 +45,5 @@ def __init__(self) -> None:
         """
         pass
 
-    def prepare(self, node, context: LaunchContext, action: Action):
+    def prepare(self, node: 'Node', context: LaunchContext, action: Action):
         """Perform any actions necessary to prepare the node for execution."""

launch_ros/launch_ros/descriptions/ros_executable.py

@@ -25,6 +25,7 @@
 from typing import Iterable
 from typing import Optional
 
+from launch import Action
 from launch import LaunchContext
 from launch import SomeSubstitutionsType
 from launch.descriptions import Executable
@@ -77,7 +78,7 @@ def nodes(self):
         """Getter for nodes."""
         return self.__nodes
 
-    def apply_context(self, context: LaunchContext):
+    def prepare(self, context: LaunchContext, action: Action):
         """
         Prepare a ROS executable description for execution in a given environment.
 
@@ -86,6 +87,6 @@ def apply_context(self, context: LaunchContext):
         - performs substitutions on various properties
         """
         for node in self.__nodes:
-            node.prepare(context, self)
+            node.prepare(context, self, action)
 
-        super().apply_context(context)
+        super().prepare(context, action)

test_launch_ros/test/test_launch_ros/actions/test_lifecycle_node.py

@@ -51,5 +51,5 @@ def test_node_name():
         namespace='my_ns',
     )
     lc = LaunchContext()
-    node_object._perform_substitutions(lc)
+    node_object._Node__node_desc._perform_substitutions(lc, [])
     assert node_object.is_node_name_fully_specified() is True

test_launch_ros/test/test_launch_ros/actions/test_node.py

@@ -348,5 +348,5 @@ def get_test_node_name_parameters():
 )
 def test_node_name(node_object, expected_result):
     lc = LaunchContext()
-    node_object._perform_substitutions(lc)
+    node_object._Node__node_desc._perform_substitutions(lc, [])
     assert node_object.is_node_name_fully_specified() is expected_result

test_launch_ros/test/test_launch_ros/actions/test_push_ros_namespace.py

@@ -127,7 +127,7 @@ def test_push_ros_namespace(config):
         namespace=config.node_ns,
         name=config.node_name
     )
-    node._perform_substitutions(lc)
+    node._Node__node_desc._perform_substitutions(lc, [])
     expected_ns = (
         config.expected_ns if config.expected_ns is not None else
         NodeDescription.UNSPECIFIED_NODE_NAMESPACE

test_launch_ros/test/test_launch_ros/actions/test_set_parameter.py

@@ -45,7 +45,7 @@ def extend_locals(self, val):
         pass
 
     def perform_substitution(self, sub):
-        return sub.perform(None)
+        return sub.perform(self)
 
 
 def get_set_parameter_test_parameters():
@@ -121,7 +121,7 @@ def test_set_param_with_node():
     )
     set_param = SetParameter(name='my_param', value='my_value')
     set_param.execute(lc)
-    node._perform_substitutions(lc)
+    node._Node__node_desc._perform_substitutions(lc, [])
     expanded_parameter_arguments = node._Node__node_desc.expanded_parameter_arguments
     assert len(expanded_parameter_arguments) == 2
     param_file_path, is_file = expanded_parameter_arguments[0]

test_launch_ros/test/test_launch_ros/actions/test_set_remap.py

@@ -93,7 +93,7 @@ def test_set_remap_with_node():
     )
     set_remap = SetRemap('from1', 'to1')
     set_remap.execute(lc)
-    node._perform_substitutions(lc)
+    node._Node__node_desc._perform_substitutions(lc, [])
     assert len(node.expanded_remapping_rules) == 2
     assert node.expanded_remapping_rules == [('from1', 'to1'), ('from2', 'to2')]
 

test_launch_ros/test/test_launch_ros/frontend/test_node_frontend.py

@@ -158,7 +158,7 @@ def check_launch_node(file):
     assert(0 == ls.run())
     evaluated_parameters = evaluate_parameters(
         ls.context,
-        ld.describe_sub_entities()[3]._Node__parameters
+        ld.describe_sub_entities()[3].process_description.nodes[0]._Node__parameters
     )
     assert len(evaluated_parameters) == 3
     assert isinstance(evaluated_parameters[0], dict)
@@ -199,7 +199,7 @@ def check_launch_node(file):
     assert param_dict['param_group1.param15'] == ['2', '5', '8']
 
     # Check remappings exist
-    remappings = ld.describe_sub_entities()[3]._Node__remappings
+    remappings = ld.describe_sub_entities()[3].process_description.nodes[0]._Node__remappings
     assert remappings is not None
     assert len(remappings) == 2