$(dirname) is affected by previous ($find-pkg-share ...)

[felixdivo]

Bug report

Required Info:

• Operating System:
  • Ubuntu 20.04
• Installation type:
  • binaries
• Version or commit hash:
  • up to date
• DDS implementation:
  • irrelevant
• Client library (if applicable):
  • irrelevant

Steps to reproduce issue

Create file test.launch.yaml.

launch:
  - executable:
      cmd: echo $(dirname)
      name: obvious
      output: screen
  - include:
      file: $(find-pkg-share some_package)/file.launch.yaml
  - executable:
      cmd: echo $(dirname)
      name: surprise
      output: screen

The file file.launch.yaml in some_package:

launch: []

Run ros2 launch test.launch.yaml.

Expected behavior

echo $(dirname) outputs the path to test.launch.yaml each time.

Actual behavior

echo $(dirname) outputs the path to test.launch.yaml the first time and the path to the package share of some_package the second time. It appears that "the context" is somehow not reset. Is this really intentional? I also could not find the docs for that substitution.

Additional information

launch:
  - executable:
      cmd: echo $(dirname)
      name: obvious
      output: screen
  - executable:
      cmd: echo $(find-pkg-share some_package)
      name: testing_this
      output: screen
  - executable:
      cmd: echo $(dirname)
      name: surprise
      output: screen

In this setting echo $(dirname) outputs the path to test.launch.yaml each time.

Implementation considerations

It is very unexpected but also breaking when changed.

[jacobperron]

I can confirm the reported behavior for Rolling; the same applies to a regular Python launch file:

import launch
from launch.actions import ExecuteProcess
from launch.actions import IncludeLaunchDescription
from launch.launch_description_sources import PythonLaunchDescriptionSource
from launch.substitutions import ThisLaunchFile

from launch_ros.substitutions import FindPackageShare


def generate_launch_description():
    return launch.LaunchDescription([
        ExecuteProcess(name='obvious', cmd=['echo', ThisLaunchFile()], output='screen'),
        IncludeLaunchDescription(PythonLaunchDescriptionSource([FindPackageShare('dummy_robot_bringup'), '/launch/dummy_robot_bringup.launch.py'])),
        ExecuteProcess(name='surprise', cmd=['echo', ThisLaunchFile()], output='screen'),
    ])

I've marked this ticket as a bug since I don't think this is the intended behavior for the ThisLaunchFile substitution.

[jacobperron]

So, the issue happens because ThisLaunchFile returns the value from the launch context's locals,

https://github.com/ros2/launch/blob/074cd2903ddccd61bce8f40a0f58da0b7c200481/launch/launch/substitutions/this_launch_file.py#L59

and this value is overridden by IncludeLaunchDescription when it is executed,

https://github.com/ros2/launch/blob/074cd2903ddccd61bce8f40a0f58da0b7c200481/launch/launch/actions/include_launch_description.py#L150-L152

Note, I think we have the same issue with ThisLaunchFileDir.

One possible solution that comes to mind is to scope the context locals to the IncludeLaunchDescription action. We could achieve this by changing this line to something like,

return [PushContextLocals(), *set_launch_configuration_actions, launch_description, PopContextLocals()]

where PushContextLocals() and PopContextLocals() are new actions. If we don't want to expose these are new actions, we could use an OpaqueFunction.

The question I have is whether this will disrupt other functionality. I don't actually know the intention behind context.locals, so scoping them to an included description might have undesired side-effects. @wjwwood @hidmic @ivanpauno Thoughts?

[ivanpauno]

One possible solution that comes to mind is to scope the context locals to the IncludeLaunchDescription action. We could achieve this by changing this line to something like,

+1 I think that making the "include" action scoped by default is a good idea. If we don't want to do that, we could do something like (hope the pseudocode is clear enough):

return [*set_launch_configuration_actions, launch_description, ResetCurrentLaunchFileAndDirname(launch_file=old_launch_file)]

The question I have is whether this will disrupt other functionality. I don't actually know the intention behind context.locals, so scoping them to an included description might have undesired side-effects.

I don't think it can have undesired side-effects. IIRC, I think the idea was that include should work like if you literally wrote the included launch file inline, and if you wanted another scope you would use a scoped group. I think that having by default scoped includes is more intuitive, but I think we can fix this issue independently of that change.

[hidmic]

I think that making the "include" action scoped by default is a good idea.

I agree, but I also think include and group actions should scope the same way. It'd be very unintuitive to have scope=True mean different things for different "composite" actions.

[fnobis]

I am facing the same issue. Is there any workaround for this, until it is fixed on rolling?