Closed matthew-reynolds closed 3 years ago
@mateus-amarante I see that in #478, you toyed with having the UPSTREAM_WORKSPACE as part of only job 3 or as global. Any reason you decided on leaving it in the global?
After having gone through the CI for the other repos, I think we should actually just be testing the downstream ros_controllers
in every config of the matrix. So something like:
env: # <-- This is basically 'global'
# Test downstream package ros_controllers
UPSTREAM_WORKSPACE: 'ros_control.rosinstall -ros_control -ros_controllers'
DOWNSTREAM_WORKSPACE: 'ros_control.rosinstall -ros_control -realtime_tools -control_toolbox -control_msgs'
strategy:
matrix:
env:
- {ROS_DISTRO: noetic, ROS_REPO: main}
- {ROS_DISTRO: noetic, ROS_REPO: testing}
@mateus-amarante @bmagyar Thoughts on this? I know you guys just added this downstream test fairly recently, so I'd like your input :)
@mateus-amarante I see that in #478, you toyed with having the UPSTREAM_WORKSPACE as part of only job 3 or as global. Any reason you decided on leaving it in the global?
I don't remember why I did that :sweat_smile:. I think it makes more sense to have UPSTREAM_WORKSPACE only in job 3 (source-build). Looking at #478, I noticed it was my first trial in 9d5c2bb83231e01a302a8aa55c04882737c90612, but it failed. However, I don't think this failure is related to the removal of the USPSTREAM_WORKSPACE from the global env.
I changed the downstream ros_controllers job config from DOWNSTREAM_WORKSPACE='ros_control.rosinstall -ros_control -realtime_tools -control_toolbox -control_msgs' to just DOWNSTREAM_WORKSPACE='github:ros-controls/ros_controller#noetic-devel'. It's a lot cleaner, at the small cost of explicitly specifying the branch rather than reading it from the rosinstall file.On second thought, to stay consistent with all the other repos, let's just stick to using the rosinstall file.
By the way, I also tried the notation DOWNSTREAM_WORKSPACE='github:ros-controls/ros_controller#noetic-devel
in 9d5c2bb83231e01a302a8aa55c04882737c90612. I removed it because I couldn't make it work at that time. I think specifying the ros_controllers
repo directly is much better for readability.
After having gone through the CI for the other repos, I think we should actually just be testing the downstream ros_controllers in every config of the matrix.
I agree with your proposal! Additionally, I think we could also set ros_controllers
as downstream to all other ros_controls' packages like realtime_tools
and control_toolbox
I agree with your proposal! Additionally, I think we could also set ros_controllers as downstream to all other ros_controls' packages like
realtime_tools
andcontrol_toolbox
Great! I've updated this PR accordingly. I did the same for the other packages too, adding downstream tests where appropriate:
https://github.com/ros-controls/control_msgs/pull/51
(I didn't add downstream tests to https://github.com/ros-controls/urdf_geometry_parser/pull/19 since that package isn't quite as tightly integrated as the rest)
I think specifying the ros_controllers repo directly is much better for readability.
I landed on sticking with using the rosinstall file since it makes it easier when you have multiple downstream packages to test, and since some of our packages use one branch for multiple distros (eg. kinetic-devel
for Kinetic, Melodic, and Noetic) and thus need to run downstream tests in multiple distros at once (See the control_msgs
and control_toolbox
PRs linked above, for example).
Switch over from Travis CI to GH Actions for our CI.
I also reworked the tests a little:
UPSTREAM_WORKSPACE
since this repo doesn't depend oncontrol_toolbox
orrealtime_tools
or anything. This should speed things up a little.UPSTREAM_WORKSPACE
is only in the "test downstream" job config, since that's the only place it's needed.ros_controllers
job config fromDOWNSTREAM_WORKSPACE='ros_control.rosinstall -ros_control -realtime_tools -control_toolbox -control_msgs'
to justDOWNSTREAM_WORKSPACE='github:ros-controls/ros_controller#noetic-devel'
. It's a lot cleaner, at the small cost of explicitly specifying the branch rather than reading it from the rosinstall file.~ On second thought, to stay consistent with all the other repos, let's just stick to using the rosinstall file.