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

rcl_logging_interface is only valid path with build environment. #122

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

fujitatomoya
Copy link
Collaborator

closes #121

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Copy link
Collaborator Author

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either @clalancette or @ahcorde can you take a look?

Copy link

@gergondet-woven gergondet-woven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick response @fujitatomoya I think that would do it for #121

Could it be cherry-picked to jazzy once it's merged? Thanks!

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC because rcl_logging_spdlog uses target_link_libraries(... PUBLIC) with a target from rcl_logging_interface it needs to export the dependency on rcl_logging_interface. The package.xml looks ok, but the call to ament_export_dependencies(rcl_logging_interface) line was removed in #102

If adding ament_export_dependencies(rcl_logging_interface) does not resolve #121 then there's likely a bug in the CMake scraping done by drake-ros/bazel_ros2_rules.

@fujitatomoya
Copy link
Collaborator Author

@gergondet-woven

I think i was misunderstanding, I am not 100% sure but I think @sloretz 's comment #122 (review) is correct. rcl_logging_spdlog needs to export the dependency on rcl_logging_interface, so putting ament_export_dependencies(rcl_logging_interface) back to see if the problem solves?

@gergondet-woven
Copy link

so putting ament_export_dependencies(rcl_logging_interface) back to see if the problem solves?

Yes, this would work as well and it's even more correct.

I have also confirmed that I had no scrapping issues in that case since the rcl_logging_interface::rcl_logging_interface is correctly imported before the rcl_logging_spdlog defines its targets.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator Author

@sloretz @gergondet-woven could you review one more time?

Copy link

@gergondet-woven gergondet-woven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@ahcorde
Copy link
Contributor

ahcorde commented Oct 1, 2024

Pulls: #122
Gist: https://gist.githubusercontent.com/ahcorde/d01cd595cc9a1be19db5cdb3f823f200/raw/b71a04a9d35d807536727ae4a258d53cd5b6fdbb/ros2.repos
BUILD args: --packages-above-and-dependencies rcl_logging_noop rcl_logging_spdlog --packages-above-and-dependencies rcl_logging_noop rcl_logging_spdlog
TEST args: --packages-above rcl_logging_noop rcl_logging_spdlog --packages-above rcl_logging_noop rcl_logging_spdlog
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14635

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@clalancette clalancette merged commit 0724aeb into rolling Oct 1, 2024
3 checks passed
@clalancette clalancette deleted the fujitatomoya/fix-build-dependency branch October 1, 2024 21:03
@clalancette
Copy link
Contributor

@Mergifyio backport jazzy iron humble

Copy link

mergify bot commented Oct 1, 2024

backport jazzy iron humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Oct 1, 2024
* logging impls need to export dependency for 'rcl_logging_interface'.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 0724aeb)
mergify bot pushed a commit that referenced this pull request Oct 1, 2024
* logging impls need to export dependency for 'rcl_logging_interface'.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 0724aeb)

# Conflicts:
#	rcl_logging_noop/CMakeLists.txt
#	rcl_logging_spdlog/CMakeLists.txt
mergify bot pushed a commit that referenced this pull request Oct 1, 2024
* logging impls need to export dependency for 'rcl_logging_interface'.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 0724aeb)

# Conflicts:
#	rcl_logging_noop/CMakeLists.txt
#	rcl_logging_spdlog/CMakeLists.txt
ahcorde pushed a commit that referenced this pull request Oct 2, 2024
… (#123)

* logging impls need to export dependency for 'rcl_logging_interface'.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 0724aeb)

Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
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.

Issue with the generated CMake config files
5 participants