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

build: fix Ros2 humble build #36

Merged
merged 13 commits into from
Jun 12, 2024

Conversation

tfoldi
Copy link

@tfoldi tfoldi commented Nov 30, 2023

No description provided.

@twdragon
Copy link
Collaborator

@tfoldi could you please provide a description about the issue you resolved, and some proofs?

include_directories(include)
include_directories(/opt/ros/humble/include/tf2_geometry_msgs)
Copy link
Author

Choose a reason for hiding this comment

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

Removed any hardcoded path to ensure it will compile on all ROS2 deployments like robostack, source builds, etc.

- humble
- iron
- rolling
include:
Copy link
Author

Choose a reason for hiding this comment

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

Add CI/CD build test for humble, iron and rolling

@@ -54,27 +50,30 @@ add_library(witmotion_ros

message(WARNING "tf2_geometry_msgs include dir: ${tf2_geometry_msgs_INCLUDE_DIRS} ")

#find_package(Qt5 REQUIRED COMPONENTS Core SerialPort)
set(INCLUDE_DIRS ${ament_cmake_INCLUDE_DIRS} ${tf2_INCLUDE_DIRS}
Copy link
Author

Choose a reason for hiding this comment

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

change CMAKE style dependency management to more idiomatic ament_target_dependencies


<buildtool_depend>ament_cmake</buildtool_depend>
<buildtool_depend>ament_cmake_auto</buildtool_depend>

<depend>tf2_geometry_msgs</depend>
Copy link
Author

Choose a reason for hiding this comment

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

add depend to ensure rosdep can install all required dependencies

@tfoldi
Copy link
Author

tfoldi commented Jan 7, 2024

hey @twdragon, the following changes were applied:

  • remove hardcoded pathes from CMakeLists.txt
  • replaced vanilla CMake macros to amend macros like ament_target_dependencies. This is more standard/idiomatic in ROS2, plus it looks way cleaner than the original file
  • added dependencies to package.xml. Now rosdep can install the missing dependencies for the project.
  • Made a github action (seems you actually executed them) to test the build for all current ros2 dists + rolling. As you can see all of them all green, ensuring the project can be built on top of the official ROS2 images:
image

I made a few comments on the relevant code lines, hope that helps. Should you have any question please let me know

In general, other than the project definition and make files no

@fllay
Copy link
Contributor

fllay commented May 29, 2024

I build this branch successfully with ROS 2 Jazzy. Can this one merge to the main?

@twdragon twdragon merged commit b5caf71 into ElettraSciComp:ros2 Jun 12, 2024
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants