use modern interface targets if available, otherwise classic variables

[dirk-thomas]

Follow up of #230

Use modern interface targets when available for a dependency, otherwise fall back to the classic variables. This will allow a bottom-up approach to update packages without breaking downstream packages.

• Linux
• Linux-aarch64
• macOS
• Windows

[dirk-thomas]

The CI builds without the INTERFACE keyword also include ros2/rcutils#221:

• Linux
• Linux-aarch64
• macOS
• Windows

[christophebedard]

@dirk-thomas I know this isn't the best time or place for questions like this, but this PR seems to have broken a call to/use of ament_export_link_flags() in tracetools which resulted in a test failure in tracetools_test (which unfortunately only runs on our own CI) https://gitlab.com/micro-ROS/ros_tracing/ros2_tracing/-/issues/81

I assume that tracetools' CMake setup was somewhat invalid and that this simply made it finally break, but I cannot figure what is wrong. I've tried to apply changes (see https://gitlab.com/micro-ROS/ros_tracing/ros2_tracing/-/merge_requests/162) similar to https://github.com/ros2/rcutils/pull/221/files but to no avail.

[dirk-thomas]

@christophebedard Almost all of the patch of https://gitlab.com/micro-ROS/ros_tracing/ros2_tracing/-/merge_requests/162 seems unnecessary:

• you can keep using PROJECT_SOURCE_DIR etc.
• the export name can stay as ${PROJECT_NAME}_export
• the install() directive doesn't need to be changed / moved

The only necessary from the patch is:

• remove include_directories() in favor of target_include_directories(... $<...> ...)

The one thing missing from the exported interface/target is:

• declaring that -rdynamic is necessary to use the library, the following should do it (haven't tried it though):

  target_link_libraries(${PROJECT_NAME} PUBLIC "-rdynamic")

[christophebedard]

@dirk-thomas yeah, most of that was just me trying stuff.

However,

remove include_directories() in favor of target_include_directories(... $<...> ...)

I've tried that:

diff --git a/tracetools/CMakeLists.txt b/tracetools/CMakeLists.txt
index 3de8d4d..1405ddb 100644
--- a/tracetools/CMakeLists.txt
+++ b/tracetools/CMakeLists.txt
@@ -68,11 +68,6 @@ foreach(_header ${HEADERS})
   )
 endforeach()
 
-# Only use output/binary include directory
-include_directories(
-  ${PROJECT_BINARY_DIR}/include
-)
-
 add_library(${PROJECT_NAME} ${SOURCES})
 if(TRACETOOLS_LTTNG_ENABLED)
   target_link_libraries(${PROJECT_NAME} ${LTTNG_LIBRARIES})
@@ -83,6 +78,10 @@ if(WIN32)
   target_compile_definitions(${PROJECT_NAME} PRIVATE "TRACETOOLS_BUILDING_DLL")
 endif()
 
+# Only use output/binary include directory
+target_include_directories(${PROJECT_NAME} PUBLIC
+  "$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/include>"
+  "$<INSTALL_INTERFACE:include>")
 ament_export_interfaces(${PROJECT_NAME}_export HAS_LIBRARY_TARGET)
 
 # Status checking tool

and it also doesn't work, see: https://gitlab.com/micro-ROS/ros_tracing/ros2_tracing/-/jobs/504379922#L1638

Either there's something missing in the above CMake config, or this PR did break something.

The one thing missing from the exported interface/target is:

• declaring that -rdynamic is necessary to use the library, the following should do it (haven't tried it though):
  target_link_libraries(${PROJECT_NAME} PUBLIC "-rdynamic")

That does work, and I can switch to that, but then why do we have ament_export_link_flags() (which doesn't seem to work here)? Shouldn't it work for this?

[dirk-thomas]

The exported targets use modern CMake where all the other export functions use classic CMake style. If you export a modern target (which will be available as an interface target to downstream packages) that target must contain all the necessary information. In your case your exported interface / target was incomplete.

Until now that only worked because both information was used together which isn't the case anymore. If targets are exported only those are now being used so that the classic CMake style can at some point be deprecated / removed.

[christophebedard]

I see. The fact that it used to work but does not anymore is unfortunate, but, like you said, my CMake config wasn't actually complete/valid. Thank you for the explanation and tips.

[dirk-thomas]

Related to ros2/ros2#904.