moveit / moveit_planners

THIS REPO HAS MOVED TO https://github.com/ros-planning/moveit
15 stars 34 forks source link

Fixed include directory order to make ros package shadowing work. #64

Closed hersh closed 9 years ago

hersh commented 9 years ago

Before this change, the -I/opt/ros/indigo/include compile option from ${OMPL_INCLUDE_DIRS} would come before all of the -I/home/hersh/my-ws/src/myproject/include options defined in ${catkin_INCLUDE_DIRS}. That is a problem when I have a project in my workspace which is also installed in /opt/ros/indigo and which has header files with changes. Then the old version builds ompl_interface with the old version of the include files and my other code with the new version, and the results are not good when they get linked together.

After switching the lines, /opt/ros/indigo/include is correctly way at the end, after all of my workspace's include dirs, and only the correct version of the include file is used.

isucan commented 9 years ago

I am fine with merging this, but the reason it was done this way is that if you have ompl istalled as a ROS package, and as your own local lib, you need this order to prefer the local lib.

davetcoleman commented 9 years ago

@isucan is right, this undos @ryanluna's change and breaks our ability to develop on the OMPL library with MoveIt!. Is there a good alternative to these two conflicting interests?

mikeferguson commented 9 years ago

@davetcoleman can you post what the include directories look like with and without a local build of OMPL? It seems like we should be able to come up with a regex that decides if OMPL_INCLUDE_DIRS is based on debian install or local workspace and gets the ordering correct

davetcoleman commented 9 years ago

That's a good idea. I don't have a MoveIt! system up right now to get the include directories, will do in future

hersh commented 9 years ago

How do you build OMPL? If you built it in a catkin workspace it should work fine, shouldn't it?

If you have OMPL not in a catkin workspace and build and install with something like "make install" directly to /usr/lib etc, of course catkin will ignore it and prefer the catkin version. Anything in /usr is understood to be the "system" version by catkin.

If you want to build OMPL separate from catkin and have it "shadow" the one from ros-indigo-ompl, you should set the install prefix to /opt/ros/indigo instead of /usr.

davetcoleman commented 9 years ago

I build OMPL in a catkin workspace, yes. I don't use moveit_planners anymore (have my own custom package for this) but actually I have it set the way @hersh wants it to be:

include_directories(include
                    ${catkin_INCLUDE_DIRS}
                    ${OMPL_INCLUDE_DIRS})

So actually this PR seems fine. Mostly it seemed important to point out it reverted a specific past commit.