-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix rolling builds #289
Fix rolling builds #289
Conversation
CMakeLists.txt
Outdated
@@ -154,6 +154,13 @@ elseif("$ENV{ROS_VERSION}" STREQUAL "2") | |||
ros2_foxglove_bridge/src/parameter_interface.cpp | |||
ros2_foxglove_bridge/src/generic_client.cpp | |||
) | |||
|
|||
# Check if the ROS_DISTRO is greater than iron and set a compile definition if that's the case. | |||
string(COMPARE GREATER $ENV{ROS_DISTRO} "iron" ROS_DISTRO_GT_IRON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this will do what you want? > on a string comparison?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think technically yes because ROS distro names go in alphabetical order for ROS2 (but letters are re-used from ROS1) but I also think we might want something more robust than this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not super happy with this approach but not sure what else to do. I could check if ROS_DISTRO == rolling but then we will have to change this everytime a new release is spun off. Any other ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use target_compile_definitions(... PUBLIC RCLCPP_VERSION_MAJOR=${rclcpp_VERSION_MAJOR})
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems to work, great suggestion
Same question as John about the string comparison
Public-Facing Changes
None
Description
Fixes ROS rolling compile errors, namely:
get_typesupport_handle
is deprecated on rolling)Fixes #284 (hopefully)
Resolves FG-6444