-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add ROS2 build test #33
Conversation
03db472
to
3b29395
Compare
Fix format Remove wstool Run with bash
1c739b2
to
615d9e7
Compare
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.
Nice! Everything looks good. If you remove all the commented code, that would make it clearer to read. It could be pretty short.
Is there a reason you didn't call colcon test
after the build?
src/test_tif_loader.cpp
Outdated
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.
Did you mean to change the indent in this PR?
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.
Yes, that was part of a format fix. Are you using a different formatter than the one in this repo? (Tools)
@@ -14,8 +15,9 @@ jobs: | |||
fail-fast: false | |||
matrix: | |||
config: | |||
- {rosdistro: 'melodic', container: 'px4io/px4-dev-ros-melodic:2021-12-11'} | |||
- {rosdistro: 'noetic', container: 'px4io/px4-dev-ros-noetic:2021-12-11'} | |||
- {rosdistro: 'humble', container: 'osrf/ros:humble-desktop'} |
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.
If you only have one set in the matrix, I think just hard code these. Matrix like this will only work if humble, iron, and rolling have no difference. The more common pattern is to create a unique branch to manage each ROS version and then do all dev on rolling.
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 intended to test different ROS versions on the same branch. Can we keep it as is until we need different branches for different ROS releases?
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.
Absolutely.
c5ffa85
to
3321f02
Compare
Problem Description
This PR adds a build test CI using github actions for ros2
Fixes #29
@srmainwaring Would it make sense to add test for macOS as well?
@Ryanf55 Would you be able to review the PR? Not sure if this is the best way to build test a ROS 2 package :)
Additional context