Statically allocate string concatenations using FMT formatting #2205

mini-1235 · 2025-04-26T15:46:27Z

closes #1906

saikishor

@mini-1235 can you check other parts of code where these kinds of strings are built and can you replace them with fmt ?

Thank you

mini-1235 · 2025-04-26T16:10:51Z

@mini-1235 can you check other parts of code where these kinds of strings are built and can you replace them with fmt ?

Thank you

@saikishor Do I need to do this for the tests as well?

saikishor · 2025-04-26T16:23:46Z

@saikishor Do I need to do this for the tests as well?

I don't think it's needed for the tests!

mergify · 2025-04-26T19:01:57Z

This pull request is in conflict. Could you fix it @mini-1235?

mini-1235 · 2025-04-26T21:13:15Z

I can see that the CI is reporting fmt not linking properly which I could not reproduce on my local machine, I will try to fix according to the CI

bmagyar · 2025-04-26T21:28:43Z

For the CI related issues this package may be a good resource to look into: https://github.com/facontidavide/ros2_logging_fmt

Probably there are some hints to find here leading to a solution.

saikishor · 2025-04-26T22:17:44Z

/usr/bin/ld: CMakeFiles/test_joint_handle.dir/test/test_handle.cpp.o: in function `std::make_unsigned<long>::type fmt::v9::detail::to_unsigned<long>(long)':
  test_handle.cpp:(.text._ZN3fmt2v96detail11to_unsignedIlEENSt13make_unsignedIT_E4typeES4_[_ZN3fmt2v96detail11to_unsignedIlEENSt13make_unsignedIT_E4typeES4_]+0x25): undefined reference to `fmt::v9::detail::assert_fail(char const*, int, char const*)'
  collect2: error: ld returned 1 exit status
  gmake[2]: *** [CMakeFiles/test_joint_handle.dir/build.make:271: test_joint_handle] Error 1
  gmake[1]: *** [CMakeFiles/Makefile2:303: CMakeFiles/test_joint_handle.dir/all] Error 2
  gmake[1]: *** Waiting for unfinished jobs....

Seems like there is a linking issue. It is missing as a dependency or in the CMakeLists.txt
Please take a look

christophfroehlich

Thanks for the great work, I have some comments about dependencies.

I never used fmt, so I might be wrong: Which strings should be allocated with fmt? Every string where any variable is included with format specifiers, or +operator of std::string? I made a quick search and still found some occurrences without fmt
controller_manger.cpp: L117, L130, L137, L591, L598, L607, L1357, L1378, L1380, L1424, L1430, L1520, L1574, L1664, L1677, L1690, etc
I suggest searching once again for %s, %d, %f, %lu, throw?

hardware_interface/package.xml

hardware_interface_testing/package.xml

christophfroehlich · 2025-04-27T18:26:18Z

hardware_interface/include/hardware_interface/component_parser.hpp

@@ -15,10 +15,10 @@
 #ifndef HARDWARE_INTERFACE__COMPONENT_PARSER_HPP_
 #define HARDWARE_INTERFACE__COMPONENT_PARSER_HPP_

+#include <fmt/compile.h>
 #include <string>
 #include <unordered_map>
 #include <vector>


please re-add this new-line (google code style)

Do you mean two new lines after #define? Or a new line after `<fmt/compile.h>?

I have reformat the header files in the latest commit, can you please check if that is what you mean @christophfroehlich ?

joint_limits/package.xml

transmission_interface/package.xml

controller_manager/src/controller_manager.cpp

saikishor

In general, I don't think we need to add target_link_libraries of fmt::fmt every where, in most cases adding to THIS_PACKAGE_INCLUDE_DEPENDS alone should do, as it is being used in the ament_target_dependencies

I also observed the missing include of fmt in the controller_interface and controller_manager along with the dependency in package.xml and changes in CMakeLists.txt -> We need this as we want to be with "include-what-you-use" principle

hardware_interface/CMakeLists.txt

