search

ros-infrastructure / catkin_pkg

Standalone Python library for the catkin build system.
https://github.com/ros/catkin
Other
47 stars 91 forks source link

Is following version convention critical? (warnings of ompl version style) #140

Closed wkentaro closed 8 years ago

wkentaro commented 8 years ago

I have a lot of below warnings when using ROS packages, and it seems this problem remains for a long time. https://github.com/ros-gbp/ompl-release/releases

WARNING: Package "ompl" does not follow the version conventions. It should not contain leading zeros (unless the number is 0).

Is following version convention crucial? (If so why ompl can be released in ROS farm?) If not, I'd like to remove the line in https://github.com/ros-infrastructure/catkin_pkg/blob/master/src/catkin_pkg/package.py#L188 for few warnings.

Related to: https://github.com/ros-gbp/ompl-release/issues/4

davetcoleman commented 8 years ago

+1, its pretty annoying getting this warning. ive been getting it for months but am finally investigating. its because the package.xml says <version>1.0.0003094</version> instead of <version>1.0.3094</version>

dirk-thomas commented 8 years ago

The package in question ompl has last been released in 2014 into Indigo. The Jade version is basically the same. The package could easily be rereleased with a compliant version number. I don't see a reason to remove the check / warning in catkin_pkg just because some package decides to not follow the convention.

We support numerous platforms which each has its own rules and regulations for package names, version numbers, changelogs. We simply use the lowest common denominator here to ensure that packages works "everywhere" without needing patching for some platforms.

mamoll commented 8 years ago

The leading zeros are @isucan's idea. The version number can be broken down as:

<major>.<minor>.<patchlevel>00<hgrevision>

In the beginning when the OMPL package changed rapidly this made sense. I'd say that OMPL has been very stable. The ROS deb packages also don't track hg revisions and (should) track official releases. So in my opinion the 0's and the hg revision part of the version number can be dropped. It'd be good if the OMPL package in ROS could be updated to version 1.1, though.

davetcoleman commented 8 years ago

Is that format followed manually or is it coded somewhere? Could you change it?

ROS Kinetic would be ideal for OMPL 1.1, though we could sneak it into Jade too, probably. 1.1 does not include the C++11 support, correct?

mamoll commented 8 years ago

I think it is just hard-coded here: https://github.com/ros-gbp/ompl-release/blob/master/tracks.yaml. C+11 support is currently unreleased.

wjwwood commented 8 years ago

It would be great to see a released version of C++11 support (since the C++11 compiler is now a minimum requirement for Kinetic). However, if given the choice between C++11 support on an unreleased branch and a released version without, I'd favor the released version for Kinetic since it will be an LTS. Maybe interested parties could lean on the OMPL maintainers to get another release out.

mamoll commented 8 years ago

I am one of the OMPL maintainers. The official release should compile with -std=c++11. In other words it's supported, but not required. What's in our repo requires that flag.

wjwwood commented 8 years ago

I see. My point was that you can now require C++11 and not have any issues as of Kinetic so it would be a good time to release that version. In Jade and earlier, it was still recommended not to require C++11 features.

davetcoleman commented 8 years ago

I've switched my OMPL code and work to the C++11 version of OMPL and really like it. I've already ported moveit to using it on my private branch. Would be happy to merge in the changes publicly for Kinetic

mamoll commented 8 years ago

Is it ROS policy that for a given ROS release, the included packages cannot change the API/ABI? I.e., if we don't push out a new release soon, Kinetic will have OMPL 1.1.* for its whole lifetime?

wjwwood commented 8 years ago

We strive for API/ABI in the core packages (desktop-full), but it isn't enforced. It's up to you and your community if you want to break API/ABI during a release. However, it is almost certainly preferable to avoid it and get major changes in as near to the release as possible.

mamoll commented 8 years ago

We depend on Boost. As far as I know Boost packages in Ubuntu are still not compiled in C++11 mode. If we compile / link against Boost in C++11 mode and another package that depends on OMPL also uses Boost and does not compile in C++11 mode, bad things will happen. We cannot be the only ones having to deal with this. I wonder how others have fixed this...

davetcoleman commented 8 years ago

yea, like @wjwwood said, it would be best if we switched to 1.1 while releasing ROS-K for the first time

i think we'll basically have to switch most of moveit to C++11 as well, which I would prefer. do you have some regex/findreplace scripts you used while upgrading OMPL? Id be interested in those

wjwwood commented 8 years ago

As far as I know Boost packages in Ubuntu are still not compiled in C++11 mode.

That might be true, can you link to any reference which supports that?

If we compile / link against Boost in C++11 mode and another package that depends on OMPL also uses Boost and does not compile in C++11 mode, bad things will happen.

Have you run into that yet? This is another example of something that's good to discover now while we're in pre-release of Kinetic. We haven't seen this issue yet, but maybe we've just avoided it.

mamoll commented 8 years ago

I don't have any reference for Boost being compiled in C++0x mode. Gcc still defaults to C++0x, although that will change with gcc 6 where they switch to C++14 (https://gcc.gnu.org/gcc-6/changes.html).

I have only had problems on OS X with Boost and FCL compiled in C++0x mode and then OMPL and OMPL.app (OMPL + simple "app" layer that adds support for FCL, PQP, Assimp and a GUI) compiled in C++11 mode. Perhaps g++ handles mixing C++ modes better than clang++.

dirk-thomas commented 8 years ago

If someone could rerelease the existing ompl package with a compliant version number (preferably with no other changes) into Indigo and Jade that would be great since it would get rid of the annoying warning for every ROS user.

A new release of ompl into e.g. Kinetic can then choose whatever version scheme which complies in the first place.

mamoll commented 8 years ago

@vrabaud, can you fix the leading zeros in the patch-level version in https://github.com/ros-gbp/ompl-release/blob/master/tracks.yaml? Could you also add an entry for ROS Kinetic for OMPL version 1.1?

davetcoleman commented 8 years ago

ping @vrabaud, we are nearing ready to release moveit into Kinetic

vrabaud commented 8 years ago

Fixed in https://github.com/ros/rosdistro/pull/11849 (Indigo), https://github.com/ros/rosdistro/pull/11850 (Jade) and https://github.com/ros/rosdistro/pull/11851 (Kinetic).

I took the latest version (June 2nd) for Kinetic for now. Whenever the final version with C++11 is released, the version will be strictly superior to 1.1 so we should be fine bumping to an official tag. https://bitbucket.org/ompl/ompl/issues/247/new-ompl-release-requested-that-includes-c

@isucan, you're still the maintainer btw ... Now that you are married, you should get back to your ROS duties so that your friends find time to get a wife too :)

davetcoleman commented 8 years ago

Thanks @vrabaud, I see you updated the version number for all three branches but only the code for Kinetic - perfect!

wkentaro commented 8 years ago

Thank you @davetcoleman @vrabaud !

isucan commented 8 years ago

Thank you @vrabaud! I will be better at this!

tfoote commented 8 years ago

Thanks @vrabaud !

I opened https://github.com/ros/rosdistro/issues/11852 to enforce this in the rosdistro so we can leave catkin_pkg with just a warning.