-
Notifications
You must be signed in to change notification settings - Fork 119
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 pre-release test to CI [master] #70
Conversation
e16155c
to
5b65354
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.
Looks good to me. I thought about adding MoveIt as a downstream repo for the prerelease test.
If we had issues in the past, then because moveit_resources and moveit didn't play nicely together.
5b65354
to
92c3585
Compare
I pushed a change to both this and the melodic-devel version that adds moveit as a downshtream dependency in the pre-release test. |
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.
See comments below.
772dc96
to
09c0292
Compare
3418d63
to
a1f8a9b
Compare
.github/workflows/prerelease.yaml
Outdated
@@ -29,4 +29,4 @@ jobs: | |||
uses: EnricoMi/publish-unit-test-result-action@v1 | |||
if: always() | |||
with: | |||
files: '**/test_results/**/*.xml' | |||
files: '/tmp/**/test_results/**/*.xml' |
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 guess you verified that the test results are actually accessible from outside the docker container?
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 tried doing that here... https://github.com/tylerjw/moveit_resources/runs/2478435289?check_suite_focus=true
It seems that non relative paths are not supported
. That means that the test results have to end up in the repo or workspace directory. I don't think that's possible without changing industrial_ci to run the pre-release job there. I'm going to remove this feature from this pull request.
This was an attempt to show that the pre-release test was failing (even though it reports passing) because of the issue with colcon in the pre-release job. I created an issue for that here: ros-industrial/industrial_ci#666
7aa7df0
to
4679ae2
Compare
Reading the industrial_ci code for pre-release it seems it does support ccache. I added that back in to speed up the pre-release builds. That reporting the test results action only really works when you are making pull requests (becasue what it does is provide summaries of the test results in a comment in the pull request). There are two reasons that's not useful here, we are not running pre-release tests on PRs and that sort of reporting only works when the pull request comes from the repo itself instead of from a fork. There is a workaround in the readme for that second issue but it looks to be fairly nasty. |
5c20652
to
c454771
Compare
@rhaschke I think ccache might work in pre-release, or it might be possible to make it work. See: ros-industrial/industrial_ci#667 |
I think, this PR might be pending for a while, until ccache issues and test results reporting is fully resolved. |
I will remove the ccache from these PRs so we can merge them. I did push tags but didn't bloom because I was unable to get pre-release tests to pass for either noetic or melodic if you include the release branch of moveit in the test. |
c454771
to
3868e7d
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.
In principle, I approve, but the prerelease test you linked is failing with:
2021-05-02T19:17:12.6568260Z /tmp/ws2/src/moveit/moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp:44:10: fatal error: moveit_ros_planning/PlanningSceneMonitorDynamicReconfigureConfig.h: No such file or directory
This is strange as the corresponding file was created before:
2021-05-02T17:59:56.2380108Z Wrote header file in /tmp/ws2/devel_isolated/moveit_ros_planning/include/moveit_ros_planning/PlanningSceneMonitorDynamicReconfigureConfig.h
For some reason, planning_scene_monitor.cpp
is built twice: once in section "build in isolation" and once in section "run_tests". Why the latter and why it cannot find the header anymore?
Co-authored-by: Robert Haschke <[email protected]>
I don't know... I should have looked more closely at the output. I have no idea why this would happen. |
Closing since this is merged with #81 |
This one adds the pre-release test and sets it to run when there is a push to the master branch or when there is a workflow-dispatch. Because it runs on a bush to the master branch it won't block any PRs but it will let us know at any time if the pre-release test is failing on the master branch.
This also cleans up a few other CI things: