search

ros2 / rcl_logging

Logging implementations for ROS 2.
Apache License 2.0
18 stars 34 forks source link

Make internal dependencies private #60

Closed sloretz closed 3 years ago

sloretz commented 3 years ago

rcpputils, rcutils, and spdlog don't need to be exposed to downstream users of this package at their build time because these dependencies are only used internally.

I think rcl_logging_interface should still be exposed because this package only appears to be useful if one has those headers, but that won't affect how this is currently used downstream because rcl already depends on both and has custom CMake logic to find_package() it.

Fixes #58

Uses target_link_libraries() because all of its dependencies are using targets already ament/ament_cmake#292

hidmic commented 3 years ago

I will also say that since we are doing this here, we should probably do a similar thing in rcl_logging_log4cxx and rcl_logging_noop.

+1

sloretz commented 3 years ago

I will also say that since we are doing this here, we should probably do a similar thing in rcl_logging_log4cxx and rcl_logging_noop

Added noop change in 1bce156. I didn't do log4cxx because it's using a find module with old-style standard CMake variables, and ament_target_dependencies() doesn't have a "PRIVATE" option.

sloretz commented 3 years ago

The change seems reasonable to me, as long as we test --packages-above rcl_logging_spdlog

Did you mean build packages above? If this broke something I would expect it to show up at build time. I also put rcl in the test args since it has tests for logging.

CI (build: --packages-above-and-dependencies rcl_logging_noop rcl_logging_spdlog test: --packages-select rcl_logging_noop rcl_logging_spdlog rcl)