saikishor · 2025-04-28T10:15:05Z

hardware_interface/CMakeLists.txt

+  target_link_libraries(test_joint_handle hardware_interface fmt::fmt)
+  ament_target_dependencies(test_joint_handle rcpputils fmt)


Suggested change

target_link_libraries(test_joint_handle hardware_interface fmt::fmt)

ament_target_dependencies(test_joint_handle rcpputils fmt)

target_link_libraries(test_joint_handle hardware_interface)

ament_target_dependencies(test_joint_handle rcpputils)

hardware_interface/CMakeLists.txt

hardware_interface_testing/CMakeLists.txt

saikishor · 2025-04-28T10:17:32Z

joint_limits/CMakeLists.txt

@@ -73,13 +74,14 @@ if(BUILD_TESTING)
  find_package(rclcpp REQUIRED)

  ament_add_gmock(joint_limits_rosparam_test test/joint_limits_rosparam_test.cpp)
-  target_link_libraries(joint_limits_rosparam_test joint_limits)
+  target_link_libraries(joint_limits_rosparam_test joint_limits fmt::fmt)


Suggested change

target_link_libraries(joint_limits_rosparam_test joint_limits fmt::fmt)

target_link_libraries(joint_limits_rosparam_test joint_limits)

fixed, but looks like it needs a link in joint_limits, so I have addedtarget_link_libraries(joint_limits INTERFACE fmt::fmt), otherwise it will report

--- stderr: joint_limits /usr/bin/ld: CMakeFiles/joint_limits_rosparam_test.dir/test/joint_limits_rosparam_test.cpp.o: undefined reference to symbol '_ZN3fmt2v96detail11assert_failEPKciS3_' /usr/bin/ld: /lib/x86_64-linux-gnu/libfmt.so.9: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status gmake[2]: *** [CMakeFiles/joint_limits_rosparam_test.dir/build.make:222: joint_limits_rosparam_test] Error 1 gmake[1]: *** [CMakeFiles/Makefile2:270: CMakeFiles/joint_limits_rosparam_test.dir/all] Error 2 gmake: *** [Makefile:146: all] Error 2

saikishor · 2025-04-28T10:17:43Z

joint_limits/CMakeLists.txt


  ament_add_gmock(joint_limits_urdf_test test/joint_limits_urdf_test.cpp)
-  target_link_libraries(joint_limits_urdf_test joint_limits)
+  target_link_libraries(joint_limits_urdf_test joint_limits fmt::fmt)


Suggested change

target_link_libraries(joint_limits_urdf_test joint_limits fmt::fmt)

target_link_libraries(joint_limits_urdf_test joint_limits)

joint_limits/CMakeLists.txt

transmission_interface/CMakeLists.txt

mini-1235 · 2025-05-01T15:34:07Z

I never used fmt, so I might be wrong: Which strings should be allocated with fmt? Every string where any variable is included with format specifiers, or +operator of std::string?

I think allocating strings are only required when concatenating? Those only with format specifiers like

 RCLCPP_DEBUG(
      rclcpp::get_logger("ControllerManager::utils"),
      "Required interface '%s' with prefix '%s' is not a chain interface.", interface_name.c_str(),
      interface_prefix.c_str());

are not required, right? @saikishor

saikishor · 2025-05-02T08:56:08Z

I think allocating strings are only required when concatenating? Those only with format specifiers like
 RCLCPP_DEBUG(
      rclcpp::get_logger("ControllerManager::utils"),
      "Required interface '%s' with prefix '%s' is not a chain interface.", interface_name.c_str(),
      interface_prefix.c_str());
are not required, right? @saikishor

Yes, it is not required for such cases

mergify · 2025-05-02T15:36:47Z

This pull request is in conflict. Could you fix it @mini-1235?

mini-1235 · 2025-05-02T15:49:52Z

Sorry for the delayed reply here, I was building a new image on my local machine so that I can reproduce the errors reported by the CI :)

I suggest searching once again for %s, %d, %f, %lu, throw?

