PRIVATE option to ament_target_dependencies does not work

[felixf4xu]

#161 (comment)

PRIVATE and INTERFACE keywords aren't allowed.

I'd like to know why?

If only PUBLIC is allowed and it's used, I can only use public for all other dependencies, even for my PRIVATE library, which leads to a problem.

For example, visualization_msgs is my private library:

ament_target_dependencies(lanelet2_extension_lib PUBLIC
  visualization_msgs
)

but if I don't call

ament_export_dependencies(visualization_msgs) 

The user of my lib package will have this error:

The link interface of target "lanelet2_extension::lanelet2_extension_lib"
  contains:

    visualization_msgs::visualization_msgs__rosidl_generator_c

  but the target was not found.  Possible reasons include:

[clalancette]

I'd like to know why?

Nowadays, INTERFACE is also allowed, but PRIVATE still isn't.

Overall, we are (slowly) moving away from ament_target_dependencies, and instead just use target_link_libraries everywhere. This is one of the reasons; target_link_libraries can do everything that we need, and more. So my suggestion is that for these private dependencies, you switch to using target_link_libraries instead, and then you won't have to export them to downstream consumers.

[EzraBrooks]

@clalancette would it make sense to add a warning to ament_target_dependencies about it being deprecated or something?

I think the average ROS developer probably doesn't understand the distinction between the various visibilities of linking in CMake and this is contributing to strange transitive dependency issues and build times across the ecosystem. This wouldn't be solved by pushing people to target_link_libraries, but it would mean that when people do discover the distinction for themselves, they at least mostly have a find-and-replace/trial-and-error means of moving to PRIVATE since they'll already be using a function that supports it.

Adding a deprecation warning might be a good way to accelerate a community transition to more modern CMake with target_link_libraries.

[clalancette]

@clalancette would it make sense to add a warning to ament_target_dependencies about it being deprecated or something?

Unfortunately, not yet. There is one fairly major problem still remaining in using target_link_libraries, and that has to do with message packages. Right now, you can't do something like:

target_link_libraries(mylib PUBLIC std_msgs::std_msgs)

Because the std_msgs::std_msgs target doesn't have all of the correct dependencies. The following does work:

target_link_libraries(mylib PUBLIC ${std_msgs_TARGETS})

And indeed, that is what we do in the core libraries. However, having all things that link to message packages have to do this kind of defeats the purpose of using only CMake targets everywhere. Once that bug is fixed, then yes, I would think we would want to do a deprecation warning.

I think the average ROS developer probably doesn't understand the distinction between the various visibilities of linking in CMake and this is contributing to strange transitive dependency issues and build times across the ecosystem.

Can you explain more about this? ament_target_dependencies should be more-or-less equivalent to target_link_libraries(mylib dep). Also, this is the first time I'm hearing about these strange transitive dependency issues. Build times are improved by using target_link_libraries, but in my experience, not substantially unless you have a pre-existing bug.

[EzraBrooks]

Yeah, I've been working on moving my codebase from ament_target_dependencies to target_link_libraries over the last week or so and definitely immediately ran into the message package issue. I wrote myself a function that does this:

foreach(package IN ITEMS ${ARGN})
  if(TARGET ${package}::${package})
    target_link_libraries(${target} ${visibility} ${package}::${package})
  elseif(DEFINED ${package}_INCLUDE_DIRS OR DEFINED ${package}_LIBRARIES)
    target_include_directories(${target} ${visibility}
                               ${${package}_INCLUDE_DIRS})
    target_link_libraries(${target} ${visibility} ${${package}_LIBRARIES})
  else()
    message(
      FATAL_ERROR
        "Package '${package}' doesn't provide modern CMake targets or legacy variables"
    )
  endif()
endforeach()

that I'm using as an intermediary step between ament_target_dependencies and target_link_libraries when I find libraries that don't expose a target properly.

By "strange transitive dependency issues" I just mean you're a lot more likely to run into partially-specified dependency graphs if you're linking/including everything PUBLIC since each target might be bringing in headers and shared objects it's not intending to since everything is PUBLIC all the way up the dependency chain. I guess theoretically this could also create unnecessary shared object / header version conflicts, but I'm not sure I've seen that symptom. The dependency graph issue is mostly a pain in larger codebases when refactoring. Probably not a huge deal for most people, but I've seen it rear its head in both MoveIt open source and MoveIt Pro.

Build times, though, are very painful in both MoveIt Pro and Space ROS.