Migrate to GitHub Actions #69

tylerjw · 2021-03-19T21:08:28Z

This will rescue this repo out of the grasp of Travis. I'll post a link to a fork where it'll run so you can see it pass before merging.

Test is running here: tylerjw#1

tylerjw · 2021-03-19T22:04:47Z

This failed catkin_lint with a wall of errors. I'm going to leave that out of this and it can be a separate PR if you'd like to enable it.

tylerjw · 2021-03-20T16:37:22Z

As you can see at the link these take 45min to complete. Merging this will atleast get you out of the Travis queue.

gavanderhoorn · 2021-03-20T17:42:27Z

As you can see at the link these take 45min to complete.

almost all time is spent building MoveIt from source.

I haven't checked, but is that necessary?

The build of the calibration packages takes about 2 minutes.

Edit: ccache is used. With the cache now populated, are subsequent build faster?

tylerjw · 2021-03-20T17:53:07Z

Edit: ccache is used. With the cache now populated, are subsequent build faster?

It should be, I can test this by restarting the CI run.

almost all time is spent building MoveIt from source.

I haven't checked, but is that necessary?

I'm not sure, it is also building a bunch of stuff from moveit's rosinstall file I'm sure is not needed (like tutorials). I can try configuring this to only build moveit (assuming it even needs that).

tylerjw · 2021-03-20T17:54:20Z

Here is the link to the re-run job: https://github.com/tylerjw/moveit_calibration/pull/1/checks?check_run_id=2156360504

tylerjw · 2021-03-20T17:55:07Z

One thing interesting is the cache size it restored is 22 bytes which makes me think it isn't actually working.

rhaschke

See below. Otherwise lgtm.

rhaschke · 2021-03-21T09:42:06Z

.github/workflows/industrial_ci_action.yml

+          - {ROS_DISTRO: melodic, ROS_REPO: main}
+    env:
+      CCACHE_DIR: ~/.ccache
+      UPSTREAM_WORKSPACE: https://raw.githubusercontent.com/ros-planning/moveit/master/moveit.rosinstall


Instead of building against all MoveIt sources again, just reuse the melodic-source docker container.

I can try that, I'm not sure if that docker container plays nice with industrial_ci, but it might. I'll update this with the fixes I now know for the ccache stuff and uploading any test failures.

@tylerjw so are you going to try it or do you want to have this merged before trying to change the container?

I'll fix this (and some other things I learned about last night) and then I'll post here when I think this is ready.

This is now fixed, we are using the moveit source docker (for now, melodic) to test this and rviz_visual_tools from source and it works.

JStech · 2021-03-21T16:27:47Z

Thanks, everyone, for the input. I'm fine with leaving the catkin_lint fixes for later (#70).

I know in the past the source build was necessary for rviz_visual_tools and moveit_visual_tools (see #23). Building MoveIt from source might not be necessary. I can figure that out, but I don't have bandwidth for this right now. I suggest we just take advantage of ccache for now, and if I figure out that a source build actually is necessary, we can switch to the source container. (#71)

tylerjw · 2021-03-21T22:29:47Z

I just pushed changes with these changes:

ccache will work correctly now (this should speed up builds after the first one)
codecov reporting (does this repo have tests?)
black formatting added to pre-commit (future python files will be nicely formatted now)
uploading test result files (when they fail)

JStech

Looks good to me, although that's not saying much--not my expertise.

JStech · 2021-03-21T22:43:07Z

.clang-format

@@ -8,7 +8,6 @@ AllowAllParametersOfDeclarationOnNextLine: false
 AllowShortIfStatementsOnASingleLine: false
 AllowShortLoopsOnASingleLine: false
 AllowShortFunctionsOnASingleLine: None
-AllowShortLoopsOnASingleLine: false


I was gonna say something about this, but decided to just open #72 instead.

I don't know a better way to do this than copy-pasting it everywhere. This change is because we copy-pasted a yaml error everywhere, this is a duplicate of line 9 above it.

We could do something like what ament_clang_format does from ament_lint and install our .clang-format file as a resource from the moveit_common package or some other similar package, release that as a debian, depend on it, and then change our clang_format commands to use the ros resource lookup commands to find the path to it and use it. That seems like a real hassle though it would solve the problem of this being copy-pasted everywhere.

tylerjw · 2021-03-21T22:55:26Z

Looks good to me, although that's not saying much--not my expertise.

Assuming this passes on my fork I'll merge it and you can get started using it.

tylerjw · 2021-03-21T23:28:52Z

I can't manually merge this (because travis, which no longer runs, is not reporting a status and the settings of this repo require all previous tests to pass). Could someone with the correct permissions in this repo push it?

v4hn

Sorry, I did not participate in your herculean effort to migrate all the repos (thank you! 💐) and can't merge this without further questions.

I guess we should also disable the travis CI jobs in the config upon merging this request?

v4hn · 2021-03-22T11:04:31Z

...bration_rviz_plugin/include/moveit/handeye_calibration_rviz_plugin/handeye_calibration_gui.h

@@ -80,6 +80,6 @@ class HandEyeCalibrationGui : public rviz::Panel
  rviz_visual_tools::TFVisualToolsPtr tf_tools_;
 };

-}  // namedist moveit_rviz_plugin
+}  // namespace moveit_rviz_plugin


Where do all these patches come from? Afaics the clang-format version and config didn't change? 😕

The config changed because now we have a yaml syntax checker and our .clang-format file was invalid and causing clang-format to miss things.

v4hn · 2021-03-22T11:06:05Z

.github/workflows/industrial_ci_action.yml

+          - {ROS_DISTRO: melodic, ROS_REPO: main}
+    env:
+      CCACHE_DIR: ~/.ccache
+      UPSTREAM_WORKSPACE: https://raw.githubusercontent.com/ros-planning/moveit/master/moveit.rosinstall


@tylerjw so are you going to try it or do you want to have this merged before trying to change the container?

tylerjw · 2021-03-22T14:13:30Z

@tylerjw so are you going to try it or do you want to have this merged before trying to change the container?

I don't mind doing it either way. For moveit itself I'm going to need to update the container and there are some other improvements I learned about last night I can apply here to make this more performant.

tylerjw · 2021-03-22T14:14:51Z

I guess we should also disable the travis CI jobs in the config upon merging this request?

Merging this will disable travis for this repo. The reason this can't be merged automatically is GitHub is trying to stop someone from merging things by deleting the ci config (which is exactly what I did). Someone with write access to master has to merge this manually.

.github/workflows/industrial_ci_action.yml

codecov · 2021-03-22T20:22:04Z

Codecov Report

❗ No coverage uploaded for pull request base (master@fedd85b). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #69   +/-   ##
=========================================
  Coverage          ?   72.75%           
=========================================
  Files             ?       10           
  Lines             ?      554           
  Branches          ?        0           
=========================================
  Hits              ?      403           
  Misses            ?      151           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fedd85b...a222428. Read the comment docs.

tylerjw · 2021-03-22T20:28:58Z

@rhaschke I believe this is now ready. I added the codecov badge to the readme (as I think it's a nice thing to have) but I resisted putting the large srv graph of it there. Thank you again for reviewing all these.

tylerjw · 2021-03-22T21:27:34Z

With the combination of the moveit source docker image and upstream repo caching, the CI runs for this is ~4min. 🎉

tylerjw · 2021-03-22T22:02:54Z

I closed and re-opened this to see if I could get rid of the TravisCI requirement. It didn't work.

gavanderhoorn · 2021-03-23T08:19:52Z

.github/workflows/industrial_ci_action.yml

+  industrial_ci:
+    env:
+      DOCKER_IMAGE: 'moveit/moveit:melodic-source'
+      UPSTREAM_WORKSPACE: 'upstream.rosinstall'


Seeing as this is only a single package, something like should also work (and reduce the nr of files the configuration is spread across):

UPSTREAM_WORKSPACE='github:PickNikRobotics/rviz_visual_tools#master'

I'm using a cache of that file to determine if I should re-use the cache of the upstream workspace. If I make this change I don't need that hash, but if someone changes this line then they could introduce confusing situations with the upstream cache.

Short story: that change would make this PR simpler as it is with the sideafect of if someone changes it later confusing things might happen. I could go either way.

Isn't caching workspaces potentially problematic anyway? You run the risk of dependencies changing and your cache becoming invalid. The hash would not change, but you'd either still have to get additional packages installed, or you'd miss the missing dependencies and end up with failed builds.

I'm specifically caching the upstream_ws folder. If the repos are changed (by changing that file) the cache is invalidated and re-created. If instead the repos themselves have changes since the cache was created, the cache gets updated. This is done by the line AFTER_SETUP_UPSTREAM_WORKSPACE: 'vcs pull $BASEDIR/upstream_ws/src' and because the upstream_cache uses ${{ github.run_id }} at the end of the key (meaning that it'll overwrite the cache on each run at the end of the run).

