Remove build_dep generate_parameter_library from public dependencies of steering_controllers_library #1463

traversaro · 2025-01-02T09:20:18Z

steering_controllers_library already depended on generate_parameter_library in the CMake, but this dependency was not reflected in the package.xml metadata.

To send us a pull request, please:

Fork the repository.
Modify the source; please focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
Ensure local tests pass. (colcon test and pre-commit run (requires you to install pre-commit by pip3 install pre-commit)
Commit to your fork using clear commit messages.
Send a pull request, answering any default questions in the pull request interface.
Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

…lers_library

christophfroehlich

It is there as <build_depend>, do we need this as <depend>? Isn't it just generating headerfiles during the build stage?

- The generic dependency on 'generate_parameter_library' is redundant with: build_depend

Maybe we need it to export the dependencies of GPL (rsl, fmt)?

traversaro · 2025-01-02T09:43:08Z

It is there as <build_depend>, do we need this as <depend>? Isn't it just generating headerfiles during the build stage?

- The generic dependency on 'generate_parameter_library' is redundant with: build_depend

Maybe we need it to export the dependencies of GPL (rsl, fmt)?

That is interesting. I got confused as some other packages instead list it as a regular dependencies:

ros2_controllers/imu_sensor_broadcaster/package.xml

Line 25 in 214e333

<depend>generate_parameter_library</depend>

.

Just to provide you the full context, the problem that I was facing that just installing steering-controllers-library and checking if find_package(steering_controllers_library) works was failing with as find_package(generate_parameter_library) was failing. Indeed, generate_parameter_library is a public cmake dependency of steering_controllers_library via

ros2_controllers/steering_controllers_library/CMakeLists.txt

Line 18 in 214e333

generate_parameter_library

and

ros2_controllers/steering_controllers_library/CMakeLists.txt

Line 57 in 214e333

    
           ament_target_dependencies(steering_controllers_library PUBLIC ${THIS_PACKAGE_INCLUDE_DEPENDS})

, so if it can't be listed as both a build_depend and depend, it should be just listed as a normal dependency I guess?

traversaro · 2025-01-02T09:44:59Z

so if it can't be listed as both a build_depend and depend, it should be just listed as a normal dependency I guess?

An alternative that perhaps is more correct is just to avoid to pass generate_parameter_library to ament_target_dependencies(steering_controllers_library PUBLIC ...) so that indeed it is a build only dependency, and it is not a build dependency for each package that depends on steering_controllers_library ?

…of steering_controllers_library

traversaro · 2025-01-02T09:47:53Z

so if it can't be listed as both a build_depend and depend, it should be just listed as a normal dependency I guess?

An alternative that perhaps is more correct is just to avoid to pass generate_parameter_library to ament_target_dependencies(steering_controllers_library PUBLIC ...) so that indeed it is a build only dependency, and it is not a build dependency for each package that depends on steering_controllers_library ?

I tried to do that to check if at least the CI is happy.

traversaro · 2025-01-02T10:00:11Z

The rolling ci is failing with:

 $ ( source /home/runner/work/ros2_controllers/ros2_controllers/.work/upstream_ws/install/setup.bash && cd /home/runner/work/ros2_controllers/ros2_controllers/.work/target_ws && colcon build --event-handlers desktop_notification- status- terminal_title- --cmake-args -DCMAKE_CXX_FLAGS=-isystem /opt/ros/rolling/include; )
  Starting >>> forward_command_controller
  Starting >>> steering_controllers_library
  Starting >>> joint_trajectory_controller
  Starting >>> diff_drive_controller
  --- stderr: forward_command_controller
  In file included from /home/runner/work/ros2_controllers/ros2_controllers/.work/target_ws/src/ros2_controllers/forward_command_controller/src/forward_controllers_base.cpp:15:
  /home/runner/work/ros2_controllers/ros2_controllers/.work/target_ws/src/ros2_controllers/forward_command_controller/include/forward_command_controller/forward_controllers_base.hpp:25:10: fatal error: realtime_tools/realtime_buffer.hpp: No such file or directory
     25 | #include "realtime_tools/realtime_buffer.hpp"
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  compilation terminated.
  gmake[2]: *** [CMakeFiles/forward_command_controller.dir/build.make:76: CMakeFiles/forward_command_controller.dir/src/forward_controllers_base.cpp.o] Error 1
  gmake[2]: *** Waiting for unfinished jobs....
  In file included from /home/runner/work/ros2_controllers/ros2_controllers/.work/target_ws/src/ros2_controllers/forward_command_controller/include/forward_command_controller/forward_command_controller.hpp:20,
                   from /home/runner/work/ros2_controllers/ros2_controllers/.work/target_ws/src/ros2_controllers/forward_command_controller/src/forward_command_controller.cpp:15:
  /home/runner/work/ros2_controllers/ros2_controllers/.work/target_ws/src/ros2_controllers/forward_command_controller/include/forward_command_controller/forward_controllers_base.hpp:25:10: fatal error: realtime_tools/realtime_buffer.hpp: No such file or directory
     25 | #include "realtime_tools/realtime_buffer.hpp"
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  compilation terminated.
  gmake[2]: *** [CMakeFiles/forward_command_controller.dir/build.make:90: CMakeFiles/forward_command_controller.dir/src/forward_command_controller.cpp.o] Error 1
  In file included from /home/runner/work/ros2_controllers/ros2_controllers/.work/target_ws/src/ros2_controllers/forward_command_controller/include/forward_command_controller/multi_interface_forward_command_controller.hpp:20,
                   from /home/runner/work/ros2_controllers/ros2_controllers/.work/target_ws/src/ros2_controllers/forward_command_controller/src/multi_interface_forward_command_controller.cpp:15:
  /home/runner/work/ros2_controllers/ros2_controllers/.work/target_ws/src/ros2_controllers/forward_command_controller/include/forward_command_controller/forward_controllers_base.hpp:25:10: fatal error: realtime_tools/realtime_buffer.hpp: No such file or directory
     25 | #include "realtime_tools/realtime_buffer.hpp"
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  compilation terminated.
  gmake[2]: *** [CMakeFiles/forward_command_controller.dir/build.make:104: CMakeFiles/forward_command_controller.dir/src/multi_interface_forward_command_controller.cpp.o] Error 1
  gmake[1]: *** [CMakeFiles/Makefile2:239: CMakeFiles/forward_command_controller.dir/all] Error 2
  gmake: *** [Makefile:146: all] Error 2
  ---
  Failed   <<< forward_command_controller [8.87s, exited with code 2]
  Aborted  <<< steering_controllers_library [10.4s]
  Aborted  <<< diff_drive_controller [11.0s]
  Aborted  <<< joint_trajectory_controller [11.1s]
  
  Summary: 0 packages finished [11.3s]
    1 package failed: forward_command_controller
    3 packages aborted: diff_drive_controller joint_trajectory_controller steering_controllers_library
    4 packages had stderr output: diff_drive_controller forward_command_controller joint_trajectory_controller steering_controllers_library
    21 packages not processed
'build_target_workspace' returned with code '2' after 0 min 11 sec
0s

I am not sure if this is related to the PR.

christophfroehlich · 2025-01-02T10:00:29Z

Just to provide you the full context, the problem that I was facing that just installing steering-controllers-library and checking if find_package(steering_controllers_library) works was failing with as find_package(generate_parameter_library) was failing. Indeed, generate_parameter_library is a public cmake dependency of steering_controllers_library via

Maybe we should just stick to
https://github.com/PickNikRobotics/generate_parameter_library?tab=readme-ov-file#add-parameter-library-generation-to-project

Otherwise RSL and other packages might not be installed, but they are included in the steering_controllers_library.hpp?

Package: ros-rolling-steering-controllers-library
Version: 4.16.0-1noble.20241125.183234
Architecture: amd64
Maintainer: Bence Magyar <[email protected]>
Installed-Size: 821
Depends: libc6 (>= 2.34), libfmt9 (>= 9.1.0+ds1), libgcc-s1 (>= 3.3.1), libstdc++6 (>= 13.1), ros-rolling-ackermann-msgs, ros-rolling-backward-ros, ros-rolling-control-msgs, ros-rolling-controller-interface, ros-rolling-geometry-msgs, ros-rolling-hardware-interface, ros-rolling-nav-msgs, ros-rolling-pluginlib, ros-rolling-rclcpp, ros-rolling-rclcpp-lifecycle, ros-rolling-rcpputils, ros-rolling-realtime-tools, ros-rolling-std-srvs, ros-rolling-tf2, ros-rolling-tf2-geometry-msgs, ros-rolling-tf2-msgs, ros-rolling-ros-workspace
Priority: optional
Section: misc
Filename: pool/main/r/ros-rolling-steering-controllers-library/ros-rolling-steering-controllers-library_4.16.0-1noble.20241125.183234_amd64.deb
Size: 180492
SHA256: 3f1bc08f87f4e545399da547b3d97fbab05c279361c1ba367a414008cdeb0f9e
SHA1: 7cff2d2780e810fec2504ec28cef588593479d00
MD5sum: 89d99c7db2928cf2bba0151c3080904f
Description: Package for steering robot configurations including odometry and interfaces.

christophfroehlich · 2025-01-02T10:01:08Z

I am not sure if this is related to the PR.

No it's not, we are just waiting for a rolling sync of realtime_tools.

traversaro · 2025-01-02T10:08:36Z

Otherwise RSL and other packages might not be installed, but they are included in the steering_controllers_library.hpp?

Ahh, I see, so the code generated by generate_parameter_library should induce a dependency on rsl and fmt, but the tooling do not permit to specify this? In that case it seems that indeed just depending on generate_parameter_library following the generate_parameter_library docs may be the easiest solution.

codecov · 2025-01-02T10:10:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.83%. Comparing base (2375aca) to head (b1ee535).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1463   +/-   ##
=======================================
  Coverage   83.83%   83.83%           
=======================================
  Files         122      122           
  Lines       11120    11119    -1     
  Branches      944      943    -1     
=======================================
  Hits         9322     9322           
  Misses       1489     1489           
+ Partials      309      308    -1

Flag	Coverage Δ
unittests	`83.83% <ø> (+<0.01%)`	⬆️

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

see 1 file with indirect coverage changes

christophfroehlich · 2025-01-02T10:14:13Z

Otherwise RSL and other packages might not be installed, but they are included in the steering_controllers_library.hpp?

Ahh, I see, so the code generated by generate_parameter_library should induce a dependency on rsl and fmt, but the tooling do not permit to specify this? In that case it seems that indeed just depending on generate_parameter_library following the generate_parameter_library docs may be the easiest solution.

Yes . But to be honest, I'm not sure what happens if you want to include the header files provided by this package. The generated header files from generate_parameter_library aren't exported by default, see PickNikRobotics/generate_parameter_library#213.

traversaro · 2025-01-02T11:17:52Z

I looked a bit more in generate_parameter_library (I was not familiar on it).

Given:

ros2_controllers/steering_controllers_library/CMakeLists.txt

Lines 55 to 56 in 214e333

target_link_libraries(steering_controllers_library PUBLIC

steering_controllers_library_parameters)
https://github.com/PickNikRobotics/generate_parameter_library/blob/0346fde52ba515593bd51b96bc520fa872af5b2a/generate_parameter_library/cmake/generate_parameter_library.cmake#L79-L93

I think everything should work fine. The generated header gets installed in include/steering_controllers_library_parameters/steering_controllers_library_parameters.hpp, and its include directory is correctly set as include directory of the steering_controllers_library_parameters target defined by the generate_parameter_library cmake function, that is then linked as public dependency to steering_controllers_library . So any downstream consume that links steering_controllers_library will also link transitively steering_controllers_library_parameters and the correct include directory.

The only "problem" is that all the targets linked in https://github.com/PickNikRobotics/generate_parameter_library/blob/0346fde52ba515593bd51b96bc520fa872af5b2a/generate_parameter_library/cmake/generate_parameter_library.cmake#L85-L91 are never accounted for by adding find_dependency in the generated cmake config file, and hence the easiest solution to ensure that this is done is just by adding a regular dependency on generate_parameter_library.

TL;DR: I think the correct solution is to move generate_parameter_library from a build_depend to depend, that indeed is what generate_parameter_library documentation suggests.

The generated header files from generate_parameter_library aren't exported by default, see PickNikRobotics/generate_parameter_library#213.

I am not sure what you mean by "exported by default". The headers are installed in https://github.com/PickNikRobotics/generate_parameter_library/blob/0346fde52ba515593bd51b96bc520fa872af5b2a/generate_parameter_library/cmake/generate_parameter_library.cmake#L93, while the target is installed in

ros2_controllers/steering_controllers_library/CMakeLists.txt

Line 85 in 214e333

TARGETS steering_controllers_library steering_controllers_library_parameters

.

traversaro · 2025-01-02T11:22:58Z

As this PR was a bit dirty, I opened a new one based on what we understood in this PR: #1465 .

christophfroehlich · 2025-01-02T11:45:55Z

I am not sure what you mean by "exported by default". The headers are installed in https://github.com/PickNikRobotics/generate_parameter_library/blob/0346fde52ba515593bd51b96bc520fa872af5b2a/generate_parameter_library/cmake/generate_parameter_library.cmake#L93, while the target is installed in

ros2_controllers/steering_controllers_library/CMakeLists.txt

Line 85 in 214e333

TARGETS steering_controllers_library steering_controllers_library_parameters

.

OK, maybe the header is installed to be found within the same package, but can't be included directly from another package?

christophfroehlich · 2025-01-02T11:48:15Z

And should we fix that for every controller in this repo? Or is it only necessary for this library, which is explicitly meant to be used by another package..

traversaro · 2025-01-02T14:55:16Z

OK, maybe the header is installed to be found within the same package, but can't be included directly from another package?

Why it can't be included? Downstream packages that include #include "steering_controllers_library/steering_controllers_library.hpp" already transitively include #include "steering_controllers_library/steering_controllers_library.hpp" already transitive include #include "steering_controllers_library_parameters.hpp", so they could also include #include "steering_controllers_library_parameters.hpp" directly, and this would work if they link the steering_controllers_library or steering_controllers_library_parameters target. For sure it is a bit awkward to include the header without any prefix differently from other headers installed by the library (and I guess PickNikRobotics/generate_parameter_library#213 is also about that), but I see no reason why the autogenerated parameter can't be used at the moment (even as if that was the case, it could not be included in a public header of the library).

christophfroehlich · 2025-01-02T18:15:36Z

I tested it and apparently, it works.

I thought that this folder structure made it not possible to access the includes from the parameter library in a different package

but your argument makes sense 🙈

This seems to do the job, a pattern that we use here but is not included in the current GPL readme

install(TARGETS minimal_node turtlesim_parameters
  EXPORT ${PROJECT_NAME}Targets)
ament_export_targets(${PROJECT_NAME}Targets HAS_LIBRARY_TARGET)

Add missing generate_parameter_library dependency in steering_control…

266c8de

…lers_library

github-actions bot requested review from bijoua29, christophfroehlich, destogl, fmauch, mcbed, rosterloh and VX792 January 2, 2025 09:20

christophfroehlich reviewed Jan 2, 2025

View reviewed changes

traversaro changed the title ~~Add missing generate_parameter_library dependency in steering_controllers_library~~ Remove build_dep generate_parameter_library from public dependencies of steering_controllers_library Jan 2, 2025

Remove build_dep generate_parameter_library from public dependencies …

b1ee535

…of steering_controllers_library

traversaro closed this Jan 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove build_dep generate_parameter_library from public dependencies of steering_controllers_library #1463

Remove build_dep generate_parameter_library from public dependencies of steering_controllers_library #1463

traversaro commented Jan 2, 2025 •

edited

Loading

christophfroehlich left a comment •

edited

Loading

traversaro commented Jan 2, 2025

traversaro commented Jan 2, 2025

traversaro commented Jan 2, 2025

traversaro commented Jan 2, 2025

christophfroehlich commented Jan 2, 2025

christophfroehlich commented Jan 2, 2025

traversaro commented Jan 2, 2025

codecov bot commented Jan 2, 2025 •

edited

Loading

christophfroehlich commented Jan 2, 2025

traversaro commented Jan 2, 2025

traversaro commented Jan 2, 2025

christophfroehlich commented Jan 2, 2025

christophfroehlich commented Jan 2, 2025

traversaro commented Jan 2, 2025

christophfroehlich commented Jan 2, 2025

Remove build_dep generate_parameter_library from public dependencies of steering_controllers_library #1463

Remove build_dep generate_parameter_library from public dependencies of steering_controllers_library #1463

Conversation

traversaro commented Jan 2, 2025 • edited Loading

christophfroehlich left a comment • edited Loading

Choose a reason for hiding this comment

traversaro commented Jan 2, 2025

traversaro commented Jan 2, 2025

traversaro commented Jan 2, 2025

traversaro commented Jan 2, 2025

christophfroehlich commented Jan 2, 2025

christophfroehlich commented Jan 2, 2025

traversaro commented Jan 2, 2025

codecov bot commented Jan 2, 2025 • edited Loading

Codecov Report

christophfroehlich commented Jan 2, 2025

traversaro commented Jan 2, 2025

traversaro commented Jan 2, 2025

christophfroehlich commented Jan 2, 2025

christophfroehlich commented Jan 2, 2025

traversaro commented Jan 2, 2025

christophfroehlich commented Jan 2, 2025

traversaro commented Jan 2, 2025 •

edited

Loading

christophfroehlich left a comment •

edited

Loading

codecov bot commented Jan 2, 2025 •

edited

Loading