Modified the CMakelists for kilted

[yun-goon]

Description

Changed ament_include_dependency to target_link_libraries

[gemini-code-assist]

Hello @yun-goon, I'm Gemini Code Assist¹! 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 team, gemini-code-assist here to provide a summary of this pull request. This PR focuses on updating the CMakeLists.txt file for the kilted package. The primary goal, as indicated by the title and description, is to migrate the dependency handling from the ROS 2-specific ament_target_dependencies CMake function to the more standard CMake function target_link_libraries. This change aims to align the build configuration more closely with standard CMake practices while still integrating with the ament build system.

Highlights

• Dependency Management Update: The core change involves replacing the use of ament_target_dependencies with target_link_libraries for linking the project's executable/library (${PROJECT_NAME}) against its required dependencies. This is a significant shift in how dependencies are declared and linked within the CMake build process for this package.
• Include Directory Addition: The include directories for the dynamixel_sdk package are now explicitly added to the target's include paths using target_include_directories. Previously, this might have been implicitly handled by ament_target_dependencies, but the change to target_link_libraries necessitates this explicit addition.
• Export Dependency Removal: The aimbot_log_interfaces package has been removed from the list of dependencies exported by this package using ament_export_dependencies. This suggests that aimbot_log_interfaces is no longer a public dependency that downstream packages consuming kilted need to be aware of.

Changelog

• CMakeLists.txt
  • Replaced ament_target_dependencies with target_link_libraries for linking core dependencies (hardware_interface, pluginlib, rclcpp, dynamixel_sdk, std_srvs, dynamixel_interfaces, realtime_tools).
  • Updated the format of dependencies in target_link_libraries to use target-based linking (e.g., package::target) or variable-based linking (e.g., ${package_TARGETS}).
  • Added ${dynamixel_sdk_INCLUDE_DIRS} to the target_include_directories for the project.
  • Removed aimbot_log_interfaces from the list of exported dependencies in ament_export_dependencies.

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 lists the way,
Dependencies linked today,
Builds now take flight.

Footnotes

1. 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. ↩

[gemini-code-assist]

Code Review

This pull request updates the CMakeLists.txt file by transitioning from ament_target_dependencies to the standard CMake target_link_libraries command for managing dependencies. This is a positive change towards more explicit and modern CMake practices. The removal of aimbot_log_interfaces from ament_export_dependencies also appears to be a good cleanup, aligning with the package.xml.

However, there are a couple of points to address for clarity and correctness, particularly concerning the public linkage of std_srvs and realtime_tools and their absence from ament_export_dependencies, and a minor point about potential redundancy in including dynamixel_sdk headers.

Summary of Findings

• Dependency Export Mismatch: std_srvs and realtime_tools are linked PUBLIC but not included in ament_export_dependencies. This can cause issues for downstream packages. They should either be added to ament_export_dependencies or linked PRIVATE if they are not part of the public API. (Severity: high)
• Potential Redundancy in Include Directories: Explicitly adding ${dynamixel_sdk_INCLUDE_DIRS} via target_include_directories might be redundant if ${dynamixel_sdk_TARGETS} (linked PUBLIC) already propagates these includes. Clarification on its necessity is requested. (Severity: medium)
• Modernization of Dependency Linking: The change from ament_target_dependencies to target_link_libraries is a good step towards using standard CMake practices, offering more explicit control over linkage and include directory propagation.
• Cleanup of Exported Dependencies: The removal of aimbot_log_interfaces from ament_export_dependencies (formerly line 99 on the left side) is a good cleanup, aligning the CMake configuration with the package.xml. This was noted as a low severity improvement and not commented inline due to review settings.

Merge Readiness

The pull request makes good strides in modernizing the CMake configuration. However, there's a high severity issue concerning the mismatch between PUBLIC linkage of std_srvs and realtime_tools and their absence in ament_export_dependencies. There's also a medium severity point about potential redundancy for dynamixel_sdk includes. These should be addressed before merging to ensure correctness and clarity for downstream packages.

As an AI, I am not authorized to approve pull requests; please ensure these points are resolved and further review and approval from team members are obtained before merging.

[Woojin-Crive]

LGTM

[sunghowoo]

LGTM