Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Execute process refactor #215

Open
wants to merge 19 commits into
base: rolling
Choose a base branch
from
Open
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Improve node name handling, unit test fixes, more
Distro A; OPSEC #4584

Signed-off-by: Roger Strain <[email protected]>
Roger Strain authored and vknisley-swri committed Dec 15, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit 87948a8ff05e72465d7eb263cb92151f8d0f95dc
9 changes: 5 additions & 4 deletions launch_ros/launch_ros/actions/node.py
Original file line number Diff line number Diff line change
@@ -183,10 +183,6 @@ def __init__(
self.__extensions = get_extensions(self.__logger)
super().__init__(process_description=self.__ros_exec, **kwargs)

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()

@@ -316,6 +312,11 @@ def node_name(self):
"""Getter for node_name."""
return self.__node_desc.node_name

@property
def ros_exec(self):
"""Getter for ros_exec."""
return self.__ros_exec

@property
def expanded_node_namespace(self):
"""Getter for expanded_node_namespace."""
34 changes: 26 additions & 8 deletions launch_ros/launch_ros/descriptions/node.py
Original file line number Diff line number Diff line change
@@ -74,17 +74,14 @@ def __init__(
self, *,
node_name: Optional[SomeSubstitutionsType] = None,
node_namespace: Optional[SomeSubstitutionsType] = None,
original_node_name: Optional[SomeSubstitutionsType] = None,
parameters: Optional[SomeParameters] = None,
remappings: Optional[SomeRemapRules] = None,
traits: Optional[Iterable[NodeTrait]] = None,
**kwargs
) -> None:
"""
Construct an Node description.

Many arguments are passed eventually to
:class:`launch.actions.ExecuteProcess`, so see the documentation of
that class for additional details.
Construct a Node description.

If the node name is not given (or is None) then no name is passed to
the node on creation and instead the default name specified within the
@@ -117,6 +114,9 @@ def __init__(

:param: node_name the name of the node
:param: node_namespace the ROS namespace for this Node
:param: original_node_name the name of the node before remapping; if not specified,
remappings/parameters (including node name/namespace changes) may be applied
to all nodes which share a command line executable with this one
:param: parameters list of names of yaml files with parameter rules,
or dictionaries of parameters.
:param: remappings ordered list of 'to' and 'from' string pairs to be
@@ -131,12 +131,14 @@ def __init__(

self.__node_name = node_name
self.__node_namespace = node_namespace
self.__original_node_name = original_node_name
self.__parameters = [] if parameters is None else normalized_params
self.__remappings = [] if remappings is None else list(normalize_remap_rules(remappings))
self.__traits = traits

self.__expanded_node_name = self.UNSPECIFIED_NODE_NAME
self.__expanded_node_namespace = self.UNSPECIFIED_NODE_NAMESPACE
self.__expanded_original_node_name = self.UNSPECIFIED_NODE_NAME
self.__expanded_parameter_arguments = None # type: Optional[List[Tuple[Text, bool]]]
self.__final_node_name = None # type: Optional[Text]
self.__expanded_remappings = None # type: Optional[List[Tuple[Text, Text]]]
@@ -157,6 +159,11 @@ def node_namespace(self):
"""Getter for node_namespace."""
return self.__node_namespace

@property
def original_node_name(self):
"""Getter for original_node_name."""
return self.__original_node_name

@property
def parameters(self):
"""Getter for parameters."""
@@ -245,6 +252,10 @@ def _perform_substitutions(self, context: LaunchContext, cmd: List) -> None:
cmd_ext = ['-r', LocalSubstitution("ros_specific_arguments['ns']")]
cmd.extend([normalize_to_list_of_substitutions(x) for x in cmd_ext])
validate_namespace(self.__expanded_node_namespace)
if self.__original_node_name is not None:
self.__expanded_original_node_name = perform_substitutions(
context, normalize_to_list_of_substitutions(self.__original_node_name))
self.__expanded_node_name.lstrip('/')
except Exception:
self.__logger.error(
"Error while expanding or validating node name or namespace for '{}':"
@@ -310,10 +321,17 @@ def _perform_substitutions(self, context: LaunchContext, cmd: List) -> None:
# 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]]] = {}
original_name_prefix = ""
if self.__expanded_original_node_name is not self.UNSPECIFIED_NODE_NAME:
original_name_prefix = '{}:'.format(self.__expanded_original_node_name)
if self.__node_name is not None:
ros_specific_arguments['name'] = '__node:={}'.format(self.__expanded_node_name)
ros_specific_arguments['name'] = '{}__node:={}'.format(
original_name_prefix, self.__expanded_node_name
)
if self.__expanded_node_namespace != '':
ros_specific_arguments['ns'] = '__ns:={}'.format(self.__expanded_node_namespace)
ros_specific_arguments['ns'] = '{}__ns:={}'.format(
original_name_prefix, self.__expanded_node_namespace
)
context.extend_locals({'ros_specific_arguments': ros_specific_arguments})

if self.is_node_name_fully_specified():
@@ -324,4 +342,4 @@ def _perform_substitutions(self, context: LaunchContext, cmd: List) -> 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)
)
)
1 change: 1 addition & 0 deletions launch_ros/launch_ros/descriptions/node_trait.py
Original file line number Diff line number Diff line change
@@ -47,3 +47,4 @@ def __init__(self) -> None:

def prepare(self, node: 'Node', context: LaunchContext, action: Action):
"""Perform any actions necessary to prepare the node for execution."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roger-strain missing pass after docstring?

pass
Original file line number Diff line number Diff line change
@@ -51,5 +51,6 @@ def test_node_name():
namespace='my_ns',
)
lc = LaunchContext()
node_object._Node__node_desc._perform_substitutions(lc, [])
for node in node_object.ros_exec.nodes:
node._perform_substitutions(lc, [])
assert node_object.is_node_name_fully_specified() is True
15 changes: 10 additions & 5 deletions test_launch_ros/test/test_launch_ros/actions/test_node.py
Original file line number Diff line number Diff line change
@@ -88,7 +88,8 @@ def test_launch_node_with_remappings(self):
self._assert_launch_no_errors([node_action])