The only risk I see with this approach to caching the upstream if somehow incremental builds of the upstream workspace is problematic. In either case the rosdep step is called after the vcs pull so if there are changed binary dependencies that's handled nicely.

I think the industrial_ci builds attempts to update dependencies from the upstream_ws and build it in any case.
However, if the cached upstream_ws was loaded before, this is most of the time a noop.
Only if there are changes, they will be performed. If you change your dependencies (in upstream.install) the cache becomes invalid automatically. Gijs' approach should work as well - if you hash the industrial_ci_action.yml file.
However, this might invalidate the cache more often then needed (not all changes to this file will change the dependencies). Hence, Tyler's approach is perfectly fine and preferred from my side.

rhaschke · 2021-03-23T14:16:20Z

.travis.yml

-  matrix:
-    - TEST=catkin_lint
-    - UPSTREAM_WORKSPACE=https://raw.githubusercontent.com/ros-planning/moveit/master/moveit.rosinstall
-      TEST_BLACKLIST=moveit_ros_perception


We don't need to disable moveit_ros_perception tests anymore? Cool!

industrial_ci isn't running the upstream tests... but I haven't seen any issues with them when running moveit itself either.

tylerjw requested a review from JStech March 19, 2021 21:08

tylerjw force-pushed the add-github-actions branch 2 times, most recently from 983a8b8 to 8dc5488 Compare March 19, 2021 22:04

rhaschke reviewed Mar 21, 2021

View reviewed changes

JStech added the enhancement New feature or request label Mar 21, 2021

tylerjw force-pushed the add-github-actions branch from 8dc5488 to b9286c8 Compare March 21, 2021 22:27

JStech approved these changes Mar 21, 2021

View reviewed changes

tylerjw force-pushed the add-github-actions branch from b9286c8 to 3baa9ab Compare March 21, 2021 23:48

v4hn reviewed Mar 22, 2021

View reviewed changes

tylerjw force-pushed the add-github-actions branch 3 times, most recently from 091f24d to 46bab1b Compare March 22, 2021 16:28

rhaschke reviewed Mar 22, 2021

View reviewed changes

.github/workflows/industrial_ci_action.yml Show resolved Hide resolved

tylerjw force-pushed the add-github-actions branch from 46bab1b to 7b9f3e7 Compare March 22, 2021 20:15

Migrate to GitHub Actions

86bec9b

tylerjw force-pushed the add-github-actions branch 2 times, most recently from f302f2f to 545436f Compare March 22, 2021 20:28

tylerjw requested a review from rhaschke March 22, 2021 20:29

fix formatting issues

a222428

tylerjw force-pushed the add-github-actions branch from 545436f to a222428 Compare March 22, 2021 20:35

tylerjw closed this Mar 22, 2021

tylerjw reopened this Mar 22, 2021

gavanderhoorn reviewed Mar 23, 2021

View reviewed changes

rhaschke approved these changes Mar 23, 2021

View reviewed changes

rhaschke merged commit 54e3a32 into moveit:master Mar 23, 2021

tylerjw deleted the add-github-actions branch March 23, 2021 14:24

Migrate to GitHub Actions #69

Migrate to GitHub Actions #69

Conversation

tylerjw commented Mar 19, 2021 • edited

tylerjw commented Mar 19, 2021

tylerjw commented Mar 20, 2021

gavanderhoorn commented Mar 20, 2021 • edited

tylerjw commented Mar 20, 2021

tylerjw commented Mar 20, 2021

tylerjw commented Mar 20, 2021

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JStech commented Mar 21, 2021 • edited

tylerjw commented Mar 21, 2021 • edited

JStech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerjw commented Mar 21, 2021

tylerjw commented Mar 21, 2021

v4hn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerjw commented Mar 22, 2021

tylerjw commented Mar 22, 2021

codecov bot commented Mar 22, 2021 • edited

Codecov Report

tylerjw commented Mar 22, 2021

tylerjw commented Mar 22, 2021

tylerjw commented Mar 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerjw commented Mar 19, 2021 •

edited

gavanderhoorn commented Mar 20, 2021 •

edited

JStech commented Mar 21, 2021 •

edited

tylerjw commented Mar 21, 2021 •

edited

codecov bot commented Mar 22, 2021 •

edited