[rcl_logging_spdlog] remove dep on spdlog (and only have it on spdlog_vendor)?

[ahcorde]

There are two kind of vendor packages in ROS 2.

• The ones that try to use a installed version of the library in the system, such as: poco_vendor or this recent PR in libyaml_vendor
• The packages that build the library no matter what you have in your system: yaml-cpp, spdlog, assimp, tinyxml, etc.

My question here is should we try to get rid off the second find_package or don't we mind the find the library twice ?

Here you can find an example what I mean with "find the library twice":

find_package(spdlog_vendor REQUIRED) # Provides spdlog 1.3.1 on platforms without it.
find_package(spdlog REQUIRED)

This question is extensible to poco, yaml, yaml-cpp and the other vendor packages.

@wjwwood and @dirk-thomas what do you think?

[wjwwood]

I'm not sure. I can see benefit in both approaches. Just find packaging the vendor is nice because no matter where the dependency comes from, or what version it is, the vendor package can provide a uniform interface to it. But on the other hand, it's more transparent to find package the actual dependency and use variables from that, it's more "natural", especially if the dependency already provides a CMake module.

There are two cases where always using the vendor is nice, imo:

• if the dependency doesn't provide cmake infrastructure, then the vendor package provides this always
  • a similar situation is that sometimes the system library (installed from apt) doesn't have a cmake module, but newer versions that we would build do (I think eigen had this?)
• if the cmake module from the dependency is deficient in some way
  • e.g. I have custom logic for interacting with Ogre to work around problems in their cmake logic:
  • https://github.com/ros2/rviz/blob/5284d2bde7d4790c2d65706a2479dffb25f65b39/rviz_ogre_vendor/rviz_ogre_vendor-extras.cmake.in#L30-L290

It also avoids any extra logic needed to use the upstream dependency from being duplicated in downstream packages (the ogre example is good for this again), because this can be encapsulated in the vendor package's logic.

The downside to this is that if you ever wanted to move away from the vendor package in the future, it would be harder to do so. Personally, I don't mind the vendor packages, but that's been a concern in the past.

[clalancette]

As far as I can tell, spdlog_vendor does try to use the system library if it is available: https://github.com/ros2/spdlog_vendor/blob/4683c3926f432cdc07ada6792c5e63142d82f000/CMakeLists.txt#L77 . But I'm not sure if that substantially changes the question.

@ahcorde is there anything in regards to rcl_logging that you think we would do here?

[ahcorde]

nope, we can close this issue