[launch_frontend] FindPackage substitution not concatenated with surrounding text

[jacobperron]

Bug report

Required Info:

• Operating System:
  • Ubuntu 18.04
• Installation type:
  • source
• Version or commit hash:
  • 897cae8f6bf5602e250f9e521497b9eb70e14a85
• DDS implementation:
  • N/A
• Client library (if applicable):
  • N/A

Steps to reproduce issue

Create a launch file with the following content and launch it.

<launch>
  <executable cmd="echo $(find-pkg-share rclcpp)/sub/path/foo" output="screen"/>
</launch>

Expected behavior

I would expect to see output like the following:

[echo-2] /home/jacob/ros2_ws/install/rclcpp/share/rclcpp/sub/path/foo

Actual behavior

The substituted string for the share directory and the subsequent text, /sub/path/foo, are separated with a space:

[echo-2] /home/jacob/ros2_ws/install/rclcpp/share/rclcpp /sub/path/foo

Additional information

There's the same behavior with find-pkg-prefix as well.

I've noticed that if we do the substitution in an <arg> tag, then it works as expected.
I'm not sure if tags other than <executable> are exhibiting the same bug.

Workaround

First do the substitution in an <arg> tag, for example:

<launch>
  <arg name="rclcpp_sub_dir" default="$(find-pkg-share rclcpp)/sub/path/foo" />
  <executable cmd="echo $(var rclcpp_sub_dir)" output="screen"/>
</launch>

---

I'm not sure where to look to resolve this issue. It's possible it should be fixed in launch (not launch_ros).
Maybe someone can point me in the right direction.

[ivanpauno]

I couldn't reproduce the error.
I'm updating my repos two double check.

@jacobperron Can you confirm if you still can reproduce the error? Thanks!

[jacobperron]

Still happens for me, here's what the launch output is for me:

[INFO] [launch]: All log files can be found below /home/jacob/.ros/log/2019-09-19-13-10-17-642777-warner-8629
[INFO] [launch]: Default logging verbosity is set to INFO
[INFO] [echo-1]: process started with pid [8645]
[echo-1] /home/jacob/ws/latest_ws/install/rclcpp/share/rclcpp /sub/path/foo
[INFO] [echo-1]: process has finished cleanly [pid 8645]

versus what I expect it to be:

[INFO] [launch]: All log files can be found below /home/jacob/.ros/log/2019-09-19-13-10-17-642777-warner-8629
[INFO] [launch]: Default logging verbosity is set to INFO
[INFO] [echo-1]: process started with pid [8645]
[echo-1] /home/jacob/ws/latest_ws/install/rclcpp/share/rclcpp/sub/path/foo
[INFO] [echo-1]: process has finished cleanly [pid 8645]

It's a subtle difference:

[INFO] [launch]: All log files can be found below /home/jacob/.ros/log/2019-09-19-13-10-17-642777-warner-8629
[INFO] [launch]: Default logging verbosity is set to INFO
[INFO] [echo-1]: process started with pid [8645]
- [echo-1] /home/jacob/ws/latest_ws/install/rclcpp/share/rclcpp /sub/path/foo
+ [echo-1] /home/jacob/ws/latest_ws/install/rclcpp/share/rclcpp/sub/path/foo
[INFO] [echo-1]: process has finished cleanly [pid 8645]

[ivanpauno]

@jacobperron this is quite strange. I have updated and rebuilt and I still cannot reproduce it.
I'm testing your example and also this other:

from launch_ros.substitutions import FindPackageShare
from launch import LaunchContext

subst = FindPackageShare('rclcpp')
print(subst.perform(LaunchContext()) + '/asd')

Can you test it?
I will also open a PR adding a test and check what CI says.

[ivanpauno]

@jacobperron could you take a look to ros2/launch_ros#76?
This seems to be working ok both locally to me and in CI.

[jacobperron]

I'm surprised you can't reproduce. It also happens with other substitutions, e.g. eval:

<launch>
  <executable cmd="echo $(eval 'str(\'hello_world\')')/sub/path/foo" output="screen"/>
</launch>

I've starting digging into the code, and it looks like the issue stems from interaction between the parser and ExecuteProcess. Adding prints before and after this line I see that the command

"echo $(find-pkg rclcpp)/foo/bar"

is broken into a list of three substitutions

[TextSubstitution('echo'), FindPackageShare('rclcpp'), TextSubstitution('/foo/bar')]

Then, the command list is naturally separated by spaces, so we end up with

"echo /path/to/rclcpp /foo/bar"

instead of the desired

"echo /path/to/rclcpp/foo/bar"

I believe the issue needs to be resolved in the parser, since there doesn't seem to be a way to distinguish the following two cases in ExecuteProcess:

"echo $(find-pkg rclcpp)/foo/bar"

versus

"echo $(find-pkg rclcpp) /foo/bar"

[jacobperron]

What do you think about changing the grammar to keep together any text containing a substitution? See this commit that adds proposed tests:

def test_space_next_to_substitutions():
    # substitutions should concatenate with surrounding text that are not whitespace
    subst = parse_substitution('$(test foo)_bar_ _bar_$(test baz)')
    assert len(subst) == 2
    assert subst[0].perform(None) == 'foo_bar_'
    assert subst[1].perform(None) == '_bar_baz'
    subst = parse_substitution('$(test foo) _bar_ _bar_ $(test baz)')
    assert len(subst) == 4
    assert subst[0].perform(None) == 'foo'
    assert subst[0].perform(None) == '_bar_'
    assert subst[0].perform(None) == '_bar_'
    assert subst[1].perform(None) == 'baz'

I guess we'd have adapt substitutions to also account for neighboring text, or introduce a new substitution type that handles this case. Thoughts?

[ivanpauno]

@jacobperron wow I read this bad twice, sorry.
Addressing this issue will help to solve the problem: #263

[ivanpauno]

Transferring to https://github.com/ros2/launch.

[ruffsl]

Has this moved since sep? Is there branch with some WIP related to this issue? This is still effecting launch users, limiting the use of substitutions.

[ivanpauno]

@ruffsl Yes, I agree. I will try to iterate on this ASAP.