# Check the expanded parameters.
expanded_remappings = node_action._Node__node_desc.expanded_remappings
for node in node_action.ros_exec.nodes:
expanded_remappings = node.expanded_remappings
assert len(expanded_remappings) == 2
for i in range(2):
assert expanded_remappings[i] == ('chatter', 'new_chatter')
@@ -147,7 +148,8 @@ def test_launch_node_with_parameter_files(self):
self._assert_launch_no_errors([node_action])

# Check the expanded parameters.
expanded_parameter_arguments = node_action._Node__node_desc.expanded_parameter_arguments
for node in node_action.ros_exec.nodes:
expanded_parameter_arguments = node.expanded_parameter_arguments
assert len(expanded_parameter_arguments) == 3
for i in range(3):
assert expanded_parameter_arguments[i] == (str(parameters_file_path), True)
@@ -184,7 +186,8 @@ def test_launch_node_with_parameter_descriptions(self):
)
self._assert_launch_no_errors([node_action])

expanded_parameter_arguments = node_action._Node__node_desc.expanded_parameter_arguments
for node in node_action.ros_exec.nodes:
expanded_parameter_arguments = node.expanded_parameter_arguments
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mlanting hmm, should these be extending the expanded_parameter_arguments list instead of overwriting? Same below.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that would probably be more appropriate. I'll make the change.

assert len(expanded_parameter_arguments) == 5
parameters = []
for item, is_file in expanded_parameter_arguments:
@@ -221,7 +224,8 @@ def test_launch_node_with_parameter_dict(self):
self._assert_launch_no_errors([node_action])

# Check the expanded parameters (will be written to a file).
expanded_parameter_arguments = node_action._Node__node_desc.expanded_parameter_arguments
for node in node_action.ros_exec.nodes:
expanded_parameter_arguments = node.expanded_parameter_arguments
assert len(expanded_parameter_arguments) == 1
file_path, is_file = expanded_parameter_arguments[0]
assert is_file
@@ -356,5 +360,6 @@ def get_test_node_name_parameters():
)
def test_node_name(node_object, expected_result):
lc = LaunchContext()
node_object._Node__node_desc._perform_substitutions(lc, [])
for node in node_object.ros_exec.nodes:
node._perform_substitutions(lc, [])
assert node_object.is_node_name_fully_specified() is expected_result
Original file line number Diff line number Diff line change
@@ -127,7 +127,8 @@ def test_push_ros_namespace(config):
namespace=config.node_ns,
name=config.node_name
)
node._Node__node_desc._perform_substitutions(lc, [])
for node in node_object.ros_exec.nodes:
node._perform_substitutions(lc, [])
expected_ns = (
config.expected_ns if config.expected_ns is not None else
NodeDescription.UNSPECIFIED_NODE_NAMESPACE
Original file line number Diff line number Diff line change
@@ -119,7 +119,8 @@ def test_set_param_with_node():
)
set_param = SetParameter(name='my_param', value='my_value')
set_param.execute(lc)
node._perform_substitutions(lc)
for node_instance in node.ros_exec.nodes:
node_instance._perform_substitutions(lc, [])
actual_command = [perform_substitutions(lc, item) for item in
node.cmd if type(item[0]) == TextSubstitution]
assert actual_command.count('--params-file') == 1
Original file line number Diff line number Diff line change
@@ -93,7 +93,8 @@ def test_set_remap_with_node():
)
set_remap = SetRemap('from1', 'to1')
set_remap.execute(lc)
node._Node__node_desc._perform_substitutions(lc, [])
for node_instance in node.ros_exec.nodes:
node_instance._perform_substitutions(lc, [])
assert len(node.expanded_remapping_rules) == 2
assert node.expanded_remapping_rules == [('from1', 'to1'), ('from2', 'to2')]