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?

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

hardware_interface/package.xml


  <build_depend>rcutils</build_depend>
  <exec_depend>rcutils</exec_depend>

  <test_depend>ament_cmake_gmock</test_depend>
  <test_depend>ros2_control_test_assets</test_depend>
+  <test_depend>fmt</test_depend>


Suggested change

<test_depend>fmt</test_depend>

this is not necessary if there is already a depend for it

christophfroehlich · 2025-04-27T18:23:47Z

hardware_interface_testing/package.xml


  <test_depend>ament_cmake_gmock</test_depend>
+  <test_depend>fmt</test_depend>


Suggested change

<test_depend>fmt</test_depend>

this is not necessary if there is already a depend for it

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 ?

christophfroehlich · 2025-04-27T18:27:17Z

joint_limits/package.xml


  <test_depend>ament_cmake_gmock</test_depend>
  <test_depend>generate_parameter_library</test_depend>
  <test_depend>launch_ros</test_depend>
  <test_depend>launch_testing_ament_cmake</test_depend>
+  <test_depend>fmt</test_depend>


Suggested change

<test_depend>fmt</test_depend>

this is not necessary if there is already a depend for it

christophfroehlich · 2025-04-27T18:27:29Z

transmission_interface/package.xml


  <test_depend>ament_cmake_gmock</test_depend>
  <test_depend>ros2_control_test_assets</test_depend>
+  <test_depend>fmt</test_depend>


Suggested change

<test_depend>fmt</test_depend>

this is not necessary if there is already a depend for it

christophfroehlich · 2025-04-27T18:43:11Z

controller_manager/src/controller_manager.cpp

+            fmt::format(
+              FMT_COMPILE("Failed to set the initial state of the component : %s to %s"),


Is %s valid fmt syntax? Here they only use it with fmt::fprintf

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

saikishor · 2025-04-28T10:13:26Z

hardware_interface/CMakeLists.txt

@@ -40,6 +41,7 @@ target_include_directories(hardware_interface PUBLIC
  $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>
  $<INSTALL_INTERFACE:include/hardware_interface>
 )
+target_link_libraries(hardware_interface PRIVATE fmt::fmt)


Suggested change

target_link_libraries(hardware_interface PRIVATE fmt::fmt)

ament_target_dependencies should alone be enough

@mini-1235 next time please commit the changes directly from the suggestions above. you can do so by clicking commit button in the UI. If you go to files tab, you can groups changes together and commit it in a single commit

@mini-1235 next time please commit the changes directly from the suggestions above. you can do so by clicking commit button in the UI. If you go to files tab, you can groups changes together and commit it in a single commit

Ok!

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)

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

hardware_interface/CMakeLists.txt

+  target_link_libraries(test_component_interfaces hardware_interface fmt::fmt)
+  ament_target_dependencies(test_component_interfaces ros2_control_test_assets fmt)


Suggested change

target_link_libraries(test_component_interfaces hardware_interface fmt::fmt)

ament_target_dependencies(test_component_interfaces ros2_control_test_assets fmt)

target_link_libraries(test_component_interfaces hardware_interface)

ament_target_dependencies(test_component_interfaces ros2_control_test_assets)

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

hardware_interface/CMakeLists.txt

+  target_link_libraries(test_component_interfaces_custom_export hardware_interface fmt::fmt)
+  ament_target_dependencies(test_component_interfaces_custom_export ros2_control_test_assets fmt)


Suggested change

target_link_libraries(test_component_interfaces_custom_export hardware_interface fmt::fmt)

ament_target_dependencies(test_component_interfaces_custom_export ros2_control_test_assets fmt)

target_link_libraries(test_component_interfaces_custom_export hardware_interface)

ament_target_dependencies(test_component_interfaces_custom_export ros2_control_test_assets)

I don't think it is needed in these files

saikishor · 2025-04-28T10:16:37Z

hardware_interface_testing/CMakeLists.txt

 ament_target_dependencies(test_components hardware_interface pluginlib ros2_control_test_assets)
+target_link_libraries(test_components fmt::fmt)


Suggested change

ament_target_dependencies(test_components hardware_interface pluginlib ros2_control_test_assets)

target_link_libraries(test_components fmt::fmt)

