-
Notifications
You must be signed in to change notification settings - Fork 804
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 ROS build CI #868
Fix ROS build CI #868
Conversation
I didn't realize there were nested submodule locaed at Fixed it and you can see it working here: https://github.com/AustinDeric/PX4-SITL_gazebo/runs/6746720074 |
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.
Thanks for the contribution! I apologize on myside that the CI has been broken for a while.
I didn't realize there were nested submodule locaed at external/OpticalFlow/external/klt_feature_tracker. I'd recommend these external ROS dependencies get installed canonically and not inside this package. This can be done with a apt install, a .repos file using vcstool or a submodule that doesn't install inside this package.
This is only relevant when building this package as part of ROS. This repository is a submodule of the PX4-Autopilot project, which does not depend on ROS. Therefore we cannot access the ros distributed package
|
||
RUN source "/opt/ros/${ROS_DISTRO}/setup.bash" && \ | ||
export CPLUS_INCLUDE_PATH=/mavlink/include/mavlink/v2.0 && \ | ||
colcon build |
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 am not very familiar with colcon, but this is build testing ROS1 versions with colcon, which was not the intention of the catkin build tests. Could you elaborate on what we would gain switching to colcon and whether it would verify also building with catkin?
matrix: | ||
config: | ||
- {rosdistro: 'melodic', container: 'px4io/px4-dev-ros-melodic:2021-05-31'} | ||
- {rosdistro: 'noetic', container: 'px4io/px4-dev-ros-noetic:2021-05-31'} |
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.
One of reasons we want to use this container is to align the CI containers and check whether SITL is functional.
Thanks for the reviews! unfortunately i don't have the time to address them but i hope the code in this PR can help y'all in the future. The PX4 ignition gazebo framework lines up more closely with my interests so i am going to focus my limited time there: https://github.com/Auterion/px4-simulation-ignition. |
fixes this bug which is currently in the master branch's CI: https://github.com/PX4/PX4-SITL_gazebo/runs/6721857960
The bug was reproduced in this PRs commit: AustinDeric@3236491.
The next commit fixes this bug: AustinDeric@9b6cdbd
These 2 commits fix the original CI bug identified in the master.
Its notable that there is a different bug that this exposes:
(there are more but its similar, I edited for clarity)
This is the bug that i want to fix, but there was no was to reproduce them. This PR enables reproducing these bugs. So this is the 1st PR of many.