Add GitHub actions for linting and source-build CI #178

mikaelarguedas · 2020-03-16T16:50:55Z

Currently Ubuntu only and no xmllint test for sros2 due to schema hosting issue.

Looking for feedback on the following:

the test workflow runs daily
the lint workflow runs on push and PR events only
log upload: usefulness of uploading colcon logs?
Some repositories using the ROS github actions upload the colcon logs (e.g. rosbag2) but they upload them only on job success, which doesn't seem very useful. I opted to not add that upload step until we come across cases where we need the logs.

Examples of jobs using these parameters:
Build + test: https://github.com/mikaelarguedas/sros2/actions/runs/56872161
Lint: https://github.com/mikaelarguedas/sros2/actions/runs/56872157

Failing on Macos for now, haven't tried windows:

E   RuntimeError: Failed to initialize init_options: failed to find shared library of rmw implementation. Searched rmw_fastrtps_cpp, at /Users/runner/runners/2.165.2/work/sros2/sros2/ros_ws/src/ros2/rmw_implementation/rmw_implementation/src/functions.cpp:61, at /Users/runner/runners/2.165.2/work/sros2/sros2/ros_ws/src/ros2/rcl/rcl/src/rcl/init_options.c:55

Possible reason: environment may be sanitized (maybe SIP is enabled) so the relevant libs are not on the DYLD_LIBRARY_PATH: upstream ticket ros-tooling/action-ros-ci#81

@kyrofa did you ever succeed to run some ROS nodes on MacOS using this action ?

kyrofa · 2020-03-16T16:59:48Z

MacOS and Windows have been a bit flaky. We're still running them in nodl, and MacOS passes, but Windows consistently fails due to Fast RTPS not building. We only require Linux to pass given the flakiness.

mikaelarguedas · 2020-03-16T17:03:17Z

We're still running them in nodl, and MacOS passes

Oh nice. Do your tests run any ROS Nodes or are they just pure python tests not relying on a running ROS system ?

kyrofa · 2020-03-16T17:04:16Z

I believe they're just pure Python at the moment, which probably explains the difference.

mikaelarguedas · 2020-03-18T10:52:42Z

This is now uploading colcon logs only on failure.

build and test: https://github.com/mikaelarguedas/sros2/actions/runs/58114846
lint: https://github.com/mikaelarguedas/sros2/actions/runs/58114842

mikaelarguedas · 2020-03-19T17:08:23Z

I kinda prefer just having one, but what do you think?

That's a good point.

I default to multiple files because one issue I have with single workflow is that it means same triggers for all jobs in the workflow. You can do some conditionals on triggers but it quickly becomes limited.This is why I use different ones in e.g. https://github.com/osrf/docker_images#table-of-contents.

In this case it doesn't make much of a difference, merging them into the same workflow means triggering linter jobs along the nightly builds and test, which is totally fine.

kyrofa

Looking good @mikaelarguedas, thank you! I have a few inline comments.

Also, note that having separate test files means you also have separate pass/fail badges:

Linter:
Tests:

I kinda prefer just having one, but what do you think?

.github/workflows/lint.yml

.github/workflows/test.yml

kyrofa · 2020-03-19T18:04:39Z

I default to multiple files because one issue I have with single workflow is that it means same triggers for all jobs in the workflow.

Good point, that makes sense.

In this case it doesn't make much of a difference, merging them into the same workflow means triggering linter jobs along the nightly builds and test, which is totally fine.

Sounds like neither of us feel too strongly about this. Let's ask this, then: do we want to show that badge somewhere? In the README, maybe? Do we care about them at all? If we do, let's make it one job so we only have one to show. If we don't, let's leave it as two.

Personally I think it's more helpful to get a code quality/health impression from the readme as opposed to "are the tests passing right at this moment" (because note that the cron job updates the badges as well). So maybe we revisit that when we have coverage setup.

mikaelarguedas · 2020-03-19T18:11:08Z

Sounds like neither of us feel too strongly about this. Let's ask this, then: do we want to show that badge somewhere? In the README, maybe? Do we care about them at all? If we do, let's make it one job so we only have one to show. If we don't, let's leave it as two..

Sounds good to me, it's now in the README.

.github/workflows/ci.yml

ros-discourse · 2020-03-20T20:41:01Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-03-18/13313/1

kyrofa

I have one last nit/question, but I'm really +1 either way.

.github/workflows/ci.yml

mikaelarguedas · 2020-03-24T16:41:43Z

I left this PR hanging the last couple days as ros2 builfarm switched to 20.04 over the weekend. This puts a bigger question mark on the relevance of this set of actions.
I'm exploring alternatives e.g. https://github.com/mikaelarguedas/sros2/actions/runs/61362217 that allow to run faster and match closely the buildfarm because based on the nightly builds.

