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

Move definition logger to cpp to avoid "multiple definition" linker error #21

Conversation

tpoignonec
Copy link
Contributor

Hi!

I ran into a little linking issue caused by the kinematics_interface package, more precisely by the definition of rclcpp::Logger LOGGER in kinematics_interface.hpp.

This repos contains the minimal example to reproduce the error, but in a nutshell:

  • Header class_a.hpp of ClassA includes kinematics_interface/kinematics_interface.hpp;
  • Header class_b.hpp of ClassB includes class_a.hpp

This setups leads to multiple definitions of rclcpp::Logger LOGGER and ultimatly to a linker error:

/usr/bin/ld: CMakeFiles/test_kinematics_interface.dir/src/class_b.cpp.o:(.bss+0x0): multiple definition of `kinematics_interface::LOGGER'; CMakeFiles/test_kinematics_interface.dir/src/class_a.cpp.o:(.bss+0x0): first defined here
collect2: error: ld returned 1 exit status

To avoid this, could you split the code between header and cpp as proposed in the PR?

Note that in there, the rclcpp::Logger is declared as extern, but I guess the declaration of the logger in the header could actually be removed if not used by inherited interfaces (e.g., the KDL one redefine its own).
Alternatively, if it really has to be a header-only package, maybe the variable could just be removed or hidden in a get_logger() function.

What do you think?

Thanks!

   - define logger as extern in header
   - define logger in cpp
   - update CMakeLists
@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

All modified lines are covered by tests ✅

❗ No coverage uploaded for pull request base (master@1408f86). Click here to learn what that means.

❗ Current head 75f6226 differs from pull request most recent head d519e9b. Consider uploading reports for the commit d519e9b to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##             master      #21   +/-   ##
=========================================
  Coverage          ?   31.40%           
=========================================
  Files             ?        3           
  Lines             ?      207           
  Branches          ?      134           
=========================================
  Hits              ?       65           
  Misses            ?       25           
  Partials          ?      117           
Flag Coverage Δ
unittests 31.40% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me.

@bmagyar bmagyar merged commit b7958c3 into ros-controls:master Nov 16, 2023
1 check passed
@tpoignonec tpoignonec deleted the tpoignonec-move_definition_logger_to_cpp branch November 27, 2023 07:42
@christophfroehlich
Copy link
Contributor

@Mergifyio backport humble

Copy link

mergify bot commented Feb 5, 2024

backport humble

✅ Backports have been created

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.

None yet

4 participants