My searching pattern was + " / " +, I think that is enough to find all concatenation cases(?)

I also observed the missing include of fmt in the controller_interface and controller_manager along with the dependency in package.xml and changes in CMakeLists.txt -> We need this as we want to be with "include-what-you-use" principle

Added.

saikishor · 2025-05-02T16:13:48Z

@mini-1235 can you rebase your changes on top of current master?

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

saikishor

@mini-1235 most of the files that have the fmt changes are missing the include of the header. Remember the "include_what_you_use" principle.

Also please fix the pre-commit

controller_interface/src/chainable_controller_interface.cpp

controller_manager/package.xml

saikishor · 2025-05-02T21:00:07Z

joint_limits/CMakeLists.txt

@@ -27,6 +28,7 @@ target_include_directories(joint_limits INTERFACE
  $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>
  $<INSTALL_INTERFACE:include/joint_limits>
 )
+target_link_libraries(joint_limits INTERFACE fmt::fmt)


Suggested change

target_link_libraries(joint_limits INTERFACE fmt::fmt)

Isn't the ament_target_dependencies doing the same thing?

If I commented out this line, I can see that the compiler reports a linking error:

--- stderr: joint_limits /usr/bin/ld: CMakeFiles/joint_limits_rosparam_test.dir/test/joint_limits_rosparam_test.cpp.o: undefined reference to symbol '_ZN3fmt2v96detail11assert_failEPKciS3_' /usr/bin/ld: /lib/x86_64-linux-gnu/libfmt.so.9: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status gmake[2]: *** [CMakeFiles/joint_limits_rosparam_test.dir/build.make:222: joint_limits_rosparam_test] Error 1 gmake[1]: *** [CMakeFiles/Makefile2:270: CMakeFiles/joint_limits_rosparam_test.dir/all] Error 2 gmake: *** [Makefile:146: all] Error 2

I will discover why

I just rechecked it locally. you are right. I think your initial approach makes sense here. Just add it to the rosparam test directly :). Sorry for the confusion

target_link_libraries(joint_limits_rosparam_test joint_limits fmt::fmt)

Surprisingly, if I remove the line

target_link_libraries(joint_limits INTERFACE fmt::fmt)

I can see more tests report the linking problem:

gmake[2]: *** [CMakeFiles/test_joint_handle.dir/build.make:271: test_joint_handle] Error 1 gmake[1]: *** [CMakeFiles/Makefile2:305: CMakeFiles/test_joint_handle.dir/all] Error 2 gmake[1]: *** Waiting for unfinished jobs.... /usr/bin/ld: CMakeFiles/test_component_interfaces_custom_export.dir/test/test_component_interfaces_custom_export.cpp.o: undefined reference to symbol '_ZN3fmt2v96detail11assert_failEPKciS3_' /usr/bin/ld: /lib/x86_64-linux-gnu/libfmt.so.9: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status gmake[2]: *** [CMakeFiles/test_component_interfaces_custom_export.dir/build.make:271: test_component_interfaces_custom_export] Error 1 gmake[1]: *** [CMakeFiles/Makefile2:389: CMakeFiles/test_component_interfaces_custom_export.dir/all] Error 2 /usr/bin/ld: CMakeFiles/test_component_interfaces.dir/test/test_component_interfaces.cpp.o: undefined reference to symbol '_ZN3fmt2v96detail11assert_failEPKciS3_' /usr/bin/ld: /lib/x86_64-linux-gnu/libfmt.so.9: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status gmake[2]: *** [CMakeFiles/test_component_interfaces.dir/build.make:271: test_component_interfaces] Error 1 gmake[1]: *** [CMakeFiles/Makefile2:361: CMakeFiles/test_component_interfaces.dir/all] Error 2 gmake: *** [Makefile:146: all] Error 2

I believe this happened when they are trying to link to joint_limit.

It seems that for INTERFACE libraries, I would need to link it like

target_link_libraries(joint_limits INTERFACE fmt::fmt)

