[roslaunch] Added condition to skip empty ('') <include> paths

[jliviero]

Our internal build system runs roslaunch-check against relevant packages, which will fail when it encounters <include file='' />. This change allows roslaunch-check to pass, but prints a warning when such an include is encountered.

This allows a file path to be passed in as an arg, and left empty by default, and still have roslaunch-check pass (but with a warning). For example:

<launch>
  <arg name="some_mesh_launch" default=""/>

  <group if="$(eval len(some_mesh_launch) > 0)">
    <include file="$(arg some_mesh_launch)"/>
  </group>

</launch>

[tfoote]

Can you explain your use case a little bit more?

And please consider adding test cases to make sure it behaves in the expected ways. From your example it looks like the modified code path will never be hit.

[jliviero]

Certainly. This covers a case where we want to configure a set of parameters in a Gazebo simulation at launch time. In our specific case, this constitutes a series of additions to the environment (which is why I chose "some_mesh" - the actual arg is arbitrary). However, we don't need or want these changes on every launch, and we want to be able to quickly swap between configurations of the additions (pose, etc.) to test different solutions.

edited to add: Our internal build system runs roslaunch-check against the relevant package, which will fail when it encounters <include file=''/>. This change allows roslaunch-check to pass, but prints a warning when such an include is encountered.

As-written, you are correct - the modified path would not be hit. However, by providing an arg in the CLI like so, it would:
roslaunch robot_sim arbitrary_environment.launch some_mesh_launch:=/path/to/config/launch
(edit: the above didn't address the actual code changes, which are relevant to roslaunch-check only - sorry for any confusion)

[dirk-thomas]

I just modified an existing launch file and added an optional arg, the group as well as the nested include as in your original comment. Without any modification in roslaunch I was able to run the launch file with as well as without the optional argument.

Can you please clarify your use case and describe why you need the modification in this PR to run the launch file with and without.

[jliviero]

Without any modification in roslaunch I was able to run the launch file with as well as without the optional argument.

Ah, I see where I haven't been clear, thank you. This allows rolaunch-check to pass; it does not affect the behaviour of roslaunch.

Without this change, a file with the modifications you've made based on my example will fail:

jliviero@CPR00612:~/ws/src$ rosrun roslaunch roslaunch-check -o - launch/custom_map.launch 
checking launch/custom_map.launch
ERROR: cannot determine package for []
Traceback (most recent call last):
  File "/home/jliviero/ws/src/ros_comm/tools/roslaunch/scripts/roslaunch-check", line 86, in <module>
    error_msg = check_roslaunch_file(roslaunch_path)
  File "/home/jliviero/ws/src/ros_comm/tools/roslaunch/scripts/roslaunch-check", line 46, in check_roslaunch_file
    error_msg = roslaunch.rlutil.check_roslaunch(roslaunch_file)
  File "/home/jliviero/ws/src/ros_comm/tools/roslaunch/src/roslaunch/rlutil.py", line 200, in check_roslaunch
    base_pkg, file_deps, missing = roslaunch.depends.roslaunch_deps([f])
  File "/home/jliviero/ws/src/ros_comm/tools/roslaunch/src/roslaunch/depends.py", line 323, in roslaunch_deps
    rl_file_deps(file_deps, launch_file, verbose)
  File "/home/jliviero/ws/src/ros_comm/tools/roslaunch/src/roslaunch/depends.py", line 224, in rl_file_deps
    parse_launch(launch_file, file_deps, verbose)
  File "/home/jliviero/ws/src/ros_comm/tools/roslaunch/src/roslaunch/depends.py", line 209, in parse_launch
    _parse_launch(launch_tag.childNodes, launch_file, file_deps, verbose, context)
  File "/home/jliviero/ws/src/ros_comm/tools/roslaunch/src/roslaunch/depends.py", line 137, in _parse_launch
    _parse_launch(tag.childNodes, launch_file, file_deps, verbose, context)
  File "/home/jliviero/ws/src/ros_comm/tools/roslaunch/src/roslaunch/depends.py", line 181, in _parse_launch
    launch_file))
roslaunch.depends.RoslaunchDepsException: Cannot load roslaunch include '' in 'launch/custom_map.launch'

With my change, the output of roslaunch is:

jliviero@CPR00612:~/ws/src$ rosrun roslaunch roslaunch-check -o - launch/custom_map.launch
checking launch/custom_map.launch
WARNING: empty <include> in launch/custom_map.launch. Skipping <include> of $(arg apriltags)
...writing test results to -
passed

While the failing test could probably be ignored, it is causing our internal build tests to fail, and this seems like a reasonable enough use-case to submit a fix.

Editing my above posts accordingly.

[dirk-thomas]

Why should this be changed from if to elif?

[jliviero]

It shouldn't; this was an error, thanks for catching. Will correct.

[dirk-thomas]

To avoid the huge diff can you please avoid the change of indentation and instead use continue in the case the include is empty. That will also prevent unintentional changes in the block which should only changed the indentation.

[jliviero]

To avoid the huge diff can you please avoid the change of indentation and instead use continue in the case the include is empty. That will also prevent unintentional changes in the block which should only changed the indentation.

Done. Still some whitespace/line length fixes in the block, but it's otherwise unchanged.

[dirk-thomas]

Any unnecessary whitespace changes just make it more difficult to backport changes between different branches. Therefore I have cherry-picked your patch (without the whitespace changes) to the kinetic-devel branch: b323b8d

Thank you for the PR.

[jliviero]

Fair enough re: whitespace. Thanks.