eval substitution errors

[Thodoris1999]

Bug report

Required Info:

• Operating System:
  Ubuntu 20.04
• Installation type:
  Binaries
• Version or commit hash:
  foxy latest release
• DDS implementation:
  Fast-RTPS
• Client library (if applicable):
  N/A

Steps to reproduce issue

Create and launch a xml launch file like this

<launch>
  <let name="variable" value="val1"/>
  <node pkg="demo_nodes_cpp" exec="talker if="$(eval variable == 'val1')" output="screen" />
</launch>

Expected behavior

talker starts running

Actual behavior

I get this error: TypeError: eval substitution expects 1 argument

Additional information

It seems that spaces confuse the eval substitution. Even if I remove the spaces, launch fails with

launch.invalid_launch_file_error.InvalidLaunchFileError: Caught exception when trying to load file of format [xml]: No terminal defined for ''' at line 1 col 20

$(eval variable=='val1')
                 ^

Expecting: {'UNQUOTED_RSTRING', 'SPACE', 'RPAR', 'DOLLAR'}

which I guess means that it doesn't like the single quote character

[hidmic]

I get this error: TypeError: eval substitution expects 1 argument

That's because the Python expression substitution expects a single argument. Making the argument a single string and escaping single quotes accordingly should work:

<launch>
  <let name="variable" value="val1"/>
  <node pkg="demo_nodes_cpp" exec="talker if="$(eval 'variable == \'val1\'')" output="screen" />
</launch>

You may also use double quotes via XML escaping, but it's a bit clunky (i.e. ").

if I remove the spaces, launch fails with

Hmm, that looks like a bug to me.

[Thodoris1999]

I get this error: TypeError: eval substitution expects 1 argument

That's because the Python expression substitution expects a single argument. Making the argument a single string and escaping single quotes accordingly should work:

If this is the designed behavior, this is not compatible with ROS1 and I suggest that it would be documented somewhere. In the tutorials it explicitly says that stayed the same, so many (myself included) might believe that these substitutions can be copy-pasted to ROS2 launch files. The arguments of each substitution could be added here https://design.ros2.org/articles/roslaunch_xml.html.

[hidmic]

If this is the designed behavior, this is not compatible with ROS1 and I suggest that it would be documented somewhere.

I agree with that statement i.e. to document the discrepancy with ROS 1.

The main reason behind this departure is to ensure 1-to-1 mappings between launch entities' descriptions in different (markup or programming) languages, which keeps them unsurprising and precludes the need for launch documentation on a per language basis.

Having said that, there's no technical limitation here. If folks are willing to make a special case for eval, I won't oppose (strongly, mildly perhaps). CC @wjwwood @ivanpauno for feedback.

[Thodoris1999]

That makes sense, if it is for consistency, I don't mind the extra escaping

[ivanpauno]

The main reason behind this departure is to ensure 1-to-1 mappings
If folks are willing to make a special case for eval

I think that it makes sense to allow to substitutions that can only take one argument to not use quotes to escape strings with spaces, in the case of eval it can be really annoying and hard to get correctly.

If we use that as a rule for all substitutions that take one argument, it won't be an special case 😉.

[hidmic]

If we use that as a rule for all substitutions that take one argument, it won't be an special case

I thought about it. It works great for command and eval, less so for find-exec, and for anon I'd bet it's a typo. But I agree escaping is annoying and often hard to get right. I'm not super eager to add more rules, but I'm onboard as long as we're consistent.

[wjwwood]

I like the idea of adding an exception (or rule, whatever you want to call it) to allows us to avoid escaping. Because it gets us closer to ROS 1 behavior, helping migrators out, and because it's just more convenient. But I don't feel strongly about it, and I'm not the one doing the work (AFAIK :D), so it's up to you guys.

[ivanpauno]

Marking this as help wanted, @hidmic @wjwwood feel free to undo it if it doesn't seem appropriate.

@Thodoris1999 if you want to address the issue, it involves modifying the parse method here to handle any number of arguments, instead of one, and joining all of them.

If you open a PR or need some help, feel free to tag me.