search

leggedrobotics / ros_best_practices

Best practices, conventions, and tricks for ROS
https://rsl.ethz.ch/education-students/lectures/ros.html
BSD 3-Clause "New" or "Revised" License
1.49k stars 413 forks source link

Correct the inclusion and usage of Eigen in CMakeLists.txt #25

Closed peci1 closed 4 years ago

peci1 commented 4 years ago

Specifying ${EIGEN3_INCLUDE_DIR} as an explicit catkin_package INCLUDE_DIR is not as good idea as putting it as a DEPENDS package. That will do the same thing and, moreover, is future-proof. Further, it also adds required compiler definitions, which you've forgot to do here (and I think lots of people would). This can have disastrous consequences if you change some flags that alter memory layout of Eigen objects.

Also, adding the include directory via a UseEigen3 file is more future-proof.

tomlankhorst commented 4 years ago

AFAIK, the best practise these days is to instruct CMake to link the Eigen3::Eigen target. target_link_libraries (${PROJECT_NAME}_core Eigen3::Eigen)

peci1 commented 4 years ago

Do you have a reference for using the Eigen3::Eigen target? It seems weird to me that you'd add include directories by putting something into target_link_libraries...

peci1 commented 4 years ago

Ah, found it... https://eigen.tuxfamily.org/dox/TopicCMakeGuide.html

peci1 commented 4 years ago

But that'd mean we have to give up on catkin passing the dependency to dependent packages? Because catkin_package(DEPENDS) will only pass the the CMake variables, and not the target...

tomlankhorst commented 4 years ago

As I understand it, catkin_package(DEPENDS) is for dependencies in the ROS ecosystem. That's with a package.xml and a typical CMakeLists.txt. System dependencies - that is Eigen, Qt, libraries, etc., - can be resolved through the regular find_package(...) and target_link_libraries(...) commands. Do you agree with this?

peci1 commented 4 years ago

You're probably talking about catkin_package(CATKIN_DEPENDS). See https://answers.ros.org/question/58498/what-is-the-purpose-of-catkin_depends/ for a good explanation.

tomlankhorst commented 4 years ago

Thanks! Eigen is a transitive dependency of packages that depend on this package, so it should indeed be passed with the DEPENDS. While the target based way of including libraries is a the modern way, passing the variables should work just as fine.

tomlankhorst commented 4 years ago

So the dependency is commented out now because the library does not actually do anything with it. I think it adds to this example to do use Eigen.

Now, if Eigen is only used in source files it will not be a transitive dependency so we should not add it to the DEPENDS, do you agree?

peci1 commented 4 years ago

Yes, if it's only build dependency and isn't anywhere in .h files, then it shouldn't be in DEPDENDS. But maybe a comment could be there explaning when to add it there.