ros2 / sros2

tools to generate and distribute keys for SROS 2
Apache License 2.0
89 stars 45 forks source link

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

Closed mikaelarguedas closed 4 years ago

mikaelarguedas commented 4 years ago

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

Looking for feedback on the following:

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 https://github.com/ros-tooling/action-ros-ci/issues/81

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

kyrofa commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

mikaelarguedas commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

ros-discourse commented 4 years ago

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

mikaelarguedas commented 4 years ago

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

mikaelarguedas commented 4 years ago

New version of this PR:

Workflow with 2 jobs:

Example of repo/PR using this: https://github.com/mikaelarguedas/sros2/pull/2

Prerequisites for merging:

Related work (non-blocking):


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

Arnatious commented 4 years ago

LGTM

artivis commented 4 years ago

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 commented 4 years ago

Who's responsibility is this?

Mine, this is already done. We think :stuck_out_tongue: .

artivis commented 4 years ago

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 commented 4 years ago

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

artivis commented 4 years ago

Left a last small comment/question. LGTM :+1:

kyrofa commented 4 years ago

Alright, tests for sros2:

mikaelarguedas commented 4 years ago

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:

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 commented 4 years ago

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.