ros2 / launch

Tools for launching multiple processes and for writing tests involving multiple processes.
Apache License 2.0
126 stars 142 forks source link

If the string argument contains a colon, the launch process will die. #508

Closed hayato-m126 closed 3 years ago

hayato-m126 commented 3 years ago

Bug report

I have placed a demo reository at the following link

https://github.com/hayato-m126/launch_arg_sample

Required Info:

Steps to reproduce issue

  1. Please build the package below. https://github.com/hayato-m126/launch_arg_sample

  2. ros2 launch launch_arg_sample sample.launch.py compare_str:="Hello World:"

Expected behavior

Keep working if string argument include colon.

Actual behavior

The launch process dies.

christophebedard commented 3 years ago

I was able to reproduce this on Foxy and Rolling (binary installations). Here's the traceback:

("Allowed value types are bytes, bool, int, float, str, Sequence[bool], Sequence[int], Sequence[float], Sequence[str]. Got <class 'dict'>.")>
Traceback (most recent call last):
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch/launch_service.py", line 276, in _process_one_event
    await self.__process_event(next_event)
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch/launch_service.py", line 296, in __process_event
    visit_all_entities_and_collect_futures(entity, self.__context))
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch/utilities/visit_all_entities_and_collect_futures_impl.py", line 45, in visit_all_entities_and_collect_futures
    futures_to_return += visit_all_entities_and_collect_futures(sub_entity, context)
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch/utilities/visit_all_entities_and_collect_futures_impl.py", line 45, in visit_all_entities_and_collect_futures
    futures_to_return += visit_all_entities_and_collect_futures(sub_entity, context)
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch/utilities/visit_all_entities_and_collect_futures_impl.py", line 45, in visit_all_entities_and_collect_futures
    futures_to_return += visit_all_entities_and_collect_futures(sub_entity, context)
  [Previous line repeated 1 more time]
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch/utilities/visit_all_entities_and_collect_futures_impl.py", line 38, in visit_all_entities_and_collect_futures
    sub_entities = entity.visit(context)
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch/action.py", line 108, in visit
    return self.execute(context)
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch_ros/actions/node.py", line 417, in execute
    self._perform_substitutions(context)
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch_ros/actions/node.py", line 377, in _perform_substitutions
    evaluated_parameters = evaluate_parameters(context, self.__parameters)
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch_ros/utilities/evaluate_parameters.py", line 145, in evaluate_parameters
    output_params.append(evaluate_parameter_dict(context, param))
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch_ros/utilities/evaluate_parameters.py", line 85, in evaluate_parameter_dict
    raise TypeError(
TypeError: Allowed value types are bytes, bool, int, float, str, Sequence[bool], Sequence[int], Sequence[float], Sequence[str]. Got <class 'dict'>.

so it's raising here: https://github.com/ros2/launch_ros/blob/8f7bfc5f7d2320c3c2003afa101683df42d6bda3/launch_ros/launch_ros/utilities/evaluate_parameters.py#L85

and on Rolling: https://github.com/ros2/launch_ros/blob/04b3cd3ec150f3a2b8d44cd54e3d421cac203666/launch_ros/launch_ros/utilities/evaluate_parameters.py#L88

It seems to be due to the way the arguments are parsed: they're parsed using YAML, so abc: is parsed as a dict (see the TypeError message).

If you use single quotes (which is something you would do in a YAML file to get around this "behaviour"), it works, e.g. 'abc:' is parsed as a string.

$ ros2 launch launch_arg_sample sample.launch.py "compare_str:='Hello World:'"
$ ros2 launch launch_arg_sample sample.launch.py compare_str:="'Hello World:'"

Both of those work, on Rolling and Foxy.

I'm not sure if this is considered a bug, but I couldn't (easily) find documentation about this YAML parsing detail/behaviour.

christophebedard commented 3 years ago

but I couldn't (easily) find documentation about this YAML parsing detail/behaviour.

I found this: https://design.ros2.org/articles/ros_command_line_arguments.html#single-parameter-assignments

This option takes a single name:=value assignment statement, where value is in YAML format and thus YAML type inference rules apply.

hayato-m126 commented 3 years ago

I have confirmed that enclosing the argument in single-quotation works correctly. I read the above comments and understood the argument parsing behaviour.

Thank you.

DLu commented 3 years ago

I have just encountered this issue as well, and wish to reopen the ticket.

In particular, the problem remains if the parameter value is a seemingly valid xacro that happens to have a colon in a comment somewhere. https://github.com/hello-robot/stretch_ros2/pull/2

I think somewhere something is not being escaped properly, possibly when the Command is returned as a string.

hidmic commented 3 years ago

I think somewhere something is not being escaped properly, possibly when the Command is returned as a string.

Thing is, you may not always want to treat a Command's output as a str. I'd be onboard with allowing the user to explicitly enable (or disable) this though. Would you be willing to contribute such a change?

DLu commented 3 years ago

I made a test here: https://github.com/ros2/launch_ros/pull/268

I'd be willing to contribute something, but I'm not sure what the best approach is. Is maybe the better fix to treat the parameter as a string if if fails to parse as a yaml?