Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new substitution to read files #707

Closed
Yadunund opened this issue May 8, 2023 · 6 comments · Fixed by #708
Closed

Add new substitution to read files #707

Yadunund opened this issue May 8, 2023 · 6 comments · Fixed by #708
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@Yadunund
Copy link
Member

Yadunund commented May 8, 2023

As reported here osrf/ros2_test_cases#588 (comment)

@Yadunund Yadunund added the bug Something isn't working label May 8, 2023
@cottsay cottsay self-assigned this May 9, 2023
@mjcarroll
Copy link
Member

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

@cottsay
Copy link
Member

cottsay commented May 11, 2023

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
Copy link
Member

cottsay commented May 11, 2023

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

@Yadunund
Copy link
Member Author

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

@cottsay
Copy link
Member

cottsay commented May 11, 2023

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
Copy link
Contributor

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.

@clalancette clalancette added the enhancement New feature or request label May 11, 2023
@clalancette clalancette transferred this issue from ros2/demos May 11, 2023
@clalancette clalancette changed the title Unable to launch dummy_robot_bringup via xml or yaml launch files on Windows Add new substitution to read files May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants