ros2 / launch

Tools for launching multiple processes and for writing tests involving multiple processes.
Apache License 2.0
124 stars 139 forks source link

Add new substitution to read files #707

Closed Yadunund closed 1 year ago

Yadunund commented 1 year ago

As reported here https://github.com/osrf/ros2_test_cases/issues/588#issuecomment-1536261556

mjcarroll commented 1 year ago

Confirmed that this also impacts my Iron source build on windows.

cottsay commented 1 year ago

The Python launch files read the contents of a URDF file directly using Python API, but the XML and YAML files call cat to dump the file contents to stdout and capture it. On Windows, cat is not implicitly available, which is the source of the file not found error. This isn't a regression - the launch files have been like this since they were introduced in 2020.

We could try to solve this by switching the command substitution to an eval substitution. It's a little less readable, but it's more portable:

-        value: $(command 'cat $(find-pkg-share dummy_robot_bringup)/launch/single_rrbot.urdf')
+        value: $(eval 'open("$(find-pkg-share dummy_robot_bringup)/launch/single_rrbot.urdf", "r").read()')

To be honest, I think the clean solution is to introduce a new substitution to read file contents in a portable way. The use case here (capturing a URDF for a robot_description parameter) isn't uncommon.

cottsay commented 1 year ago

I'll note that adding cat.exe from Git to my PATH causes the launch operations to run normally.

Yadunund commented 1 year ago

Wow nice find @cottsay! Do you think it's worth switching to eval with a note until we have a new substitution command?

cottsay commented 1 year ago

Do you think it's worth switching to eval with a note until we have a new substitution command?

In my opinion, we're too close to release to mess with a long-standing/low-impact bug like this, but I'm happy to open the PR if you think it warrants further discussion.

clalancette commented 1 year ago

I agree with @cottsay here that a new substitution to read file contents is the right way to go here.

But given that this is a long-standing bug and a new feature request, I'm going to mark this for J-Turtle instead, move it to the launch repository, and change the title.