Leaving this one on hold for now

- One source build to test against latest master - One build on top of nightly image to give quick feedback Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

mikaelarguedas · 2020-03-27T08:35:57Z

New version of this PR:

Workflow with 2 jobs:

job building on top of osrf/ros2:nightly, provides quick feedback for failures such as linters (~2min)
job building from source on top of osrf/ros2:devel, takes longer and provides more accurate representation of the code if PR was merged now (~20min)
- provides coverage information with feedback on the PR
- runs integration tests (test_security package, though disabled for now as they are not passing on Focal)

Example of repo/PR using this: mikaelarguedas#2

Prerequisites for merging:

enable codecov.io for this repository

Related work (non-blocking):

coverage:
- mixin add coverage-py mixin colcon/colcon-mixin-repository#21
- colcon default reporting: print python coverage results to console colcon/colcon-core#331
- action-ros-ci: use colcon option for pytest coverage instead of custom pytest-cov options ros-tooling/action-ros-ci#109

Future work:
There is still custom logic in this repo. Ideally we could modify the ros-tooling actions to meet our need and limit custom code to a minimum, some examples of improvements

switch back to use upstream action-ros-ci
improve action-ros-ci (non exhaustive):
- standalone: action should install its own required dependencies
- more control to users: not hardcoding colcon arguments or allow users to override any argument
- consistent failure modes (colcon lcov-result --initial fails but doesn't fail the build but colcon lcov-result does o_0)
- allow to specify rosdep arguments
- mode using current build dir and not pulling any repos file
- print test result summary
cache docker images daily to avoid pulling (80% of the time of the nightly job)

mikaelarguedas · 2020-03-27T19:10:21Z

sros2/.coveragerc

+[run]
+omit =
+    # omit test directory
+    test/*


I did this to get accurate coverage numbers. Code in test is usually pretty well covered and skewing the numbers

mikaelarguedas · 2020-03-27T19:11:13Z

.codecov.yml

@@ -0,0 +1,2 @@
+fixes:
+  - "ros_ws/src/sros2/::"


This rewrites the path of the files in the report so that we can browse them directly on the codecov.io interface and see which lines/conditionals are covered and which are not

.github/workflows/test.yml

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

Arnatious · 2020-03-31T16:47:48Z

LGTM

artivis · 2020-03-31T17:51:21Z

About the prerequisite

enable codecov.io for this repository

Who's responsibility is this? I don't know codecov.io very well, can we create groups or whatnot so that the admin work is shared?

kyrofa · 2020-03-31T17:52:06Z

Who's responsibility is this?

Mine, this is already done. We think 😛 .

artivis · 2020-03-31T18:02:34Z

Mine, this is already done.

Ok, good to know.
We'll have to make sure that this can be managed by other WG members in case it is necessary.

kyrofa · 2020-03-31T19:07:59Z

It was actually already enabled and (at least we think) actions don't need a token, so there's really nothing to do there.

README.md

artivis · 2020-03-31T19:11:23Z

Left a last small comment/question.
LGTM 👍

kyrofa · 2020-03-31T22:41:51Z

Alright, tests for sros2:

Linux
Linux-aarch64
macOS
Windows

mikaelarguedas · 2020-04-01T10:41:29Z

Mine, this is already done. We think stuck_out_tongue .

So apparently it was not done ^^
it does upload reports to codecov, but it does not process and post back progress on the PRs.

2 ways to do it:

integrate the codecov github app for ros2/sros2
add a webhook on ros2/sros2 for codecov to use

App integration seems the way to go these days but may require permission at the org level asd the app doesnt seem to be installed on the ros2 org.

Is there a preference from @ros2/team or @ros2/security-members for one approach or the other ?

Edit: the webhook allows to post comment automatically on PR. But so far didnt allow to browse files on the codecov.io interface

kyrofa · 2020-04-01T14:57:27Z

Huh, maybe that's why it looked like it was already enabled, the default settings didn't need the app installed. I prefer that, of course, but it's up to the org admins.

mikaelarguedas force-pushed the github-actions branch 2 times, most recently from 311a7da to 42ff12a Compare March 18, 2020 10:34

mikaelarguedas requested review from kyrofa and ruffsl March 18, 2020 10:35

mikaelarguedas requested a review from artivis March 19, 2020 16:37

kyrofa reviewed Mar 19, 2020

View reviewed changes

.github/workflows/lint.yml Outdated Show resolved Hide resolved

.github/workflows/test.yml Outdated Show resolved Hide resolved

kyrofa reviewed Mar 19, 2020

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

mikaelarguedas force-pushed the github-actions branch from 43f40ac to e5fb4d7 Compare March 19, 2020 18:10

mikaelarguedas commented Mar 19, 2020

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

kyrofa reviewed Mar 19, 2020

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

artivis reviewed Mar 19, 2020

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

kyrofa mentioned this pull request Mar 19, 2020

Update maintainer to point to ros-security mailing list + fix package.xml #179

Merged

mikaelarguedas requested review from artivis and kyrofa March 20, 2020 22:26

kyrofa reviewed Mar 20, 2020

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

mikaelarguedas added 6 commits March 26, 2020 22:41

Use github actions to perform CI

4ff765c

- One source build to test against latest master - One build on top of nightly image to give quick feedback Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

comment integration tests until fastrtps is fixed

0db80e5

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

comment out end-to-end testing for now

9cbfe85

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

add badges to readme

0fe2eec

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

use ROS_DISTRO env var

25126f6

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

test sequentially and print to console

bbda7ff

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

run after nightly docker images rebuild

3963861

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

mikaelarguedas force-pushed the github-actions branch from 3cb4573 to 3963861 Compare March 27, 2020 08:36

mikaelarguedas commented Mar 27, 2020

View reviewed changes

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

kyrofa reviewed Mar 27, 2020

View reviewed changes

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

kyrofa reviewed Mar 27, 2020

View reviewed changes

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

kyrofa reviewed Mar 27, 2020

View reviewed changes

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

add comment about using upstream action-ros-ci

535a700

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

mikaelarguedas requested a review from a team March 31, 2020 08:25

kyrofa approved these changes Mar 31, 2020

View reviewed changes

artivis reviewed Mar 31, 2020

View reviewed changes

README.md Show resolved Hide resolved

Arnatious approved these changes Mar 31, 2020

View reviewed changes

artivis approved these changes Mar 31, 2020

View reviewed changes

kyrofa merged commit 9e1d105 into ros2:master Apr 1, 2020

mikaelarguedas deleted the github-actions branch April 1, 2020 09:06

mikaelarguedas mentioned this pull request Jun 5, 2020

Eloquent backports #222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GitHub actions for linting and source-build CI #178

Add GitHub actions for linting and source-build CI #178

mikaelarguedas commented Mar 16, 2020 •

edited

kyrofa commented Mar 16, 2020

mikaelarguedas commented Mar 16, 2020

kyrofa commented Mar 16, 2020

mikaelarguedas commented Mar 18, 2020

mikaelarguedas commented Mar 19, 2020

kyrofa left a comment

kyrofa commented Mar 19, 2020 •

edited

mikaelarguedas commented Mar 19, 2020

ros-discourse commented Mar 20, 2020

kyrofa left a comment

mikaelarguedas commented Mar 24, 2020

mikaelarguedas commented Mar 27, 2020 •

edited

mikaelarguedas Mar 27, 2020

mikaelarguedas Mar 27, 2020

Arnatious commented Mar 31, 2020

artivis commented Mar 31, 2020

kyrofa commented Mar 31, 2020 •

edited

artivis commented Mar 31, 2020

kyrofa commented Mar 31, 2020

artivis commented Mar 31, 2020

kyrofa commented Mar 31, 2020

mikaelarguedas commented Apr 1, 2020 •

edited

kyrofa commented Apr 1, 2020

Add GitHub actions for linting and source-build CI #178

Add GitHub actions for linting and source-build CI #178

Conversation

mikaelarguedas commented Mar 16, 2020 • edited

kyrofa commented Mar 16, 2020

mikaelarguedas commented Mar 16, 2020

kyrofa commented Mar 16, 2020

mikaelarguedas commented Mar 18, 2020

mikaelarguedas commented Mar 19, 2020

kyrofa left a comment

Choose a reason for hiding this comment

kyrofa commented Mar 19, 2020 • edited

mikaelarguedas commented Mar 19, 2020

ros-discourse commented Mar 20, 2020

kyrofa left a comment

Choose a reason for hiding this comment

mikaelarguedas commented Mar 24, 2020

mikaelarguedas commented Mar 27, 2020 • edited

mikaelarguedas Mar 27, 2020

Choose a reason for hiding this comment

mikaelarguedas Mar 27, 2020

Choose a reason for hiding this comment

Arnatious commented Mar 31, 2020

artivis commented Mar 31, 2020

kyrofa commented Mar 31, 2020 • edited

artivis commented Mar 31, 2020

kyrofa commented Mar 31, 2020

artivis commented Mar 31, 2020

kyrofa commented Mar 31, 2020

mikaelarguedas commented Apr 1, 2020 • edited

kyrofa commented Apr 1, 2020

mikaelarguedas commented Mar 16, 2020 •

edited

kyrofa commented Mar 19, 2020 •

edited

mikaelarguedas commented Mar 27, 2020 •

edited

kyrofa commented Mar 31, 2020 •

edited

mikaelarguedas commented Apr 1, 2020 •

edited