Conversation
Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @ahcorde, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini or gemini-code-assist here, providing a summary of this pull request. This PR aims to update the CMake build configuration to use modern CMake targets, specifically addressing the deprecation and removal of the ament_target_dependencies macro in newer ROS distributions like kilted and rolling. The changes involve modifying the main CMakeLists.txt to export the library as a modern target and updating include paths in source and header files to be package-relative, which is the standard practice when consuming modern CMake targets. Additionally, an example CMakeLists.txt is updated to demonstrate linking against the new modern target.
Highlights
- Modern CMake Target Export: The
dynamixel_sdklibrary is now exported as a modern CMake target (dynamixel_sdk::dynamixel_sdk) usinginstall(TARGETS ... EXPORT ...)andament_export_targets. This allows downstream packages to link against it usingtarget_link_libraries. - Include Path Updates: Internal include directives within the
dynamixel_sdksource and header files have been updated from relative paths (e.g.,"port_handler.h") to package-relative paths (e.g.,"dynamixel_sdk/port_handler.h"). This is a necessary change when consuming libraries via modern CMake targets. - Deprecated Macro Replacement: The usage of the deprecated
ament_target_dependenciesmacro in the exampleCMakeLists.txt(dynamixel_sdk_examples) has been replaced with the standard modern CMake commandtarget_link_libraries.
Changelog
Click here to see the changelog
- dynamixel_sdk/CMakeLists.txt
- Removed old
include_directoriescall. - Added
target_include_directorieswith generator expressions for build and install interfaces. - Modified
install(TARGETS ...)to includeEXPORT ${PROJECT_NAME}. - Added
ament_export_targets(${PROJECT_NAME})to export the modern CMake target. - Added comments clarifying old-style vs. modern CMake exports.
- Removed old
- dynamixel_sdk/include/dynamixel_sdk/dynamixel_sdk.h
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/group_bulk_read.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/include/dynamixel_sdk/group_bulk_read.h
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/port_handler.h"). - Minor whitespace fix in a comment.
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/include/dynamixel_sdk/group_bulk_write.h
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/port_handler.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/include/dynamixel_sdk/group_sync_read.h
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/port_handler.h"). - Minor whitespace fix in a comment.
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/include/dynamixel_sdk/group_sync_write.h
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/port_handler.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/include/dynamixel_sdk/packet_handler.h
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/port_handler.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/include/dynamixel_sdk/port_handler_arduino.h
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/port_handler.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/include/dynamixel_sdk/port_handler_linux.h
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/port_handler.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/include/dynamixel_sdk/port_handler_mac.h
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/port_handler.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/include/dynamixel_sdk/port_handler_windows.h
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/port_handler.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/include/dynamixel_sdk/protocol1_packet_handler.h
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/packet_handler.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/include/dynamixel_sdk/protocol2_packet_handler.h
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/packet_handler.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/src/dynamixel_sdk/group_bulk_read.cpp
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/group_bulk_read.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/src/dynamixel_sdk/group_bulk_write.cpp
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/group_bulk_write.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/src/dynamixel_sdk/group_sync_read.cpp
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/group_sync_read.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/src/dynamixel_sdk/group_sync_write.cpp
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/group_sync_write.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/src/dynamixel_sdk/packet_handler.cpp
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/packet_handler.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/src/dynamixel_sdk/port_handler.cpp
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/port_handler.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/src/dynamixel_sdk/port_handler_linux.cpp
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/port_handler_linux.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/src/dynamixel_sdk/port_handler_mac.cpp
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/port_handler_mac.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/src/dynamixel_sdk/port_handler_windows.cpp
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/port_handler_windows.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/src/dynamixel_sdk/protocol1_packet_handler.cpp
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/packet_handler.h").
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk/src/dynamixel_sdk/protocol2_packet_handler.cpp
- Updated include paths to be package-relative (e.g.,
"dynamixel_sdk/packet_handler.h"). - Minor whitespace fixes in comments and code.
- Updated include paths to be package-relative (e.g.,
- dynamixel_sdk_examples/CMakeLists.txt
- Replaced
ament_target_dependencieswithtarget_link_libraries. - Updated dependency names to use modern target syntax (
dynamixel_sdk::dynamixel_sdk) and exported variables (${dynamixel_sdk_custom_interfaces_TARGETS}).
- Replaced
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
CMake builds take flight,
Dependencies linked just right,
Modern targets gleam.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Relevant for rt-net/rt_manipulators_cpp#36 |
There was a problem hiding this comment.
Code Review
This pull request successfully updates the dynamixel_sdk package and its examples to use modern CMake targets, addressing the deprecation of ament_target_dependencies. The changes are well-implemented and necessary for compatibility with newer ROS 2 distributions. The update to namespaced include paths is also a good practice for library development.
Summary of Findings
- Modern CMake Target Export: The core change correctly implements modern CMake target export using
target_include_directories,install(TARGETS ... EXPORT ...), andament_export_targets. This is a high-priority change for compatibility with newer ROS 2 distributions. - Namespaced Include Paths: Include paths in header and source files have been updated to use the namespaced format (
#include "dynamixel_sdk/filename.h"). This improves code clarity and robustness when the library is installed and used by other packages (Medium severity). - Example Package Linking: The example package's CMakeLists.txt has been correctly updated to use
target_link_librariesand the modern target aliasdynamixel_sdk::dynamixel_sdk(High severity). - Trailing Whitespace: Trailing whitespace was removed in
protocol2_packet_handler.cpp. This is a minor stylistic cleanup (Low severity, no comment made per settings). - Docstring Formatting: Minor formatting fixes (trailing space removal) were made in some docstrings (Low severity, no comment made per settings).
Merge Readiness
The changes in this pull request are important for adopting modern CMake practices and ensuring compatibility with future ROS 2 releases. The implementation appears correct and addresses the stated objective. The identified issues are primarily related to adopting the new include path style, which are classified as medium severity. I recommend addressing these include path consistency issues before merging. I am unable to approve the pull request; please have other reviewers review and approve this code before merging.
| target_include_directories(${PROJECT_NAME} PUBLIC | ||
| "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>" | ||
| "$<INSTALL_INTERFACE:include>" | ||
| ) |
There was a problem hiding this comment.
Using target_include_directories with generator expressions ($<BUILD_INTERFACE:...> and $<INSTALL_INTERFACE:...>) is the correct modern CMake approach to specify include paths for a target. This replaces the older include_directories command and ensures that consumers of the installed library can find the headers correctly.
|
|
||
| install( | ||
| TARGETS ${PROJECT_NAME} | ||
| TARGETS ${PROJECT_NAME} EXPORT ${PROJECT_NAME} |
There was a problem hiding this comment.
Adding EXPORT ${PROJECT_NAME} to the install(TARGETS ...) command is essential for generating the necessary files (like ${PROJECT_NAME}Config.cmake) that allow other CMake projects to find and link against this library using find_package and target_link_libraries.
| ament_export_libraries(${PROJECT_NAME}) | ||
|
|
||
| # Export modern CMake targets | ||
| ament_export_targets(${PROJECT_NAME}) |
There was a problem hiding this comment.
Calling ament_export_targets(${PROJECT_NAME}) exports the modern CMake target created by add_library and installed with install(TARGETS ... EXPORT ...). This is the correct ament macro to use in conjunction with modern CMake targets for ROS 2 packages.
| target_link_libraries(read_write_node | ||
| PUBLIC | ||
| ${dynamixel_sdk_custom_interfaces_TARGETS} | ||
| dynamixel_sdk::dynamixel_sdk | ||
| rclcpp::rclcpp | ||
| ) |
There was a problem hiding this comment.
Replacing ament_target_dependencies with target_link_libraries and using the modern CMake target alias dynamixel_sdk::dynamixel_sdk is the correct way to link against the exported library. This aligns the example package with modern CMake practices and the changes made in the dynamixel_sdk package.
| #include <Windows.h> | ||
|
|
||
| #include "port_handler.h" | ||
| #include "dynamixel_sdk/port_handler.h" |
There was a problem hiding this comment.
Consistent with the change in dynamixel_sdk.h, updating the include paths here improves clarity and robustness when the library is installed and used by other packages.
| #endif | ||
|
|
||
| #include "port_handler_mac.h" | ||
| #include "dynamixel_sdk/port_handler_mac.h" |
There was a problem hiding this comment.
Updating include paths in source files to use the namespaced format is necessary to match the changes in the header files and ensure the code compiles correctly when the library is consumed by other packages.
| #define WINDLLEXPORT | ||
|
|
||
| #include "port_handler_windows.h" | ||
| #include "dynamixel_sdk/port_handler_windows.h" |
There was a problem hiding this comment.
Updating include paths in source files to use the namespaced format is necessary to match the changes in the header files and ensure the code compiles correctly when the library is consumed by other packages.
| #include "dynamixel_sdk/protocol1_packet_handler.h" | ||
| #elif defined(__APPLE__) | ||
| #include "protocol1_packet_handler.h" | ||
| #include "dynamixel_sdk/protocol1_packet_handler.h" | ||
| #elif defined(_WIN32) || defined(_WIN64) | ||
| #define WINDLLEXPORT | ||
| #include "protocol1_packet_handler.h" | ||
| #include "dynamixel_sdk/protocol1_packet_handler.h" |
There was a problem hiding this comment.
Updating include paths in source files to use the namespaced format is necessary to match the changes in the header files and ensure the code compiles correctly when the library is consumed by other packages.
| #include "dynamixel_sdk/protocol2_packet_handler.h" | ||
| #elif defined(__APPLE__) | ||
| #include <unistd.h> | ||
| #include "protocol2_packet_handler.h" | ||
| #include "dynamixel_sdk/protocol2_packet_handler.h" | ||
| #elif defined(_WIN32) || defined(_WIN64) | ||
| #define WINDLLEXPORT | ||
| #include <Windows.h> | ||
| #include "protocol2_packet_handler.h" | ||
| #include "dynamixel_sdk/protocol2_packet_handler.h" |
There was a problem hiding this comment.
Updating include paths in source files to use the namespaced format is necessary to match the changes in the header files and ensure the code compiles correctly when the library is consumed by other packages.
| #include "dynamixel_sdk/group_bulk_read.h" | ||
| #include "dynamixel_sdk/group_bulk_write.h" | ||
| #include "dynamixel_sdk/group_sync_read.h" | ||
| #include "dynamixel_sdk/group_sync_write.h" | ||
| #include "dynamixel_sdk/packet_handler.h" | ||
| #include "dynamixel_sdk/port_handler.h" |
There was a problem hiding this comment.
Changing includes from relative paths (e.g., "group_bulk_read.h") to namespaced paths (e.g., "dynamixel_sdk/group_bulk_read.h") is a good practice when exporting a library. It makes it clear which package the header belongs to and helps prevent naming conflicts when the library is installed and used by other packages.
|
Thank you for the pull request. It's a great reference, but the target branch is different, and certain parts of the code are different as well. We're going to close this PR and move forward with a new one. Thank you very much. We will open a new issue and manage it as follows: #654 |
2 participants
Export moderm CMake Target
Required because of the deprecation of
ament_target_dependenciesNOTE:
ament_target_dependenciesis deprecated onkiltedand removed inrolling, It will require a new release onrolling.