otherwise I would need to link fmt::fmt in each test, am I correct?

the joint_limit itself is not using any fmt right?

codecov · 2025-05-02T21:10:59Z

Codecov Report

Attention: Patch coverage is 71.50000% with 57 lines in your changes missing coverage. Please review.

Project coverage is 88.94%. Comparing base (b2d9bf7) to head (2f7cb00).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
controller_manager/src/controller_manager.cpp	64.55%	28 Missing ⚠️
...r_interface/src/chainable_controller_interface.cpp	28.57%	10 Missing ⚠️
hardware_interface/src/component_parser.cpp	88.57%	4 Missing ⚠️
hardware_interface/src/resource_manager.cpp	60.00%	4 Missing ⚠️
.../include/hardware_interface/actuator_interface.hpp	81.81%	2 Missing ⚠️
...ce/include/hardware_interface/system_interface.hpp	81.81%	2 Missing ⚠️
joint_limits/src/joint_limits_helpers.cpp	0.00%	2 Missing ⚠️
...re_interface/include/hardware_interface/handle.hpp	85.71%	1 Missing ⚠️
...ce/include/hardware_interface/sensor_interface.hpp	80.00%	1 Missing ⚠️
...its/include/joint_limits/joint_limits_rosparam.hpp	75.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2205      +/-   ##
==========================================
- Coverage   89.14%   88.94%   -0.20%     
==========================================
  Files         141      142       +1     
  Lines       16168    16483     +315     
  Branches     1386     1427      +41     
==========================================
+ Hits        14413    14661     +248     
- Misses       1227     1269      +42     
- Partials      528      553      +25

Flag	Coverage Δ
unittests	`88.94% <71.50%> (-0.20%)`	⬇️

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

Files with missing lines	Coverage Δ
.../include/controller_manager/controller_manager.hpp	`96.87% <ø> (ø)`
...nt_limits/include/joint_limits/data_structures.hpp	`75.86% <100.00%> (ø)`
...re_interface/include/hardware_interface/handle.hpp	`88.69% <85.71%> (-0.60%)`	⬇️
...ce/include/hardware_interface/sensor_interface.hpp	`85.00% <80.00%> (-4.59%)`	⬇️
...its/include/joint_limits/joint_limits_rosparam.hpp	`59.42% <75.00%> (ø)`
...ansmission_interface/differential_transmission.hpp	`80.73% <90.00%> (+0.92%)`	⬆️
...ission_interface/four_bar_linkage_transmission.hpp	`79.00% <90.00%> (+1.10%)`	⬆️
.../include/hardware_interface/actuator_interface.hpp	`90.96% <81.81%> (-3.16%)`	⬇️
...ce/include/hardware_interface/system_interface.hpp	`84.26% <81.81%> (-2.58%)`	⬇️
joint_limits/src/joint_limits_helpers.cpp	`88.23% <0.00%> (+2.02%)`	⬆️
... and 4 more

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

saikishor

Just few minor things, rest looks good

saikishor · 2025-05-05T06:37:10Z

controller_interface/include/controller_interface/chainable_controller_interface.hpp

@@ -15,6 +15,8 @@
 #ifndef CONTROLLER_INTERFACE__CHAINABLE_CONTROLLER_INTERFACE_HPP_
 #define CONTROLLER_INTERFACE__CHAINABLE_CONTROLLER_INTERFACE_HPP_

+#include <fmt/compile.h>


@mini-1235 As we have included fmt as a build depend only, we will have to place this include in the .cpp, if not we will have to add the dependency to build_export_depend.

For this reason, if there is no changes in the .hpp, please avoid adding the header include there and move it to .cpp

saikishor · 2025-05-05T06:38:31Z

joint_limits/CMakeLists.txt

@@ -27,6 +28,7 @@ target_include_directories(joint_limits INTERFACE
  $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>
  $<INSTALL_INTERFACE:include/joint_limits>
 )
+target_link_libraries(joint_limits INTERFACE fmt::fmt)


