Skip to content
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

Merged
merged 2 commits into from
Oct 21, 2020
Merged

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Oct 20, 2020

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

rcpputils, rcutils, and spdlog don't need to be exposed to downstream
users of this package.

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz self-assigned this Oct 20, 2020
@sloretz sloretz requested a review from hidmic October 20, 2020 22:17
Copy link
Contributor

@clalancette clalancette left a 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.

Copy link
Contributor

@hidmic hidmic left a 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

@hidmic
Copy link
Contributor

hidmic commented Oct 21, 2020

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

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Contributor Author

sloretz commented Oct 21, 2020

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
Copy link
Contributor Author

sloretz commented Oct 21, 2020

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)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@sloretz sloretz merged commit 23841ef into master Oct 21, 2020
@delete-merged-branch delete-merged-branch bot deleted the dont_export_private_deps branch October 21, 2020 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spdlog dependency shouldn't need to be exported
3 participants