Add pass_all_args attr to include tag by gerkey · Pull Request #710 · ros/ros_comm

gerkey · 2015-12-01T01:08:10Z

Add a new attribute, pass_all_args, to the include tag. It's a bool, with default of false. E.g.:

<arg name="foo" value="bar"/>
<arg name="bat" value="bang"/>

<!-- Pass down foo=bar and bat=bang -->
<include file="foo.launch" pass_all_args="true"/>

<!-- Pass down foo=override and bat=bang -->
<include file="foo.launch" pass_all_args="true">
  <arg name="foo" value="override"/>
</include>

<!-- The existing syntax can still be used to explicitly pass down foo=bar and bat=bang -->
<include file="foo.launch" pass_all_args="false">
  <arg name="foo" value="bar"/>
  <arg name="bat" value="bang"/>
</include>

If false, then nothing changes from existing behavior.

If true, then all args set in the current context are added to the child context that is created for processing the included file. You can do this instead of explicitly listing each argument that you want to pass down.

Notes on behavior when pass_all_args="true":

In the case that the parent passes down a value for an arg that is defined as constant in the child, then the child's value is used.
If an <arg> tag explicitly passes down an argument that is also set in the parent, then that explicit value overrides.

scpeters · 2015-12-09T22:14:00Z

Wow, this sounds amazing. Lots of gazebo_ros launch files are missing parameters because they all have to be manually defined

scpeters · 2015-12-09T23:05:54Z

tools/roslaunch/test/unit/test_xmlloader.py

I don't see expectations for include_arg or if_test in the test file, even though they are set below.

I'm only setting those (https://github.com/ros/ros_comm/blob/add_pass_all_args/tools/roslaunch/test/xml/test-arg-all.xml#L6-L7) because they're required in the included file, which I'm reusing from another test (https://github.com/ros/ros_comm/blob/add_pass_all_args/tools/roslaunch/test/xml/test-arg-include.xml#L4-L5), where they're used for a different testing purpose. I could test their values here, but I feel that that case (passing an arg that is required in an included file) is already covered by testing the value of the required arg.

dirk-thomas · 2015-12-09T23:58:20Z

Does the patch result in no exception being thrown in the following case anymore:

first.xml has a set of arguments (but not grounded)
first.xmlincludes second.xml with pass_all_args="True" and grounded="foo"
second.xml defines an argument grounded with a fixed value

If that is the case I think it would be good to preserve the exception.

dirk-thomas · 2015-12-09T23:59:32Z

Beside my previous comment the patch looks good to me (merge pending the decision into which ROS distro this and followup changes should be released).

scpeters · 2015-12-10T00:01:02Z

tools/roslaunch/test/xml/test-arg-all.xml

this value "not_set" is the same as the upstream test-arg-include.xml. Should we use a different value here?

I believe this would affect the test for /notall_optional/include_test/p2_test

I'm rethinking this...

scpeters · 2015-12-10T00:16:21Z

~~One feature that just occurred to me and may be related to the exception that @dirk-thomas mentioned:~~

~~Base launch file with optional parameters:~~

<launch>
  <arg name="param1" default="this_is_optional" />
...
  <arg name="paramN" default="this_is_optional" />
</launch>

~~If I want to create a derived launch file without redefining all the optional parameters and duplicating their default values, can I do:~~

<launch>
  <include ns="all" file="base.launch" pass_all_args="true">
    <param20>gazebo_world_name.world</param20>
  </include>
</launch>

~~and then specify param10:="--verbose" when I invoke it from the command-line? That would be really helpful, but maybe that's the exception that @dirk-thomas wants to keep?~~

I think this should work fine.

gerkey · 2015-12-10T01:49:23Z

@scpeters, your test case should work fine. Did you try it?

scpeters · 2015-12-10T01:59:00Z

No, I didn't test it. Sorry for the noise.

scpeters · 2015-12-10T02:00:02Z

Sorry I didn't understand @dirk-thomas 's comment, but now I do.

gerkey · 2015-12-10T02:00:30Z

@dirk-thomas, I understand the motivation behind your desire to have that exception behavior, and I would go further to say that it would be nice to always throw an exception in the case that the parent launch file explicitly sets an arg while including a file that sets the same arg constant, whether or not pass_all_args was enabled for that include, and whether or not that arg was set in the parent.

That is, ideally, it would only be legal to attempt to pass an arg to a child that sets the same arg constant in the case that the passing is implicit, via pass_all_args.

However, I don't see a way to implement that behavior without making intrusive changes into how roslaunch passes arguments from parent to child. We'd need to carry with each argument how it was set (implicitly via pass_all_args or explicitly in the include). Given that what we're talking about is stricter error-checking in a pretty narrow use case, I don't think that it's worth the risk of making those changes to get that exception behavior. (I also don't think that the lack of that exception behavior is sufficient to justify rejecting this PR.)

For future reference, I added a test that checks for that exception behavior, commented out for now: https://github.com/ros/ros_comm/blob/add_pass_all_args/tools/roslaunch/test/unit/test_xmlloader.py#L1040-L1053.

