In short:
Is it advisable to replace ament_target_dependencies with target_link_libraries in patch releases for Kilted?
Yes it is advisable.
Or should we leave existing Kilted packages as-is and only apply the fix in rolling or future releases?
You should apply fixes to Kilted. It’s probably fine to apply fixes as far back as ROS Foxy.
Does replacing ament_target_dependencies with target_link_libraries impact downstream users in a way that might break their builds or linkage?
AFAIK, your use of ament_target_dependencies versus target_link_libraries does not impact downstream users.
Since which distro is it safe to substitute ament_target_dependencies?
Foxy-ish
In long…
The problem: build some software
Let me start with a short story.
Some dude has a top-secret high-impact C++ project. It’s is going to change the world. Here it is:
# include <tinyxml2.h>
# include <iostream>
int main (int argc, char **argv) {
if (argc != 2) {
std::cerr << "Usage " << argv[0] << " some_world.sdf\n";
return 1;
}
tinyxml2::XMLDocument doc;
tinyxml2::XMLError result = doc.LoadFile(argv[1]);
tinyxml2::XMLElement* root = doc.RootElement();
if (!root) {
std::cerr << "Invalid SDFormat\n";
return 1;
}
tinyxml2::XMLElement* world = root->FirstChildElement("world");
if (!world) {
std::cerr << "Invalid SDFormat world\n";
return 1;
}
world->SetAttribute("name", "Shane's world");
doc->Print();
}
Some dude compiles it with:
g++ world_domination.cpp -I /usr/include -L/usr/lib/x86_64-linux-gnu -ltinyxml2 -std=c++17 -oworld_domination
However, this only builds on Ubuntu. Any serious world domination effort is going to need cross platform support, at least until the year of the linux desktop arrives.
Some dude wonders if world domination would be easier with CMake.
cmake_minimum_required(VERSION 3.15)
project(world_domination VERSION 1.0.0)
find_package(tinyxml2 REQUIRED)
add_executable(world_domination world_domination.cpp)
target_link_libraries(world_domination tinyxml2::tinyxml2)
mkdir build
cd build
cmake ..
cmake --build .
Great. It’s a bit more complex to use CMake, but cross-platfom world domination makes it worth it.
The world is conquered and no one ever has any problems again.
The end
My point is that CMake’s job is compile your software.
CMake needs to give the compiler:
- Include directories, like
/usr/include
- Link directories, like
/usr/lib/x86_64-linux-gnu
- Libraries to link against, like
tinyxml2
- Misc compiler flags, like
-std=c++17
So, how does CMake know what to give to the compiler?
The solution: modern CMake targets
This line tells CMake world_domination depends on tinyxml2.
target_link_libraries(world_domination tinyxml2::tinyxml2)
The text tinyxml2::tinyxml2 is the name of a CMake target. CMake targets have target properties. Some of tinyxml2::tinyxml2’s properties tell CMake what it needs to build world_domination. For example:
When you call target_link_libraries() with CMake targets, you’re telling CMake to use all the information provided by those targets to build your software.
The old solution: CMake variables
Long ago in a galaxy far far away, CMake expected you store this information in CMake variables.
These variables had conventional names that most-ish packages used. tinyxml2 might have put that info into variables like:
tinyxml2_INCLUDE_DIRS for include directories, like /usr/includetinyxml2_LIBRARIES with full paths to libraries like /usr/lib/x86_64-linux-gnu/libtinyxml2.sotinyxml2_HECK_IF_I_KNOW for misc compiler flags, like -std=c++17
You might tell CMake about those variables with calls like
target_include_directories(world_domination PRIVATE ${tinyxml2_INCLUDE_DIRS})
target_link_libraries(world_domination PRIVATE ${tinyxml2_LIBRARIES})
target_compile_options(world_domination PRIVATE ${tinyxml2_HECK_IF_I_KNOW})
Or, maybe you’d give CMake that information with even older calls like:
Old-style CMake variables have some problems, but I’m going to resist explaining them.
Weren’t we talking about ament_target_dependencies?
So ament_target_dependencies() did two things:
It was supposed to be convenient to use ament_target_dependencies(). However, not all packages used the same “conventional names”, and so couldn’t be used with ament_target_dependencies(). Then it got worse when our dependencies stopped providing old-style CMake variables. We tried to work around this, even creating our own wrappers to export old-style CMake variables. Then we made ament_target_dependencies() support modern CMake targets if the targets were put into a CMake variable called *_INTERFACES. However, the _INTERFACES was just a ROS convention. Our dependencies didn’t have to provide a variable with that name. By ROS Foxy we made most ROS packages export modern CMake targets too.
Catkin tried to make it possible to override packages. If the installed version of roscpp had a bug, then you could put a newer version of roscpp into your workspace and use that. Catkin made this possible by carefully sorting include directories before giving them to the compiler. All you had to do was find those dependencies with find_package(catkin REQUIRED COMPONENTS foo bar etc). Frankly overriding packages doesn’t work, not even in ROS 1, except in very fragile circumstances. Still, ament_target_dependencies() tried to support it by re-ordering include directories according to ament and colcon workspaces. Then we discovered modern CMake targets took away our ability to control the order of information passed to the compiler from transitive dependencies. We solved that in ROS Humble by installing headers into unique include directories. This made the sorting in ament_target_dependencies() unnecessary.
In summary, chances are the target_link_libraries() call in ament_target_dependencies() has been the only thing doing useful work for some time now. The rest was just making your build slower.
Let’s see if Bazel, Cargo and others jump in 
