search

ros-gbp / ompl-release

8 stars 10 forks source link

latest lunar release breaks MoveIt #14

Open rhaschke opened 6 years ago

rhaschke commented 6 years ago

@mamoll With the latest release of ompl into lunar, MoveIt doesn't find ompl-config.cmake anymore. This is probably due to the fact that the installation folder for cmake files has changed:

# find /opt/ros -iname "*ompl*"
...
/opt/ros/lunar/lib/x86_64-linux-gnu/libompl.so.1.3.2
/opt/ros/lunar/lib/x86_64-linux-gnu/cmake/ompl/omplConfigVersion.cmake
/opt/ros/lunar/lib/x86_64-linux-gnu/cmake/ompl/omplConfig.cmake
...

while before we had:

# find /opt/ros -iname "*ompl*"
...
/opt/ros/lunar/lib/x86_64-linux-gnu/libompl.so.1.3.1
/opt/ros/lunar/share/ompl/ompl-config.cmake
...

On Melodic, also omplConfig.cmake is installed, but directly in /opt/ros/*/lib/cmake:

# find /opt/ros -iname "*ompl*"
...
/opt/ros/melodic/lib/libompl.so.1.4.0beta
/opt/ros/melodic/lib/cmake/ompl/omplConfigVersion.cmake
/opt/ros/melodic/lib/cmake/ompl/omplConfig.cmake
...
mamoll commented 6 years ago

I thought I had fixed the cmake config files to be compatible with lunar, but obviously I messed up. Tell me what you want OMPL's config files to be named and where you want them installed for lunar and I'll fix it.

rhaschke commented 6 years ago

I think installing cmake files to lib/cmake/ompl as in Melodic is fine. The additional x86_64-linux-gnu seems to screw up cmake.

mamoll commented 6 years ago

Done: https://bitbucket.org/ompl/ompl/commits/24b724539e7278348f6a3ff22577180ebbbbab3c https://github.com/ros/rosdistro/pull/18018

rhaschke commented 6 years ago

Now I am completely confused. How can OMPLConfig.cmake (upper-case OMPL) install to omplConfig.cmake (lowercase ompl)? Did you also changed the case recently? We just changed MoveIt to use lowercase...

mamoll commented 6 years ago

I just changed the config file names to uppercase a couple days ago, because that's what MoveIt! wanted in Lunar. That's why I asked what filename you wanted, in case you wanted to change it.

rhaschke commented 6 years ago

Argh! We did it the other way around: https://github.com/ros-planning/moveit/pull/907. You, as the maintainer of the upstream package define the name. We need to follow. However, please use consistent naming throughout the various releases.

mamoll commented 6 years ago

Fixed for real this time, I hope: https://bitbucket.org/ompl/ompl/commits/fec5dcd200f5d43458dbc67af139580cb42a8b13 https://github.com/ros/rosdistro/pull/18019

mikaelarguedas commented 6 years ago

@rhaschke @mamoll : note that this will break the currently released moveit version in Lunar and that a new release of MoveIt! including https://github.com/ros-planning/moveit/pull/907 will need to be made shortly (as the next Lunar sync will likely happen later this week).

We encourage to make breaking / non backward compatible changes only in new distributions, keeping the other ROS distributions stable.

Linking to the same discussion from last year for book-keeping: https://github.com/ros/rosdistro/pull/16367

While MoveIt is the only released repository using this package, it doesn't mean that there are not several, unreleased or private, ROS packages using it and relying on the original uppercase naming.