Conversation
Cmake fix for buildfarm
Signed-off-by: Hyungyu Kim <kimhg@robotis.com>
change name of variable
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @robotpilot, 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!
This pull request primarily focuses on a version bump for several turtlebot3 application packages to 1.3.1. A significant part of this update includes a fix for a CMake issue within the turtlebot3_panorama package, improving its compatibility and build process across different ROS 2 distributions.
Highlights
- Version Bump: The version of multiple
turtlebot3application packages (includingturtlebot3_applications,turtlebot3_aruco_tracker,turtlebot3_automatic_parking,turtlebot3_automatic_parking_vision,turtlebot3_follower,turtlebot3_panorama, andturtlebot3_yolo_object_detection) has been updated from1.3.0to1.3.1in their respectivepackage.xmlandsetup.pyfiles. - CMake Error Fix in
turtlebot3_panorama: A CMake error in theturtlebot3_panoramapackage has been addressed. TheCMakeLists.txtnow detects the ROS distribution by checking for the existence of/opt/ros/humbleinstead of relying on theROS_DISTROenvironment variable. This change definesROS2_HUMBLEorROS2_LATESTfor conditional compilation. - Conditional Compilation Update: The
panorama.hppfile inturtlebot3_panoramahas been updated to align itscv_bridgeinclude logic with the new ROS distribution detection, changing the conditional compilation fromROS2_JAZZY_OR_ROLLINGtoROS2_LATEST.
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.
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. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR bumps the version of multiple TurtleBot3 packages from 1.3.0 to 1.3.1, adds corresponding changelog entries, and refactors the ROS2 distro detection logic and header inclusion in the panorama package.
- Version updates in setup.py and package.xml for all affected packages
- Added 1.3.1 entries to each package’s CHANGELOG.rst
- Simplified CMakeLists ROS2 distro detection and unified panorama.hpp macro to
ROS2_LATEST
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| turtlebot3_yolo_object_detection/setup.py | version -> 1.3.1 |
| turtlebot3_yolo_object_detection/package.xml | version -> 1.3.1 |
| turtlebot3_yolo_object_detection/CHANGELOG.rst | added 1.3.1 entry |
| turtlebot3_panorama/package.xml | version -> 1.3.1 |
| turtlebot3_panorama/include/turtlebot3_panorama/panorama.hpp | switched from ROS2_JAZZY_OR_ROLLING to ROS2_LATEST |
| turtlebot3_panorama/CMakeLists.txt | simplified distro check, defined ROS2_LATEST |
| turtlebot3_panorama/CHANGELOG.rst | added 1.3.1 entry with fix note |
| turtlebot3_follower/package.xml | version -> 1.3.1 |
| turtlebot3_follower/CHANGELOG.rst | added 1.3.1 entry |
| turtlebot3_automatic_parking_vision/setup.py | version -> 1.3.1 |
| turtlebot3_automatic_parking_vision/package.xml | version -> 1.3.1 |
| turtlebot3_automatic_parking_vision/CHANGELOG.rst | added 1.3.1 entry |
| turtlebot3_automatic_parking/setup.py | version -> 1.3.1 |
| turtlebot3_automatic_parking/package.xml | version -> 1.3.1 |
| turtlebot3_automatic_parking/CHANGELOG.rst | added 1.3.1 entry |
| turtlebot3_aruco_tracker/setup.py | version -> 1.3.1 |
| turtlebot3_aruco_tracker/package.xml | version -> 1.3.1 |
| turtlebot3_aruco_tracker/CHANGELOG.rst | added 1.3.1 entry |
| turtlebot3_applications/package.xml | version -> 1.3.1 |
| turtlebot3_applications/CHANGELOG.rst | added 1.3.1 entry with panorama fix reference |
Comments suppressed due to low confidence (1)
turtlebot3_yolo_object_detection/CHANGELOG.rst:7
- [nitpick] An entry labeled "* None" yields an empty release note. If there are truly no changes, consider omitting this section or updating it to a clearer message such as "No user-visible changes in this release."
* None
| message("Humble") | ||
| else() | ||
| add_definitions(-DROS2_LATEST) | ||
| message("Latest") |
There was a problem hiding this comment.
Use an explicit message(STATUS ...) call (e.g., message(STATUS "Detected ROS2 Humble; defining ROS2_HUMBLE")) to clarify the message type and avoid confusion with warnings or errors.
| message("Humble") | |
| else() | |
| add_definitions(-DROS2_LATEST) | |
| message("Latest") | |
| message(STATUS "Detected ROS2 Humble; defining ROS2_HUMBLE") | |
| else() | |
| add_definitions(-DROS2_LATEST) | |
| message(STATUS "Detected ROS2 Latest; defining ROS2_LATEST") |
| if(EXISTS "/opt/ros/humble") | ||
| add_definitions(-DROS2_HUMBLE) | ||
| elseif(ROS_DISTRO STREQUAL "jazzy" OR ROS_DISTRO STREQUAL "rolling") | ||
| add_definitions(-DROS2_JAZZY_OR_ROLLING) | ||
| message("Humble") | ||
| else() | ||
| add_definitions(-DROS2_LATEST) | ||
| message("Latest") |
There was a problem hiding this comment.
Hardcoding the ROS installation path may be brittle. Consider parameterizing the distro path or falling back to the ROS_DISTRO environment variable for greater flexibility across different install locations.
There was a problem hiding this comment.
Code Review
This pull request bumps the version of several packages to 1.3.1 and updates their changelogs accordingly. The main functional change is in turtlebot3_panorama/CMakeLists.txt, where the method for detecting the ROS distribution has been modified. While the new method ensures a preprocessor definition is always set, it relies on a hardcoded path check (/opt/ros/humble), which reduces portability compared to the previous environment variable-based approach. A suggestion is provided to improve the robustness of this detection logic.
| if(EXISTS "/opt/ros/humble") | ||
| add_definitions(-DROS2_HUMBLE) | ||
| elseif(ROS_DISTRO STREQUAL "jazzy" OR ROS_DISTRO STREQUAL "rolling") | ||
| add_definitions(-DROS2_JAZZY_OR_ROLLING) | ||
| message("Humble") | ||
| else() | ||
| add_definitions(-DROS2_LATEST) | ||
| message("Latest") | ||
| endif() |
There was a problem hiding this comment.
The updated logic for detecting ROS Humble by checking EXISTS "/opt/ros/humble" is less robust than the previous method that utilized the ROS_DISTRO environment variable.
Concerns:
- Portability: Hardcoding
/opt/ros/humbleassumes a standard installation path. This will fail if ROS Humble is installed in a custom location (e.g., built from source in a home directory, different path in a Docker container, or on systems where/optis not the standard). - Accuracy: If multiple ROS distributions are present, this check might not reflect the currently active ROS environment sourced by the user.
The previous method using ENV{ROS_DISTRO} (and printenv ROS_DISTRO as a fallback) is generally more reliable as it respects the environment set up by ROS sourcing scripts.
While this change might fix a specific "cmake error" (as mentioned in the changelog) related to ROS_DISTRO not being defined or execute_process failing in some edge cases, it introduces a broader portability issue. The new approach ensures that a preprocessor definition (-DROS2_HUMBLE or -DROS2_LATEST) is always added, which is good for preventing C++ compilation errors if no definition was previously set. However, the choice of definition might be incorrect due to the hardcoded path.
A more robust approach would be to primarily rely on ENV{ROS_DISTRO} and then provide a default if it's not set or recognized, ensuring a definition is always made without sacrificing portability.
set(_ENV_ROS_DISTRO "$ENV{ROS_DISTRO}")
if(_ENV_ROS_DISTRO STREQUAL "humble")
add_definitions(-DROS2_HUMBLE)
message(STATUS "Configuring for ROS Humble based on ENV{ROS_DISTRO}.")
else()
# Default to LATEST for other distros (e.g., jazzy, rolling) or if ENV{ROS_DISTRO} is not 'humble', empty, or not set.
# This ensures a definition is always added.
if(_ENV_ROS_DISTRO AND NOT _ENV_ROS_DISTRO STREQUAL "")
message(STATUS "ENV{ROS_DISTRO} is '${_ENV_ROS_DISTRO}'. Configuring as LATEST.")
else()
message(STATUS "ENV{ROS_DISTRO} not set or empty. Defaulting to LATEST configuration.")
endif()
add_definitions(-DROS2_LATEST)
endif()
No description provided.