Skip to content
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

Merged
merged 4 commits into from Feb 24, 2024
Merged

Fix rolling builds #289

merged 4 commits into from Feb 24, 2024

Conversation

achim-k
Copy link
Collaborator

@achim-k achim-k commented Feb 23, 2024

Public-Facing Changes

None

Description

Fixes ROS rolling compile errors, namely:

  • Replace std::bind wtih lambda to make it compatible with latest rolling changes
  • Fix deprecation warning (get_typesupport_handle is deprecated on rolling)

Fixes #284 (hopefully)
Resolves FG-6444

defunctzombie
defunctzombie previously approved these changes Feb 23, 2024
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)
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Collaborator Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😭

Copy link
Contributor

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})?

Copy link
Collaborator Author

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

@defunctzombie defunctzombie dismissed their stale review February 23, 2024 17:06

Same question as John about the string comparison

@achim-k achim-k merged commit 9d92068 into main Feb 24, 2024
10 checks passed
@achim-k achim-k deleted the achim/rolling-build-fixes branch February 24, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fix failing ROS build farm jobs
3 participants