I just rechecked it locally. you are right. I think your initial approach makes sense here. Just add it to the rosparam test directly :). Sorry for the confusion

target_link_libraries(joint_limits_rosparam_test joint_limits fmt::fmt)

saikishor · 2025-05-05T17:38:21Z

hardware_interface/include/hardware_interface/handle.hpp

      throw std::runtime_error(
-        "Invalid data type : '" + interface_description.interface_info.data_type +
-        "' for interface : " + interface_description.get_name());
+        fmt::format(
+          FMT_COMPILE("Invalid data type : '{}' for interface : {}"),
+          interface_description.interface_info.data_type, interface_description.get_name()));
    }


@bmagyar I would like to know your opinion here. fmt is cool when we use it for the string creation etc. I'm just not sure for the exceptions. Do you have a strong opinion here?

bmagyar · 2025-05-07T17:32:41Z

@mini-1235 not for this PR but I was wondering if FMT_COMPILE supported specifying the length of the placeholders in the string? it'd gain us fixed length strings

github-actions bot requested review from arne48, christophfroehlich, destogl, fmauch, harderthan, LukasMacha97 and peterdavidfagan April 26, 2025 15:46

saikishor requested changes Apr 26, 2025

View reviewed changes

mini-1235 force-pushed the patch-static-compile branch from e051b0b to 8332c26 Compare April 26, 2025 19:15

mini-1235 requested a review from saikishor April 26, 2025 19:19

christophfroehlich requested changes Apr 27, 2025

View reviewed changes

saikishor requested changes Apr 28, 2025

View reviewed changes

mini-1235 added 5 commits May 2, 2025 17:37

Statically allocate string concatenations using FMT formatting

f98a3ae

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

Add other strings to be also statically allocated

87268bd

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

Fix

ee329d6

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

Fix linking issues

a6ba0c0

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

Fix as requested

2f7cb00

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

mini-1235 force-pushed the patch-static-compile branch from b131097 to 2f7cb00 Compare May 2, 2025 17:39

saikishor reviewed May 2, 2025

View reviewed changes

mini-1235 and others added 3 commits May 3, 2025 22:05

Update controller_manager/package.xml

b79b11f

Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>

Add fmt headers

057d367

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

Fix precommit

6471ecf

Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>

saikishor reviewed May 5, 2025

View reviewed changes

saikishor added 2 commits May 5, 2025 08:38

Merge branch 'master' into patch-static-compile

3687a76

fix the fmt dependency

9cc3083

saikishor reviewed May 5, 2025

View reviewed changes

		target_link_libraries(test_joint_handle hardware_interface fmt::fmt)
		ament_target_dependencies(test_joint_handle rcpputils fmt)

	target_link_libraries(joint_limits_rosparam_test joint_limits fmt::fmt)
	target_link_libraries(joint_limits_rosparam_test joint_limits)

	target_link_libraries(joint_limits_urdf_test joint_limits fmt::fmt)
	target_link_libraries(joint_limits_urdf_test joint_limits)

Statically allocate string concatenations using FMT formatting #2205

Are you sure you want to change the base?

Statically allocate string concatenations using FMT formatting #2205

Conversation

mini-1235 commented Apr 26, 2025

saikishor left a comment

Choose a reason for hiding this comment

mini-1235 commented Apr 26, 2025 • edited Loading

saikishor commented Apr 26, 2025

mergify bot commented Apr 26, 2025

mini-1235 commented Apr 26, 2025

bmagyar commented Apr 26, 2025

saikishor commented Apr 26, 2025

christophfroehlich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saikishor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mini-1235 commented May 1, 2025

saikishor commented May 2, 2025

mergify bot commented May 2, 2025

mini-1235 commented May 2, 2025

saikishor commented May 2, 2025

saikishor left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 2, 2025

Codecov Report

saikishor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmagyar commented May 7, 2025

mini-1235 commented Apr 26, 2025 •

edited

Loading

saikishor left a comment •

edited

Loading