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

Unexpected $(dirname) behaviour when value assigned to arg. #1487

Closed
mikepurvis opened this issue Aug 16, 2018 · 4 comments · Fixed by #2173
Closed

Unexpected $(dirname) behaviour when value assigned to arg. #1487

mikepurvis opened this issue Aug 16, 2018 · 4 comments · Fixed by #2173

Comments

@mikepurvis
Copy link
Member

mikepurvis commented Aug 16, 2018

The existing test is here, added as part of the overall feature in #1103:

https://github.com/ros/ros_comm/blob/e96c407c64e1c17b0dd2bb85b67f388380527097/tools/roslaunch/test/xml/test-dirname.xml

If the test is augmented to be:

<launch>
  <param name="foo" value="$(dirname)/bar" />
  <include file="$(dirname)/test-dirname/included.xml">
    <arg name="baz" value="$(dirname)/baz" />
  </include>
</launch>

And then the included.xml file is changed to:

<launch>
  <arg name="baz" />

  <param name="bar" value="$(dirname)/baz" />
  <param name="baz" value="$(arg baz)" />
</launch>

You'd expect the baz parameter to have tools/roslaunch/test/xml/baz as its path, but instead it's tools/roslaunch/test/xml/test-dirname/baz. That is, dirname is being lazily resolved at the point where the value is assigned to the parameter, rather than when the arg value is assigned. Historically this hasn't mattered, as no other substitutions have this kind of context sensitivity.

I'm not sure how much effort this will be to fix, but I had an instance recently where it would have been nice to be able to pass a "back path" into an included file, and wasn't able to do so due to this problem.

@danielwangksu
Copy link
Contributor

this indeed is a issue

@v4hn
Copy link
Contributor

v4hn commented Jun 28, 2021

This is entirely unintuitive behavior demoting the use of the (in ROS terms) relatively new $(dirname).
Sad to see this unresolved three years after Mike went through the trouble to document it...

@mikepurvis
Copy link
Member Author

@v4hn In fairness to the maintainers of this repo, I am in fact the author of the original $(dirname) patch, so this ticket was as much about self-disclosure as it was anything else. :) We ended up reworking our launchfiles to not require this fixed, but I left the ticket up in case someone else was interested in tackling it.

@v4hn
Copy link
Contributor

v4hn commented Jun 28, 2021 via email

v4hn added a commit to v4hn/moveit that referenced this issue Jun 30, 2021
Available since lunar. This simplifies the descriptions in moveit_resources
quite a bit because their package name does not match the include path
and we have to adapt the names everywhere after edits.

Note that two candidates for this replacement still remain -
they define the file path to the rviz config in an arg value.
Due to [a bug](ros/ros_comm#1487)
these two can not be replaced at this moment.
v4hn added a commit to moveit/moveit that referenced this issue Jul 1, 2021
Available since lunar. This simplifies the descriptions in moveit_resources
quite a bit because their package name does not match the include path
and we have to adapt the names everywhere after edits.

Note that two candidates for this replacement still remain -
they define the file path to the rviz config in an arg value.
Due to [a bug](ros/ros_comm#1487)
these two can not be replaced at this moment.
rhaschke added a commit to ubi-agni/moveit that referenced this issue Jul 17, 2021
... due to this bug: ros/ros_comm#1487
This partially reverts 442c320
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants