Xacro should not use plain 'include' tags but only namespaced ones. #9

davetcoleman · 2013-06-07T23:09:45Z

This is an unfortunate conflict because it might break some people's launch files, but xacro currently is not obeying its namespace as it says it does in its documentation. Gazebo's extension has its own that is only used within its element, but Xacro is improperly trying to parse these as Xacro includes.

More details on Gazebo's issue tracker:
https://bitbucket.org/osrf/gazebo/issue/730/xacro-not-compatible-with-urdfs-sdfs

I'm only proposing we make this change in Hydro...

Reviewers
@jonbinney @jbohren @nkoenig @hsu

scpeters · 2013-06-07T23:49:57Z

This would be useful for people who want to reuse models in gazebo, since sdf has its own include tag.

If this goes into hydro, we should also put a deprecation warning in groovy so people have a chance to change their models.

jonbinney · 2013-06-08T08:00:59Z

Looks good but I agree with @scpeters that a deprecation warning in groovy is needed so that stuff doesn't break unexpectedly for people when they install hydro. @davetcoleman do you have time to add that in a pull request to groovy-devel?

nkoenig · 2013-06-08T17:35:11Z

+1

jonbinney · 2013-06-09T02:11:24Z

I just had a look through some urdf's (for instance the pr2 xacro files), and it seems like use of include instead of xacro:include is pretty widespread. To make things easier, we should probably give people a script to fix their files. The following seems to work:

sed -i 's/<include/<xacro:include/g' `find . -iname *.xacro`

Thoughts on the best way to notify people of the change and provide the script? We could just include this one liner in the deprecation message in groovy.

Would it also make sense for a plain "include" to raise an error in hydro? Would be better than silently failing to do the include.

jbohren · 2013-06-10T02:24:47Z

Thoughts on the best way to notify people of the change and provide the script? We could just include this one liner in the deprecation message in groovy.

Yeah, I like that idea. Maybe we could include the sed line in a script (call it "migrate" or something) that we package with the groovy version which takes proper input arguments, and suggest people call that.

Would it also make sense for a plain "include" to raise an error in hydro? Would be better than silently failing to do the include.

I'd even say we should just deprecate it in hydro, and not force an error until igloo.

davetcoleman · 2013-06-10T18:05:05Z

a deprecation warning in groovy is needed

+1

we should probably give people a script to fix their files

+1

Would it also make sense for a plain "include" to raise an error in hydro?

A plain include is not a bad thing if it is used in the proper context, e.g.

<gazebo>
  <include>
  </include>
</gazebo>

So it would have to be fairly context aware to know when to throw an error. I'm not sure we could test for every possible edge case - e.g. ppl with custom tags in URDF.

Would be better than silently failing to do the include.

It won't be that silent since parts of your robot won't properly load in Rviz/Gazebo/MoveIt.

I'd even say we should just deprecate it in hydro, and not force an error until igloo.

That's a long time from now to have something broken and causing issues with Gazebo. The new yearly release cycle requires us to make more changes between releases.

jonbinney · 2013-06-10T18:26:41Z

What is the workflow we are trying to enable for people using gazebo who also use URDF? Do they keep a bunch of xacro files which expand into a urdf which is then converted into an sdf? If that is the case, than this <include tag shouldn't be a problem because it doesn't exist in URDF, right?

jonbinney · 2013-06-10T19:29:58Z

Adding a couple more people for opinions on how disruptive this change will be for users: @chadrockey @ahendrix

chadrockey · 2013-06-10T20:48:42Z

I haven't touched this stuff seriously in a long time, but it looks like xacro is always limited to the form:

<include filename="foo" />

and Gazebo is always

<include><other /></include>

Is it possible for xacro to not parse include tags unless they have the filename attribute? (Or attributes in general?)

davetcoleman · 2013-06-10T21:21:07Z

Chad, great idea. We would use this workaround for Groovy and Hyrdo, but with a deprecated warning if your Xacro include doesn't have the namespace. In Igloo we'll remove this workaround and require the <xacro:include> namespace. We'll also include the conversion script.

Does this sound good to everyone? I'll make a new pull request soon.

jonbinney · 2013-06-11T20:20:58Z

Yeah, that sounds like a good workaround.

…ings if not present.

davetcoleman · 2013-06-11T22:41:43Z

Okay, I've just updated the pull request with the new changes. This update will cause a deprecated warning for every plain element that doesn't immediately contain a Gazebo element within it. It includes the quick fix script which I have tested works on the PR2 xacro files.

The message is:

DEPRECATED IN HYDRO:
  The <include> tag should be prepended with 'xacro' if that is the intended use 
  of it, such as <xacro:include ...>. Use the following script to fix incorrect
  xacro includes:
     sed -i 's/<include/<xacro:include/g' `find . -iname *.xacro`"""

This should definitely go into Hydro, but should it go into Groovy?
@tfoote

tfoote · 2013-06-11T23:52:04Z

This kind of major change shouldn't be backported to an already released
system.

On Tue, Jun 11, 2013 at 3:41 PM, Dave Coleman notifications@github.comwrote:

Okay, I've just updated the pull request with the new changes. This update
will cause a deprecated warning for every plain element that doesn't
immediately contain a Gazebo element within it. It includes the quick fix
script which I have tested works on the PR2 xacro files.

The message is:

DEPRECATED IN GROOVY/HYDRO:
The tag should be prepended with 'xacro' if that is the intended use
of it, such as <xacro:include ...>. Use the following script to fix incorrect
xacro includes:
sed -i 's/<include/<xacro:include/g' find . -iname *.xacro"""

This should definitely go into Hydro, but should it go into Groovy?
@tfoote https://github.com/tfoote

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/9#issuecomment-19297292
.

davetcoleman · 2013-06-12T00:43:52Z

Ok, I think its ready for hydro-devel

jonbinney · 2013-06-12T00:52:39Z

src/xacro/__init__.py

+            if elt.tagName == 'include':
+                check_next = next_element(elt)
+                # check if there is a <uri> element within the <include> tag
+                if check_next.tagName == 'uri':


in a gazebo <include> tag, is the <uri> tag guaranteed to be first? Couldn't it be <name> or <pose>?

good point, no guarantee. in fact, i believe just the existence of child elements is enough to conclude xacro should not be involved.

agreed. can you change that before we merge?

… of an <include> tag

davetcoleman · 2013-06-13T17:58:39Z

Ok, now an tag is considered a Xacro one if it has no child elements (and a deprecated warning is thrown) but Xacro ignores the tag if it has child elements (i.e. Gazebo's element).

jonbinney · 2013-06-13T18:35:31Z

Aaaaalmost there. The only problem with this is minidom creates text nodes which count as child nodes. So if someone wrote

<include filename="foo">
</include>

Then childNodes is

[<DOM Text node "u'\n'">]

Which evaluates to True, and not be expanded, even though it probably should.

Can we filter out text nodes for the purpose of this check?

… nodes

davetcoleman · 2013-06-14T21:27:07Z

Good point, fixed.

Xacro should not use plain 'include' tags but only namespaced ones.

jonbinney · 2013-06-14T21:29:59Z

I've merged this. If we find corner cases where we need to tweak the workaround, we can do that in separate pull requests.

jonbinney · 2013-06-15T00:59:37Z

Hah! It totally worked! I just launched some stuff, got a ton of these warnings, copy/pasted the suggested sed line, and re-launched. Perfect!

davetcoleman · 2013-06-18T17:49:26Z

Awesome! I've update the Xacro documentation with a note about the Hydro deprecation

Changes since 1.7.3: 1.9.2 (2014-07-11) ------------------ * add a few more tests to exercise the symbol table a bit more * allow for recursive evaluation of properties in expressions * add useful debugging information when parameters are not set * stop test from failing the second time it is run * unified if/unless handling, correctly handle floating point expressions * floating point expressions not equal zero are now evaluated as True * changed quotes to omit cmake warning * Contributors: Robert Haschke, Mike Ferguson 1.9.1 (2014-06-21) ------------------ * fixup tests so they run * export architecture_independent flag in package.xml * installed relocatable fix * Contributors: Michael Ferguson, Mike Purvis, Scott K Logan 1.9.0 (2014-03-28) ------------------ * Remove the roslint_python glob, use the default one. * Add roslint target to xacro; two whitespace fixes so that it passes. * fix evaluation of integers in if statements also added a unit test, fixes `#15 <https://github.com/ros/xacro/issues/15>`_ * fix setting of _xacro_py CMake var, fixes `#16 <https://github.com/ros/xacro/issues/16>`_ * Add support for globbing multiple files in a single <xacro:include> * code cleanup and python3 support * check for CATKIN_ENABLE_TESTING 1.8.4 (2013-08-06) ------------------ * Merge pull request `#9 <https://github.com/ros/xacro/issues/9>`_ from davetcoleman/hydro-devel Xacro should not use plain 'include' tags but only namespaced ones. * Fix for the fact that minidom creates text nodes which count as child nodes * Removed <uri> checking and made it more general for any child element of an <include> tag * Removed Groovy reference, only being applied to Hydro * Created check for Gazebo's <uri> tabs only only shows deprecated warnings if not present. * Small spelling fix * Xacro should not use plain 'include' tags but only namespaced ones. * Merge pull request `#8 <https://github.com/ros/xacro/issues/8>`_ from piyushk/hydro-devel-conditional xacro conditional blocks * using refined arguments instead of sys.argv for xml file location * adding conditional blocks to xacro 1.8.3 (2013-04-22) ------------------ * bumped version to 1.8.3 for hydro release * backwards compatilibity with rosbuild * adding unit test for substitution args * Adding supoprt for substitution_args 'arg' fields * Remove bin copy of xacro.py

davetcoleman added 2 commits June 7, 2013 16:00

Xacro should not use plain 'include' tags but only namespaced ones.

ab90157

Small spelling fix

29defad

Created check for Gazebo's <uri> tabs only only shows deprecated warn…

43e7d2d

…ings if not present.

Removed Groovy reference, only being applied to Hydro

bc121cc

jonbinney reviewed Jun 12, 2013
View reviewed changes

Removed <uri> checking and made it more general for any child element…

cd65deb

… of an <include> tag

Fix for the fact that minidom creates text nodes which count as child…

0952003

… nodes

jonbinney pushed a commit that referenced this pull request Jun 14, 2013

Merge pull request #9 from davetcoleman/hydro-devel

433af8e

Xacro should not use plain 'include' tags but only namespaced ones.

jonbinney merged commit 433af8e into ros:hydro-devel Jun 14, 2013

piyushk mentioned this pull request Aug 6, 2013

New release into Hydro? #14

Closed

osrf-migration mentioned this pull request Apr 19, 2020

Xacro not compatible with URDFs->SDFs gazebosim/gazebo-classic#730

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xacro should not use plain 'include' tags but only namespaced ones. #9

Xacro should not use plain 'include' tags but only namespaced ones. #9

davetcoleman commented Jun 7, 2013

scpeters commented Jun 7, 2013

jonbinney commented Jun 8, 2013

nkoenig commented Jun 8, 2013

jonbinney commented Jun 9, 2013

jbohren commented Jun 10, 2013

davetcoleman commented Jun 10, 2013

jonbinney commented Jun 10, 2013

jonbinney commented Jun 10, 2013

chadrockey commented Jun 10, 2013

davetcoleman commented Jun 10, 2013

jonbinney commented Jun 11, 2013

davetcoleman commented Jun 11, 2013

tfoote commented Jun 11, 2013

davetcoleman commented Jun 12, 2013

jonbinney Jun 12, 2013

davetcoleman Jun 12, 2013

jonbinney Jun 12, 2013

davetcoleman commented Jun 13, 2013

jonbinney commented Jun 13, 2013

davetcoleman commented Jun 14, 2013

jonbinney commented Jun 14, 2013

jonbinney commented Jun 15, 2013

davetcoleman commented Jun 18, 2013

Xacro should not use plain 'include' tags but only namespaced ones. #9

Xacro should not use plain 'include' tags but only namespaced ones. #9

Conversation

davetcoleman commented Jun 7, 2013

scpeters commented Jun 7, 2013

jonbinney commented Jun 8, 2013

nkoenig commented Jun 8, 2013

jonbinney commented Jun 9, 2013

jbohren commented Jun 10, 2013

davetcoleman commented Jun 10, 2013

jonbinney commented Jun 10, 2013

jonbinney commented Jun 10, 2013

chadrockey commented Jun 10, 2013

davetcoleman commented Jun 10, 2013

jonbinney commented Jun 11, 2013

davetcoleman commented Jun 11, 2013

tfoote commented Jun 11, 2013

davetcoleman commented Jun 12, 2013

jonbinney Jun 12, 2013

Choose a reason for hiding this comment

davetcoleman Jun 12, 2013

Choose a reason for hiding this comment

jonbinney Jun 12, 2013

Choose a reason for hiding this comment

davetcoleman commented Jun 13, 2013

jonbinney commented Jun 13, 2013

davetcoleman commented Jun 14, 2013

jonbinney commented Jun 14, 2013

jonbinney commented Jun 15, 2013

davetcoleman commented Jun 18, 2013