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 :wink:.

[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.

[ivanpauno]

+enhacement -bug

[Thodoris1999]

Looks straightforward, however I do need to compile ROS source and probably check out the developer guidelines and I'm a bit pressured on time lately, so in the meantime if anyone wants to take this feel free.

What about launch.invalid_launch_file_error.InvalidLaunchFileError? Should a separate issue be created?

[ivanpauno]

What about launch.invalid_launch_file_error.InvalidLaunchFileError? Should a separate issue be created?

Good question, no need to open another issue about that one. I'm not sure if we can make that case more flexible, @hidmic do you think that would be possible?

[hidmic]

I'm not sure if we can make that case more flexible, @hidmic do you think that would be possible?

I'd think so, yeah. Though I haven't read that grammar in a good while.

[mhl787156]

I have a related error where the evaluation cannot recognize the arg. Unsure if I should open a new issue as it is related to this one.

Steps to reproduce Create and launch a xml launch file like this

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

Expected behavior talker starts running

Actual Behaviour NameError: name 'variable' is not defined

I am running within the latest foxy base docker image. I would really appreciate pointers on what I might be going wrong!

[hidmic]

@mhl787156 just like in ROS1, eval substitutions do not use launch context as their scope. You have to nest substitutions like $(eval '$(val variable) == \'value\'').

As a side note, @mhl787156 please consider opening a separate ticket for seemingly related issues. Moreover, questions like this are better suited for answers.ros.org.

[mhl787156]

@hidmic apologies will do next time, it seemed related is all, I couldn't identify this solution clearly through any of the documentation! Many thanks for your answer nevertheless.

[Khush-dev]

Hey @ivanpauno, I would like to work on this issue. Can you help me out? I took a look at the file you mentioned and came up with some code https://github.com/ros2/launch/blob/d648e19590d425e5d38411fe83fdb3b59f56d586/launch/launch/substitutions/python_expression.py#L52-L56

if len(data) != 1:
      data_str = '\'' + '\''.join([element[0].text for element in data]) + '\''
      data = [[TextSubstitution(text = data_str)]]

The above seems to work for some cases(like 'a' + 'a' gets evaluated to 'aa' without the need of escaping) but I believe we need to resolve the issue at some stage before. Hopefully, that will also help with the launch.invalid_launch_file_error.InvalidLaunchFileError

Where should I look?

Edit: I think the grammar used in Lark must be changed, maybe even the parse_substitution.py file. But I don't know what exactly to change One idea is to add another Token that allows everything till the last unpaired ')'. Then we can set ambiguity to 'explicit' here and use this token for substitutions like eval and command Is this a feasible idea? Also, this is better than above incorrect code.

data = [[element[0] for element in data]]

[ivanpauno]

I think the grammar used in Lark must be changed

I'm not sure there's a way to change the grammar to also accept $(eval variable=='val1'), without making it ambiguous. It's easier to make the code accept $(eval variable==\'val1\') or $(eval variable == 'val1'), as you don't need to change the grammar in those cases.

[kenji-miyake]

Hi, I sent a draft PR to resolve this issue. #600 Since $(eval $(var value) == 'abc' is enough for my use case, I supported only the case for now.

Could you review it, please? :pray: