search

orocos-toolchain / rtt

Orocos Real-Time Toolkit
http://www.orocos.org
Other
72 stars 79 forks source link

Update to package.xml format 3 and update maintainer email #321

Closed meyerj closed 2 years ago

meyerj commented 4 years ago

Format 3 is the latest specification of ROS package manifests (REP-0149).

Changes in manifest.xml are pure formatting.

@doudou @g-arjones Do you expect any problems with Autoproj (or any other build tool) if we update the package.xml files of all orocos-toolchain packages to format 3? I see that Autoproj supports parsing package.xml instead of manifest.xml since https://github.com/rock-core/autoproj/pull/183. Does that mean that we can finally drop manifest.xml at some point or solve the incompatibility issues in another way?

Although rosbuild is really outdated now and I don't see a problem to remove support in the near future, some ROS tools (e.g. rosdep) still prefer manifest.xml ("dry") over package.xml ("wet") and would be confused by applying incompatible changes, like <depend_optional> (https://github.com/orocos-toolchain/typelib/pull/94). The latter is still not supported by ROS, but since package.xml format 3 dependencies can be conditional. So I guess optional dependencies can be declared with a condition that evaluates to false by default.

g-arjones commented 4 years ago

Does that mean that we can finally drop manifest.xml at some point

I personally think so, yes.

doudou commented 4 years ago

Does that mean that we can finally drop manifest.xml at some point

I personally think so, yes.

I would rather not, no. I'm actually very happy with the current situation which is more or less manifest.xml == Rock and package.xml == ROS, allowing us to not have to align for all the dependency naming and other stuff.

snrkiwi commented 4 years ago

I think we should be consistent and write down what versions of things (e.g. operating system, ROS versions, catkin, compilers, etc.) that we support. Then the version of the XML format should be driven by that. At the moment it seems kind of scattershot ...

meyerj commented 4 years ago

I think we should be consistent and write down what versions of things (e.g. operating system, ROS versions, catkin, compilers, etc.) that we support. Then the version of the XML format should be driven by that. At the moment it seems kind of scattershot ...

I agree that those things need to be documented better. doc/xml/orocos-installation.xml is clearly outdated. But this is largely unrelated to the proposed patch here, at least from a pure ROS user's perspective:

So overall: No (intended) change in RTT dependencies or minimal versions due to this patch. It is only meant as a cleanup and fix for the maintainer email address. I just wanted to make sure that package.xml is not also used by some non-ROS tools, like it is the case for manifest.xml.

Does that mean that we can finally drop manifest.xml at some point

I personally think so, yes.

I would rather not, no. I'm actually very happy with the current situation which is more or less manifest.xml == Rock and package.xml == ROS, allowing us to not have to align for all the dependency naming and other stuff.

Okay. Let's hope that the ROS tools at some day drop support for manifest.xml, such that the "more or less" becomes 100% true. Until then, manifest.xml == Rock + ROS, and package.xml == ROS.

doudou commented 4 years ago

Until then, manifest.xml == Rock + ROS, and package.xml == ROS.

They prefer manifest.xml to package.xml ? I would have assumed the other way around.

I picked manifest.xml at the beginning with the hope to be compatible with ROS. This boat has long sailed. My proposal would be to add support for autoproj.xml in autoproj and then we can have a clean split.

meyerj commented 4 years ago

They prefer manifest.xml to package.xml ? I would have assumed the other way around.

Yes, unfortunately. catkin itself (and the underlying catkin_pkg Python module) does not, but other tools like rosdep and rospack (which are based on rospkg) open manifest.xml first and error out if it does not have the expected format. It is also not possible to avoid those backwards-compatible tools completely, e.g. when releasing into ROS. We already discussed this problem back in 2014 in https://github.com/orocos-toolchain/utilrb/issues/5 (and later issues/PRs) and I opened an issue for rosdep (https://github.com/ros-infrastructure/rosdep/issues/353), which confirmed that the behavior is intentional and won't be changed. The situation is still the same today.

Would it be possible to introduce an alternative file name for AutoProj instead, e.g. autoproj.xml or manifest.autoproj.xml, and to prefer that file over manifest.xml if it exists? Then later, once this new "feature" is generally available, we can rename manifest.xml to autoproj.xml here and for other toolchain packages and you are free to update the format at any time without the risk to break ROS tools.

doudou commented 4 years ago

Would it be possible to introduce an alternative file name for AutoProj instead, e.g. autoproj.xml or manifest.autoproj.xml

I think autoproj.xml would be reasonable, yes.

meyerj commented 2 years ago

I think we should be consistent and write down what versions of things (e.g. operating system, ROS versions, catkin, compilers, etc.) that we support. Then the version of the XML format should be driven by that. At the moment it seems kind of scattershot ...

I agree that an updated list of dependencies and supported versions is useful. rtt/doc/xml/orocos-installation.xml hosted at https://orocos-toolchain.github.io/rtt/toolchain-2.9/xml/orocos-installation.html or ocl/INSTALL is outdated and must be removed. The "official" and most up-to-date documentation is now hosted at https://github.com/orocos/orocos-docs and served at https://docs.orocos.org/docs_main/installation.html using Read The Docs. So all legacy per-repo README and INSTALLATION files, if not removed, could refer to that page, which I think is still up-to-date (we should probably verify this or run CI jobs in a virtual machine or container with only the minimal documented dependencies installed).

But regarding the list of dependencies or RTT and OCL this patch and its counterpart https://github.com/orocos-toolchain/ocl/pull/90 does change almost nothing, except if catkin is used as a build tool. package.xml format 3 is supported since version 0.4.24 of catkin_pkg (https://github.com/ros-infrastructure/catkin_pkg/pull/197), which was released into the ROS or Debian/Ubuntu repositories since Ubuntu 16.04 Xenial and all newer Ubuntu distributions. Apparently the patch was also backported into version 0.4.13 in Ubuntu Trusty 14.04 according to my last post above. Additionally, pip provides a sufficiently recent version that understands format 3 for all other distributions. If cmake is used directly or via the top-level orocos_toolchain/CMakeLists.txt file, the package.xml file is not even used.

However the patch does change

This update is required to build RTT and OCL in a ROS 2 environment or with ROS 2 build tools, for example with colcon that also uses the package.xml manifest but where catkin is not available.

We do not plan to remove manifest.xml until AutoProj would support an alternative name in the future, for example autoproj.xml (https://github.com/orocos-toolchain/rtt/pull/321#issuecomment-631755015).

So I will go forward and merge this patch and https://github.com/orocos-toolchain/ocl/pull/90 into toolchain-2.9 and newer branches. You are still welcome to comment here if it breaks something for you unexpectedly, or to open extra pull requests to update outdated documentation.