-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make internal dependencies private #60
Conversation
rcpputils, rcutils, and spdlog don't need to be exposed to downstream users of this package. Signed-off-by: Shane Loretz <[email protected]> Signed-off-by: Shane Loretz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change seems reasonable to me, as long as we test --packages-above rcl_logging_spdlog
.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending green CI
+1 |
Signed-off-by: Shane Loretz <[email protected]>
Added noop change in 1bce156. I didn't do |
Did you mean build packages above? If this broke something I would expect it to show up at build time. I also put CI (build: |
rcpputils
,rcutils
, andspdlog
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 becausercl
already depends on both and has custom CMake logic tofind_package()
it.Fixes #58
Uses
target_link_libraries()
because all of its dependencies are using targets already ament/ament_cmake#292