ament_target_dependencies(test_components hardware_interface pluginlib ros2_control_test_assets fmt)

saikishor · 2025-04-28T10:16:55Z

hardware_interface_testing/CMakeLists.txt

+  target_link_libraries(test_resource_manager fmt::fmt)
+  ament_target_dependencies(test_resource_manager hardware_interface ros2_control_test_assets fmt)


Suggested change

target_link_libraries(test_resource_manager fmt::fmt)

ament_target_dependencies(test_resource_manager hardware_interface ros2_control_test_assets fmt)

ament_target_dependencies(test_resource_manager hardware_interface ros2_control_test_assets fmt)

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)

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

joint_limits/CMakeLists.txt

+    PRIVATE PARAMETERS_FILE_PATH="${CMAKE_CURRENT_LIST_DIR}/test/"
+  )


Suggested change

PRIVATE PARAMETERS_FILE_PATH="${CMAKE_CURRENT_LIST_DIR}/test/"

)

PRIVATE PARAMETERS_FILE_PATH="${CMAKE_CURRENT_LIST_DIR}/test/")

Any particular reason for this change?

Nope, I have reverted it

saikishor · 2025-04-28T10:18:35Z

transmission_interface/CMakeLists.txt

@@ -27,6 +28,7 @@ target_include_directories(transmission_interface PUBLIC
  $<INSTALL_INTERFACE:include/transmission_interface>
 )
 ament_target_dependencies(transmission_interface PUBLIC ${THIS_PACKAGE_INCLUDE_DEPENDS})
+target_link_libraries(transmission_interface PUBLIC fmt::fmt)


Suggested change

target_link_libraries(transmission_interface PUBLIC fmt::fmt)

As you have added it to the THIS_PACKAGE_INCLUDE_DEPENDS, it should be automatic

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 <[email protected]>

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

saikishor · 2025-05-02T20:56:33Z

controller_interface/src/chainable_controller_interface.cpp

@@ -62,12 +62,12 @@ ChainableControllerInterface::export_state_interfaces()
  {
    if (interface.get_prefix_name().find(get_node()->get_name()) != 0)


@mini-1235 the include of the fmt is missing in this file

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

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 <[email protected]>

Signed-off-by: mini-1235 <[email protected]>

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 <[email protected]>

Add other strings to be also statically allocated

87268bd

Signed-off-by: mini-1235 <[email protected]>

Fix

ee329d6

Signed-off-by: mini-1235 <[email protected]>

Fix linking issues

a6ba0c0

Signed-off-by: mini-1235 <[email protected]>

Fix as requested

2f7cb00

Signed-off-by: mini-1235 <[email protected]>

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 2 commits May 3, 2025 22:05

Update controller_manager/package.xml

b79b11f

Co-authored-by: Sai Kishor Kothakota <[email protected]>

Add fmt headers

057d367

Signed-off-by: mini-1235 <[email protected]>


		<test_depend>ament_cmake_gmock</test_depend>
		<test_depend>fmt</test_depend>

		fmt::format(
		FMT_COMPILE("Failed to set the initial state of the component : %s to %s"),

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

		target_link_libraries(test_component_interfaces hardware_interface fmt::fmt)
		ament_target_dependencies(test_component_interfaces ros2_control_test_assets fmt)

		ament_target_dependencies(test_components hardware_interface pluginlib ros2_control_test_assets)
		target_link_libraries(test_components fmt::fmt)

		target_link_libraries(test_resource_manager fmt::fmt)
		ament_target_dependencies(test_resource_manager hardware_interface ros2_control_test_assets 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)

		PRIVATE PARAMETERS_FILE_PATH="${CMAKE_CURRENT_LIST_DIR}/test/"
		)

		@@ -62,12 +62,12 @@ ChainableControllerInterface::export_state_interfaces()
		{
		if (interface.get_prefix_name().find(get_node()->get_name()) != 0)

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

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

Choose a reason for hiding this comment

christophfroehlich Apr 27, 2025 • edited Loading

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

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

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

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

codecov bot commented May 2, 2025

Codecov Report

mini-1235 commented Apr 26, 2025 •

edited

Loading

christophfroehlich Apr 27, 2025 •

edited

Loading

saikishor left a comment •

edited

Loading