cmake exported target options cause problem with project using gtsam

[Maciej Matuszak]

Hi, I use the gtsam in CUDA based project. Recent changes by pull req #372 switch to modern cmake target based options, definitions and libraries. This is great move however I think you need to evaluate what is public and private. Make public only what is really needed. The problem I come across is the flag "-Wno-unused-local-typedefs". It is added to GTSAM_COMPILE_OPTIONS only when compiler version supports it however later on the GTSAM_COMPILE_OPTIONS is exposed as public: https://bitbucket.org/gtborg/gtsam/src/2a0153cf255205241b1c300aea78ba9fe0d8c03c/gtsam/CMakeLists.txt#lines-107

This means users of gtsam are forced to use "-Wno-unused-local-typedefs". In my case the cuda compiler nvcc does not understand this option and dies. I think this option should be made private. @jlblancoc opinion?

[Maciej Matuszak]

I think I would not collect all the options into GTSAM_COMPILE_OPTIONS list but rather use target_compile_options directly, deciding on the spot what is private what is public.

[José Luis Blanco-Claraco]

Hi, You're absolutely right!

Could you please give it a test to the (pending) PR #379 ? I will append a fix to this issue to that PR.

[José Luis Blanco-Claraco]

Done. Give it a try...

Cheers

[Maciej Matuszak]

I tested your PR branch, looks good. I just went trough cmake modernisation exercise for my project. I put find_package into the installed ...Config.cmake for every public library dependency (target_link_libraries(<target_name> PUBLIC ...) I notice gtsam only has entry for boost in Config.cmake but there is plenty more public libs. Is that because those libraries are optional?

[José Luis Blanco-Claraco]

Good point. Ideally, all deps should go through find_package() in xxx-config.cmake, but... not all external projects support cmake exported targets!

Boost does, so it's included there.

Eigen does... except in older Ubuntu Xenial, I think. To avoid problems, and since it's a pure header lib, it's enough to just add it via PUBLIC include_directories of the exported gtsam target.

TBB, SuiteSparse, etc. doesn't support exported targets (AFAIK), so they are just exported as PUBLIC include & link deps of the exported gtsam targets.

So, not the ideal clean solution, but the alternative is to go through all those dependencies and "modernize" them too... :-)

[Maciej Matuszak]

Thanks @jlblancoc for explanation! Yes this is the cost of flexibility!

[Thomas Horstink]

Hi Jose,

I took the most recent development and I had cmake -> boost issues when linking to gtsam;

For each executable/library that is linked to gtsam it now produces the following error:

CMake Error at odometry/CMakeLists.txt:61 (add_executable):
  Target "odometry_test" links to target "Boost::thread" but the target was
  not found.  Perhaps a find_package() call is missing for an IMPORTED
  target, or an ALIAS target is missing?

I also recompiled using your PR#379 and it indeed fixed the above.. I hope your fix gets merged into dev =-) I think currently it breaks a lot of installations.

Is it also a good idea to update the example project (which has an example cmake), to reflect best practices?

[Thomas Horstink]

clarification: I think the current dev (since the cmake modernization/cleanup) breaks a lot of installations.

[José Luis Blanco-Claraco]

Ok!

@dellaert: All this discussion means:

• PR 379 can be merged, then

• The merge will solve this issue (#425), which can be closed.

Cheers.

[Frank Dellaert]

Merged in jlblancoc/gtsam/more-cmake-modernizations (pull request #379) A lot of cmake modernizations as well as cleanup some cmake issues/docs. close issue #425 Approved-by: Chris Beall [e-mail]

→ <<cset 40d67f49dac5>>