Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

tylerjw
Copy link
Member

@tylerjw tylerjw commented Apr 29, 2021

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:

  • use ccache dir correctly
  • use yaml style lists
  • use yaml file extension
  • make links in README more useful

@tylerjw tylerjw requested a review from rhaschke April 29, 2021 21:55
@tylerjw tylerjw linked an issue Apr 29, 2021 that may be closed by this pull request
@tylerjw tylerjw force-pushed the noetic-pre-release branch 3 times, most recently from e16155c to 5b65354 Compare April 29, 2021 23:06
Copy link
Contributor

@rhaschke rhaschke left a 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.

@tylerjw
Copy link
Member Author

tylerjw commented Apr 30, 2021

I pushed a change to both this and the melodic-devel version that adds moveit as a downshtream dependency in the pre-release test.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments below.

.github/workflows/prerelease.yaml Outdated Show resolved Hide resolved
.github/workflows/prerelease.yaml Show resolved Hide resolved
.github/workflows/prerelease.yaml Outdated Show resolved Hide resolved
@tylerjw tylerjw force-pushed the noetic-pre-release branch 4 times, most recently from 772dc96 to 09c0292 Compare April 30, 2021 16:14
@tylerjw
Copy link
Member Author

tylerjw commented Apr 30, 2021

@tylerjw tylerjw force-pushed the noetic-pre-release branch 2 times, most recently from 3418d63 to a1f8a9b Compare April 30, 2021 16:56
@@ -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'
Copy link
Contributor

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?

Copy link
Member Author

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

@tylerjw
Copy link
Member Author

tylerjw commented Apr 30, 2021

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.

@tylerjw tylerjw force-pushed the noetic-pre-release branch 2 times, most recently from 5c20652 to c454771 Compare May 2, 2021 17:31
@tylerjw
Copy link
Member Author

tylerjw commented May 2, 2021

@rhaschke I think ccache might work in pre-release, or it might be possible to make it work. See: ros-industrial/industrial_ci#667

@tylerjw
Copy link
Member Author

tylerjw commented May 2, 2021

@rhaschke
Copy link
Contributor

rhaschke commented May 3, 2021

I think, this PR might be pending for a while, until ccache issues and test results reporting is fully resolved.
Independent of this, I triggered a release: ros/rosdistro#29384

@rhaschke rhaschke marked this pull request as draft May 3, 2021 09:03
@tylerjw
Copy link
Member Author

tylerjw commented May 3, 2021

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.

Copy link
Contributor

@rhaschke rhaschke left a 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?

README.md Outdated Show resolved Hide resolved
Co-authored-by: Robert Haschke <[email protected]>
@tylerjw tylerjw marked this pull request as ready for review May 3, 2021 19:16
@tylerjw
Copy link
Member Author

tylerjw commented May 3, 2021

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?

I don't know... I should have looked more closely at the output. I have no idea why this would happen.

@vatanaksoytezer
Copy link

Closing since this is merged with #81

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release
3 participants