search

moveit / moveit_ikfast

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

Generated packages append catkin_LIBRARY_DIRS to linker path #23

Closed gavanderhoorn closed 10 years ago

gavanderhoorn commented 10 years ago

The CMakeLists.txt of generated packages contains the following line:

link_directories(${catkin_LIBRARY_DIRS})

This would seem to be unnecessary as catkin / cmake link all libraries using absolute paths. According to the docs, it could also lead to subtle bugs trying to find libraries and resolving symbols.

Can this line be removed from the generated CMakeLists.txt? Or is it required to compile the plugin packages? Some simple tests suggest it is not.

davetcoleman commented 10 years ago

There are lots of other MoveIt/ROS packages that include this line, but I don't know enough about CMakeLists.txt. @dirk-thomas ?

dirk-thomas commented 10 years ago

As long as all libraries are referenced with absolute paths the line is not necessary which is try for all libraries coming from CMake config files generated by catkin. If any library would be listed with its name only the libraries are crucial to find the library. If you are sure that all libraries are always coming from "pure" catkin packages the line is not necessary. A lot of packages don't use the above line and work fine. (But I could not imagine why it would do any harm to keep it though.)

gavanderhoorn commented 10 years ago

I agree it currently 'just works'. Changing the library search path can have unanticipated consequences though, exactly in the case where target_link_libraries() doesn't get only absolute paths (iirc).

It seems catkin_lint also doesn't like it too much:

X_moveit_plugins: CMakeLists.txt(12): warning: use of link_directories() is strongly discouraged
gavanderhoorn commented 10 years ago

Hm, the catkin_lint warning is just there to warn people about the fact that link_directories() will not be propagated to dependent pkgs. This wouldn't seem to be a problem here then.

I think this can then be closed.

davetcoleman commented 10 years ago

If its unnecessary and if catkin_lint doesn't like it, I'd say lets remove it. This package is just a one-time "tool" and is not linked to by any other packages so isn't really necessary anyway.