-
Notifications
You must be signed in to change notification settings - Fork 516
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
Migrate from Travis to GitHub Actions #396
Conversation
b1f46e9
to
b8c8f5b
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.
Thanks for this great work. I have some open questions, though.
moveit2: | ||
type: git | ||
url: https://github.com/ros-planning/moveit2 | ||
version: main |
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 see your point of only listing upstream stuff here. But, removing the main source will probably invalidate our build-from-source instructions, which essentially reads like this: "Fetch moveit2.repos and build." They should be adapted then.
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'll prepare a PR for that. This being here breaks the CI system because it creates a vcs error when it tries to import this file and moveit2 is already there.
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.
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.
What about vcs import --skip-existing
?
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.
This caused the docker images to not have moveit2 source code, I think we should revert and apply @henningkayser suggestion, @rhaschke @tylerjw what do you think .?
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.
As @tylerjw already pointed out, vcs import
is performed by industrial_ci
. What about adding the MoveIt source explicitly in the docker image?
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.
Because the docker build is running from the repo you should be able to use the docker COPY command to get the moveit repo in docker. See this line in the nav2 docker file: https://github.com/ros-planning/navigation2/blob/e13459441dd4895450b2f2073f73bf340823fcfe/Dockerfile#L26
480ed0e
to
0c9563a
Compare
Codecov Report
@@ Coverage Diff @@
## main #396 +/- ##
==========================================
- Coverage 53.37% 52.99% -0.37%
==========================================
Files 210 210
Lines 18814 22511 +3697
==========================================
+ Hits 10040 11928 +1888
- Misses 8774 10583 +1809
Continue to review full report at Codecov.
|
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.
More comments.
with: | ||
files: ${{ env.BASEDIR }}/coverage.info | ||
- name: prepare target_ws for cache | ||
if: ${{ always() && ! env.CCOV }} |
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.
What's the meaning of always()
here? Seems to be redundant.
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.
Always causes this to run even when previous steps failed (such as tests in industrial_ci).
0c9563a
to
aa0929e
Compare
This one has been updated per feedback and is ready for another review @rhaschke. |
aa0929e
to
e8f0a8d
Compare
Rebased and pushed as there was a different change merged. |
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.
Rebased and pushed as there was a different change merged.
Sorry ;)
... PoseTracking is also flaky, you might need to disable that one as well.
IKFAST_TEST: true | ||
CLANG_TIDY: true | ||
env: | ||
DOCKER_IMAGE: moveit/moveit2:${{ matrix.env.IMAGE }} |
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
moveit2: | ||
type: git | ||
url: https://github.com/ros-planning/moveit2 | ||
version: main |
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.
What about vcs import --skip-existing
?
@@ -7,6 +7,8 @@ on: | |||
workflow_dispatch: | |||
pull_request: | |||
push: | |||
branches: | |||
- main |
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.
Should this include other branches as well, for instance feature/hybrid-planning
after that one has been rebased?
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 didn't know there were other branches that might be rebased on main. I'll add that one.
no, because I'm not calling vcs import, industrial_ci is. |
977a72f
to
7fd8848
Compare
I disabled it too for now. I'll create issues for the flaky tests in servo. |
@henningkayser The newest failure seems maybe related to one of the things you merged Do you think that caused it? Any ideas on how to fix this? |
I pushed changes suggested by @JafarAbdi. Hopefully this fixes the newly broken test. |
It is still failing, @henningkayser @JafarAbdi any ideas? |
3e70582
to
4968370
Compare
Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
4968370
to
1a1a8c8
Compare
Description
This is finally here. There two features of this that are temporary. First
clang-tidy
filtering is implemented in my PR to industrial_ci here: ros-industrial/industrial_ci#649. I'd appreciate any reviews of that to get it merged and cleaned up. But until that happens we can use my fork.Secondly, preparing the code coverage report is being done by a script. Eventually after this PR on industrial_ci is merged we won't need that script.
Lastly, this PR is here on the repo itself (and not from my fork) so the Actions jobs will run here.
Thank you @rhaschke for helping me get this here. I'll make a copy of this over on moveit1 also.
Also, this disables a test in moveit_servo. That test has become flaky recently and I don't know why. I'm disabling it here so it doesn't get in the way. I will work on re-enabling and debugging it in the future.