It is very good that you brought up this point, as I found a nasty bug while investigating the question, and added more tests as a result: https://github.com/ros/ros_comm/blob/add_pass_all_args/tools/roslaunch/test/unit/test_xmlloader.py#L1020-L1037.

ros-pull-request-builder · 2015-12-10T18:16:13Z

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/456/

dirk-thomas · 2015-12-14T17:47:22Z

@ros/ros_team Please review and state your opinion to merge this into Indigo and Jade.

gerkey · 2015-12-16T02:30:56Z

Having written it, I can't review it, but I vote to merge into both distros. It's a new feature that has no effect unless explicitly enabled, and I feel confident in both the characterization of the behavior and the coverage of the test suite.

However, I can appreciate the general stability argument for Indigo, in which case we can just put it in Jade.

gerkey · 2016-02-01T22:19:15Z

Result from today: merge to indigo and jade.

scpeters · 2016-02-08T22:44:30Z

Some tests failed. Are those flaky tests? Can the builds be re-triggered?

dirk-thomas · 2016-02-08T22:48:20Z

The two failing roswtf tests are due to ros/catkin#777 and will pass once that fix is available in a released catkin package.

If there are more failing tests that is likely due to the patch.

dirk-thomas · 2016-03-02T04:22:29Z

@ros-pull-request-builder retest this please

dirk-thomas · 2016-03-02T22:53:20Z

And squashed.

Add pass_all_args attr to include tag

dirk-thomas · 2016-03-02T23:54:33Z

@gerkey Please update the ROS wiki in the near future to document this new argument and comment here with a link to the diff.

gerkey · 2016-03-09T23:28:12Z

Docs updated!

http://wiki.ros.org/roslaunch/XML/include

dirk-thomas · 2016-03-09T23:35:26Z

Thank you. I added the version number to it in case anyone wants to check: http://wiki.ros.org/action/diff/roslaunch/XML/include?action=diff&rev1=6&rev2=7

gerkey · 2016-03-09T23:45:47Z

Awesome, thanks. I wanted to do that, but didn't know which version it would appear in.

Add pass_all_args attr to include tag

gerkey force-pushed the add_pass_all_args branch 2 times, most recently from 4239a43 to 73a9430 Compare December 1, 2015 01:42

scpeters reviewed Dec 9, 2015
View reviewed changes

scpeters reviewed Dec 10, 2015
View reviewed changes

add pass_all_args attr to include tag

0fa7e49

dirk-thomas force-pushed the add_pass_all_args branch from b9ab84f to 0fa7e49 Compare March 2, 2016 22:53

dirk-thomas added a commit that referenced this pull request Mar 2, 2016

Merge pull request #710 from ros/add_pass_all_args

1dba858

Add pass_all_args attr to include tag

dirk-thomas merged commit 1dba858 into indigo-devel Mar 2, 2016

dirk-thomas deleted the add_pass_all_args branch March 2, 2016 23:54

k-okada mentioned this pull request May 11, 2016

add option <arg> in launch file for flight mode which is based on the… start-jsk/jsk_mbzirc#74

Merged

scpeters mentioned this pull request Sep 7, 2016

roslaunch: pass_all_args exception if there are extra args defined #889

Closed

rsinnet pushed a commit to MisoRobotics/ros_comm that referenced this pull request Jun 19, 2017

Merge pull request ros#710 from ros/add_pass_all_args

4827df5

Add pass_all_args attr to include tag

wjwwood mentioned this pull request May 25, 2019

fix detection of arguments passed when including a launch description ros2/launch#252

Closed

wjwwood mentioned this pull request Aug 23, 2019

Propagate launch arguments in included launch descriptions ros2/launch#313

Open

Conversation

gerkey commented Dec 1, 2015

Uh oh!

scpeters commented Dec 9, 2015

Uh oh!

scpeters Dec 9, 2015

Choose a reason for hiding this comment

Uh oh!

gerkey Dec 9, 2015

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Dec 9, 2015

Uh oh!

dirk-thomas commented Dec 9, 2015

Uh oh!

scpeters Dec 10, 2015

Choose a reason for hiding this comment

Uh oh!

scpeters Dec 10, 2015

Choose a reason for hiding this comment

Uh oh!

scpeters Dec 10, 2015

Choose a reason for hiding this comment

Uh oh!

scpeters commented Dec 10, 2015

Uh oh!

gerkey commented Dec 10, 2015

Uh oh!

scpeters commented Dec 10, 2015

Uh oh!

scpeters commented Dec 10, 2015

Uh oh!

gerkey commented Dec 10, 2015

Uh oh!

ros-pull-request-builder commented Dec 10, 2015

Uh oh!

dirk-thomas commented Dec 14, 2015

Uh oh!

gerkey commented Dec 16, 2015

Uh oh!

gerkey commented Feb 1, 2016

Uh oh!

scpeters commented Feb 8, 2016

Uh oh!

dirk-thomas commented Feb 8, 2016

Uh oh!

dirk-thomas commented Mar 2, 2016

Uh oh!

dirk-thomas commented Mar 2, 2016

Uh oh!

dirk-thomas commented Mar 2, 2016

Uh oh!

gerkey commented Mar 9, 2016

Uh oh!

dirk-thomas commented Mar 9, 2016

Uh oh!

gerkey commented Mar 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants