Jade sync'd back to Indigo

[rethink-imcmahon]

There have been some fantastic updates to the xacro-Jade branch, but none of these have been backported to Indigo. Seeing how Indigo is an LTS ROS distro, can these changes be sync'd back?

[codebot]

Yes, there are lots of fun new xacro features in Jade! Unfortunately, there is a difficult balance to achieve between not breaking previously-released distros (i.e. Indigo LTS) and providing access to new useful features, since Indigo will be around for a very long time. Since Indigo is our first LTS release, we're still trying to figure out where the balance is.

Are there one or two new features in particular that would be of most use to you? Perhaps we can evaluate them together on a case-by-base basis. Cheers

[rethink-imcmahon]

I think the biggest thing is the --inorder option and variable filenames. I have a pretty complex set of configurations that attempt to capture all the ways one can construct a Rethink Electric Gripper, a modular end effector with many fingers and fingertips.

The parent xacro, left_end_effector.urdf.xacro specifies the left and right finger slots, the kind of fingers, kind of finger tips, etc. All of those parameters need to be evaluated in order:

Indigo:

$ rosrun xacro xacro.py `rospack find baxter_description`/urdf/left_end_effector.urdf.xacro
Traceback (most recent call last):
  File "/data/users/imcmahon/dev/mrsp_ws/src/xacro/xacro.py", line 60, in <module>
    xacro.main()
  File "<string>", line 687, in main
  File "<string>", line 246, in process_includes
  File "<string>", line 483, in eval_text
  File "<string>", line 470, in handle_expr
  File "<string>", line 444, in eval_expr
  File "<string>", line 418, in eval_term
  File "<string>", line 398, in eval_factor
  File "<string>", line 369, in eval_lit
xacro.XacroException: Property wasn't defined: u'l_finger'

In Jade without --inorder:

Traditional processing is deprecated. Switch to --inorder processing!
To check for compatibility of your document, use option --check-order.
For more infos, see http://wiki.ros.org/xacro#Processing_Order
variable filename is supported with --inorder option only
when processing file: /data/users/imcmahon/dev/mrsp_ws/src/baxter_common/baxter_description/urdf/electric_gripper/rethink_electric_gripper.xacro
included from: /data/users/imcmahon/dev/mrsp_ws/src/baxter_common/baxter_description/urdf/left_end_effector.urdf.xacro

But in Jade with --inorder this command works beautifully (aside from the xacro.py deprecation warning):

rosrun xacro xacro.py --inorder `rospack find baxter_description`/urdf/left_end_effector.urdf.xacro

If you'd like to check out the dev branch with this xacro, you can find it here - https://github.com/RethinkRobotics/baxter_common/tree/gripper_model_xacro/baxter_description/urdf

I would very much like to include this gripper flexibility in the upcoming release of baxter_simulator with grippers, but that requires compatibility with Indigo. Thanks for all your effort maintaining xacro :)

[rhaschke]

Instead of merging back all those jade-devel changes, I suggest to include xacro into your distribution of baxter_simulator, thus overlaying the default installation of xacro. That's the way we go at CITEC too to have jade-devel's xacro in Indigo.

[rethink-imcmahon]

Yep, that's my backup plan, to require that wstool be checked out from
source on the Jade branch and included in the wstool workspace (we've had
to do this with xacro in the past). This would make using the Debian
package more complicated, but we would likely find a workaround.
Indigo is an LTS, and --inorder processing is an incredibly useful feature
for the whole community, more than just baxter_simulator users, so I
figured I'd bring it up here. Wasn't the rational for --inorder processing
being a flag not to disrupt normal xacro execution in the Indigo branch?

[rhaschke]

You are right, that the rationale of the --inorder option was to bring in the features without disrupting classical use. However, this new feature came with a lot of internal changes to xacro and we are not bold enough to declare that it's totally bug-free. That was the reason to push those features into Jade only.
I guess, extracting the --inorder changes will be a lot of extra effort, again risking the intro of some new bugs. @codebot: What do you think?

[rethink-imcmahon]

I certainly do not recommend breaking API or ABI for Indigo, and I can appreciate the desire to keep Indigo bug free. However, without this feature being backported, any Xacro URDF that utilizes the --inorder flag becomes incompatible with ROS Indigo, making cross-distro URDF maintenance a headache, especially when Indigo is here to stay until 2019.

[codebot]

I wonder if we could embed the jade xacro version into the indigo xacro package somehow. In other words, if the --inorder flag is seen, it would evaluate the input with xacro/src/xacro-jade/xacro or some other path within the xacro package. Or we could make up a new flag, like --jade that was perhaps more clear. That way, we could keep the existing Indigo xacro implementation and behavior intact, but provide the jade version for those who want new features. This would effectively be the same as overlaying the jade-devel xacro release into your Indigo workspace, but it would be slightly easier for end-users to install. The tradeoff is that the Indigo xacro would have a bit more spaghetti and the overall somewhat clunky feel. What do you think?

[rhaschke]

Embedding Jade xacro into the Indigo release and dynamically switching between them, becomes really ugly IMHO. For the user, it's probably not clear anymore, which xacro version is used or where the code comes from.

We currently focus particularly on the --inorder feature. However, with the same argument of @rethink-imcmahon, one could argue for some other new features, e.g. python-based property evalution, yaml support, etc. All of them will change the way we will write URDFs in future - in an incompatible fashion to the old xacro versions. Where do you want to draw the borderline between back-porting and new version? IMHO, if somebody wants to use these features, he should switch to Jade xacro.

@codebot I'm not an expert in debian packaging. Would it be possible to install Jade xacro instead of Indigo xacro on purpose? Then users could more easily switch to the new version using the package management system. As ros-<distro>-xacro is heavily bound to Jade, one would need another package ros-indigo-xacro-jade, which binds to Indigo and is an alternative to ros-indigo-xacro.

[rhaschke]

Looking into bloom a little bit, I believe one could define a new track indigo-jade, targeting Indigo distro, but building from jade-devel branch, and being named xacro-jade. Question remains, how bloom can handle the version constraints (conflicts / replaces). Obviously, this is supported, but I couldn't find any docs how to configure this.

[codebot]

@rhaschke that's a great idea. I'll look into the details of how that can be done.

[rethink-imcmahon]

Short of being able to cleanly port the changes to Indigo, I think the xacro-jade track is a good compromise. That would allow those who wish to use the updated features without breaking backwards compatibility. Thank you both for looking into this.

[codebot]

Sorry for the delay; we were running hard the past few weeks to get ROS2 pre-alpha out the door.

We just looked into this some more, and it looks like having multiple packages will be hard/impossible. From my current understanding, the Debian "provides" rules are only intended to be used with respect to virtual packages, and currently ROS doesn't use any virtual packages. That's not to say we couldn't figure out how to do that, but it's not currently being done (to my knowledge). I'm not sure how the "replaces" rule works, but even if we did that, I'm not sure it could be automated for users; they might need to run another apt or dpkg step to replace the "stock" indigo xacro with the upgraded xacro.

Because of the potential usability issues and our fear of changing debian dependency rules for a released distro, we're currently afraid to go that route. Besides the path of "wait until the new xacro features come out in an LTS version" which will be another 8 months, I think the easiest path forward would be to add a --jade flag to the Indigo xacro branch, which would then pass processing to a version of the Jade xacro branch that would be embedded inside the Indigo branch. It's really ugly, but at least we can then be sure of two things: 1) users who are using a current version of Indigo will have access to it and 2) in newer ROS distros, we can deprecate or silently ignore the --jade switch. I worry that using Debian packaging rules or requiring Indigo robot packages to depend on a different xacro package name would break one or both of those claims either now (for people who don't follow all installation steps) or in the future (when the dependency name needs to change back to just "xacro").

As an alternative, we could create a new package in Indigo called xacro-jade, and the entry processing of xacro could be to look for the --jade switch, and if it is seen, to pass the incoming command to the xacro-jade package. To make that work, robots using this functionality would need to depend on xacro-jade instead of xacro, and then in future ROS distros we'd need to keep a placeholder empty package called xacro-jade for at least one distro. IMHO, that's more work and a bit less clear to users, but we're willing to listen to all opinions.

What do you folks think?

Cheers 🐔

[rethink-imcmahon]

Thanks for investigating the dual-release and explaining your understanding of why it won't work, Morgan. Your reasoning is sound, and having potential fragmentation of packages for users to get caught up in is pretty undesirable.
I think the --jade switch does solve the problem. Just a thought, since this is a backport to Indigo, we could name this switch --inorder and then execute Jade code with the inorder flag... it's pretty sneaky, but regular processing is deprecated in Jade anyway, right?

[rhaschke]

If a debian package is too complicated, your proposed method (embedding xacro-jade into xacro-indigo) seems to be the only alternative. As you say, Morgan, it's ugly. That's why I was hoping for a cleaner solution. Your third alternative isn't much cleaner, either and leaves some traces in the future too.
Although --inorder flag is not mandatory in jade (nor old processing is deprecated yet), we could indeed use it as the switch.
This will be keep indigo robots, using this flag, compatible in the future automatically.

[codebot]

We're certainly open to other options, but we weren't able to come up with one or to envision a way that the Debian package relationship rules could be used in this case. Although it's ugly, embedding the Jade xacro and using --inorder to switch between them from a highest-level "dispatch" script seems like it would work. Maintaining forwards (and backwards!) compatibility is critical, and this strategy would seem to provide both. Cheers

[rethink-imcmahon]

I agree that the --inorder switch seems like a good compromise for forward and backward compatibility, and allows users to have just one xacro package, regardless of their ROS distro. It seems like the only downside is the "cleanliness" of the code, but the benefit in URDF flexibility is huge. Thank you both for brainstorming this solution.

[rethink-imcmahon]

Hi there, I know everyone is crazy busy with ROSCon coming up, but just a friendly ping on this issue. Is there a list of technical issues that need to be resolved to embed Jade xacro in Indigo?

[codebot]

Hi Ian,

Indeed, we've been running hard for ROSCon; sorry for the slow response.

In terms of technical issues, I don't think there are any huge "gotchas" here. I imagine that there will need to be a bit of code added to the Indigo xacro entry point which checks to see if the --inorder switch is there, and if so, it will need to call through to its embedded Jade version rather than running through its existing code. I'll start coding up an ugly version of that as a placeholder, but I suspect that @rhaschke can do a much nicer-looking version :)

Cheers

[codebot]

ok, I just threw up a strawman. I'm sure it has lots of problems, but an initial sanity-check seems to be working. It's on branch indigo-jade-via-inorder : https://github.com/ros/xacro/tree/indigo-jade-via-inorder

hopefully you can just check it out to your Indigo install and see if it works for you. I just did a plain copy of the jade-devel source and threw it in there, as well as a few lines in the xacro and xacro.py startup scripts which sniff for --inorder and choose which package to import. Again, I'm not pretending to know if this is the best way to do it, but at least it seems to do the right thing when faced with test/test_inorder_with_invalid_include.xml (i.e., the indigo-jade branch xacro fails to process that file without any arguments, but adding the --inorder argument causes it to process it successfully).

[rhaschke]

Dear Morgan,

your implementation looks perfectly fine.

[codebot]

@rethink-imcmahon does the strawman implementation work for your use case?

[rethink-imcmahon]

ah! sorry Morgan, I haven't tested this yet. I'll get back to you tonight after I've given it a try

[rethink-imcmahon]

+1 this strawman branch works beautifully. Thank you so much!

[codebot]

I know it's adding a lot of stuff to an already-released distro, but I
think the likelihood of impacting any existing Indigo xacro files is low
and the potential payoff is high since Indigo will be around for so long.
Any objection from anyone if this is merged to indigo-devel

On Wed, Oct 14, 2015 at 2:55 PM, Ian McMahon notifications@github.com
wrote:

+1 this strawman branch works beautifully. Thank you so much!

—
Reply to this email directly or view it on GitHub
#110 (comment).

[wjwwood]

Sounds like you found a winning strategy +1

[codebot]

ok, merging now.

[codebot]

merged #117

[rethink-imcmahon]

Thank you again, Morgan. This functionality is fantastic.

[codebot]

ok awesome! hopefully there aren't any weird side effects. I'll leave this
merged on indigo-devel for a few days for a soak-test by any interested
parties before doing a release.

On Mon, Oct 19, 2015 at 11:16 AM, Ian McMahon notifications@github.com
wrote:

Thank you again, Morgan. This functionality is fantastic.

—
Reply to this email directly or view it on GitHub
#110 (comment).

[rethink-imcmahon]

Hiya @codebot, any chance we could push this out to a bloom release? I've been using the indigo-devel branch for weeks without any unforeseen side effects. If others have seen any issues, please feel free to chime in. Thanks!

[codebot]

Yeah it seems like we've had a long enough soak period. I'll push out a release today. Cheers

[codebot]

ok, I just tagged 1.9.5. It will go out with the next Indigo sync

[mathias-luedtke]

I really appreciate this backport! But I guess it is quite difficult to maintain, especially if bug fixes needs to be synced back.

In addition the default behaviour was changed in jade and it might be preferable to have the old version for legacy xml files.

What do you think of introducing version suffixes, e.g. xacro1 and xacro2?
For indigo xacro points to xacro1 and for jade xacro points to xacro2, while both versions are accessible directly. If the branching could be done by catkin/cmake then the same code base could be maintained for indigo and jade.

[rhaschke]

We discussed that option in this track too (see here). However, it was too difficult to realize with bloom.

[mathias-luedtke]

However, it was too difficult to realize with bloom.

I did not mean to release it in a single track.
I propose to keep the same source layout for both to:

• be able to cherry-pick bug fixes easily
• enforce a specific version if needed, e.g. use legacy xacro in jade

The default version branching (xacro->xacro1 or xacro2) could be done at runtime (ROS_DISTRO env?) or explicitly in the distro-dependent code.

[rhaschke]

Probably I don't understand exactly what your are asking for. However, if there should be a xacro1 and a xacro2 package installed side by side, how should ROS switch between them?
As far as I know, rosrun, roslaunch etc. find packages by reading packages.xml. Hence, one would need to symbolic-link different xacro installation into the ROS package path.
Futhermore, the PYTHONPATH should be adapted. I don't think, that this is easy to achieve at runtime.

[mathias-luedtke]

1. by explicitly stating the version. e.g. xacro1
2. by runtime selection based on ROS_DISTRO or something like that; alternative: hardcoded defaults for the respective distros

Side-by-side installation is no problem, it is already implemented for indigo.
If you really consider this solution I can prepare a pull request.

[rhaschke]

You should wait for a comment from @codebot on this. I cannot see yet, how this can work out.
You definitely don't want to call xacro by explicitly stating the version, i.e. xacro1 or xacro2. This would break a lot of existing launch files, currently stating somethink like

<param name="robot_description" command="$(find xacro)/xacro ..."/>

[mathias-luedtke]

You definitely don't want to call xacro by explicitly stating the version, i.e. xacro1 or xacro2.

I don't want to break the launch files.
xacro should reference xacro1 on indigo and xacro2 on jade.
In addition it should be possible to use xacro1 on jade for legacy files that might be adapted for xacro2 towards a kinetic release.

[rethink-imcmahon]

While I agree that there is probably no solution that is both practical and elegant for merging the major Jade features back into Indigo, Morgan and Robert did come up with a very useful solution that enables Indigo, as our LTS, to see some of the benefits brought to Jade. It may be a bit more difficult to maintain in the short term, but there is nothing requiring every new commit to be backported. Having used the backported --inorder switch myself, I can attest that this is a huge improvement over regular xacro in Indigo.

[mathias-luedtke]

I have migrated the indigo version as an example: indigo-devel...ipa-mdl:restructuring
For jade only src/xacro/__init__.py needs to be changed.
The version could be derived from ROS_DISTRO as well.

[codebot]

@ipa-mdl thank you for looking into this and for posting the example migration.

Do you have legacy xacro files where the evaluation behavior with --inorder processing results in undesirable evaluation? If so, are they public so we can look at them and understand the issue better?

I suspect that for most people, especially new users, --inorder processing is what they actually assume is happening, regardless of how "old" xacro used to work; that's why I think it's the right thing to do as the default for Jade and K-turtle. I agree that future bugfixes in Jade need to be manually backported to Indigo, but that's a maintenance task I am willing to perform, because of the improved user experience that allows people to use the dramatically-improved xacro features of Jade in the LTS. Indigo and Jade xacro branched apart a long time ago, so I would expect that manual backporting of bug fixes would be required in either case.

I'd be interested to learn more about your use case and any issues you are seeing with porting your xacro files forward, since we will need to trade off the complexity of updating your legacy xacro files against the complexity of retaining a switchable branch of the "old" xacro processing regime on K-turtle. I'm currently of the opinion that the "new" xacro is so much better that it's worth the pain of updating legacy xacro files, but again, I'd like to know more about how this affects your systems. Thanks!

[mathias-luedtke]

@codebot: I do not have a specific use case since we will probably not migrate to jade, but directly to k-turtle (not yet decided). However, I really see a use case for the new xacro features (especially the namespacing) in indigo, even for LTS set-ups.

My focus was just on simplifying the maintenance.
In addition the migration I proposed will adhere to the (unofficial?) one-distro overlap policy :)

[rhaschke]

I like your modifications because now both xacro versions are handled in a symmetric fashion.
However, I don't see how this structure improves maintainability of the code. There is still the same effort to backport bug fixes, isn't it?
Morgan raised a very important point: Which issues do you foresee in jade or later ROS versions requiring the use of the "old" xacro. For jade, the idea was to have the old processing order as a default. Only in K-turtle this will be changed to the new inorder processing.
Regarding expression evaluation, we took care to maintain backwards compatibility. Indeed it would be very helpful to learn about any issues you observed with that.

[mathias-luedtke]

However, I don't see how this structure improves maintainability of the code. There is still the same effort to backport bug fixes, isn't it?

For improved maintainability the jade code needs to be migrated to the same scheme. This would enable to cherry-pick bug fixes (in both directions).
Indigo will be around for four more years and should get at least bug fixes in that period..

Morgan raised a very important point: Which issues do you foresee in jade or later ROS versions requiring the use of the "old" xacro.

As pointed out before I do not (yet?) have a specific issue to solve. But for some reason you decided to not merge it into indigo.

I guess I stumbled upon this issue because #117 coupled the inorder(=true) flag to the other new features and I just wanted to provide a cleaner solution.

If you don't like to introduce the version-based scheme, I am fine with it!
I do not rely on an indigo-version in jade. I am just happy to test the new features in indigo. We are going to automate URDF generation of multi-robots setups and will need to use legacy macros to a certain extent. I will get back to you if I find anything that does not work as expected :)