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

Use GitHub Actions CI #136

Closed
scpeters opened this issue Sep 1, 2020 · 28 comments
Closed

Use GitHub Actions CI #136

scpeters opened this issue Sep 1, 2020 · 28 comments
Assignees

Comments

@scpeters
Copy link
Contributor

scpeters commented Sep 1, 2020

We'd like to try using GitHub Actions CI for maliput and other delphyne dependencies, which will be especially useful after open-sourcing these repositories.

I've started by looking at maliput, which is easiest because it doesn't have any private dependencies. To checkout private GitHub repositories, you need to add a GitHub token as a secret inside the repository, so it's much easier to configure GitHub actions for repositories with only public dependencies.

My first attempt is in the following branch:

It tries to avoid using packages.ros.org by installing python packages using pip and adding ament to the colcon workspace. I ran into some trouble configuring rosdep, so I'm going to try using https://github.com/ros-tooling/action-ros-ci to see if that's easier to configure.

@scpeters scpeters self-assigned this Sep 1, 2020
@scpeters
Copy link
Contributor Author

scpeters commented Sep 4, 2020

I've been experimenting with https://github.com/ros-tooling/action-ros-ci in the following branch:

It recommends using the https://github.com/ros-tooling/setup-ros action step to configure the system for using ROS (installing build tops, adding ROS apt repositories), which I plan to use for configuring base system. The action-ros-ci workflow doesn't have great support for private repositories though. As outlined in ros-tooling/action-ros-ci#224, despite the fact that actions/checkout@v2 can checkout a private repository without configuring an extra token, action-ros-ci does not use this. They added a parameter for specifying a token in ros-tooling/action-ros-ci#252, but I think there's still room for improvement there.

For now, I will use ros-tooling/setup-ros and configure the rest manually.

@agalbachicar
Copy link
Collaborator

Thanks for the update!

@agalbachicar
Copy link
Collaborator

Relates to #135

@scpeters
Copy link
Contributor Author

scpeters commented Sep 4, 2020

It looks like ros-tooling/setup-ros installs DDS implementations, which are not needed, but also shouldn't hurt anything, so I will keep using it for now

@scpeters
Copy link
Contributor Author

scpeters commented Sep 4, 2020

I happened to try a build with ROS eloquent instead of dashing, and it gave the following error:

--- stderr: maliput
CMake Error at /opt/ros/eloquent/share/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake:52 (message):
  ament_target_dependencies() the passed package name 'maliput' was not found
  before
Call Stack (most recent call first):
  test/utilities/CMakeLists.txt:23 (ament_target_dependencies)
  test/utilities/CMakeLists.txt:36 (add_dependencies_to_test)


---

I'm guessing some of the ament syntax has changed since dashing, so we should keep this in mind when updating to a new version of ros2 / ament.

@scpeters
Copy link
Contributor Author

scpeters commented Sep 4, 2020

first approach in maliput/maliput#343

@scpeters
Copy link
Contributor Author

scpeters commented Sep 7, 2020

maliput/maliput#343 has merged. Next steps could include:

  • more CI steps in maliput (clang, sanitizers, linters)
  • adding GitHub actions to other packages (will require access tokens)

@agalbachicar
Copy link
Collaborator

Before moving forward with more CI steps, I think we should take another package, let's say maliput-dragway which consumes maliput as a dependency.

I think a reasonable path forward would be:

  • maliput
  • maliput-dragway
  • maliput-multilane
  • maliput-integration
  • maliput-integration-tests
  • malidrive
  • delphyne
  • delphyne-gui

On a separate note, which linters are you thinking of? We have clang-format that checks the code is properly linted and similarly we have for python. That is already checked via ctests. Am I missing something else?

@scpeters
Copy link
Contributor Author

scpeters commented Sep 8, 2020

Before moving forward with more CI steps, I think we should take another package, let's say maliput-dragway which consumes maliput as a dependency.

I think a reasonable path forward would be:

  • maliput
  • maliput-dragway
  • maliput-multilane
  • maliput-integration
  • maliput-integration-tests
  • malidrive
  • delphyne
  • delphyne-gui

ok, shall I create a new GitHub user account to hold read access to these repositories so we can use its token in the Actions CI?

On a separate note, which linters are you thinking of? We have clang-format that checks the code is properly linted and similarly we have for python. That is already checked via ctests. Am I missing something else?

sorry, I meant static analyzers, like scan-build

@agalbachicar
Copy link
Collaborator

ok, shall I create a new GitHub user account to hold read access to these repositories so we can use its token in the Actions CI?

@stonier @liangfok would you provide us with that account or shall we create it?

@scpeters
Copy link
Contributor Author

scpeters commented Sep 8, 2020

Todo @scpeters: reach out to https://github.com/ros-tooling/setup-ros maintainers to see if they are willing to create a separate action that doesn't install DDS implementations

@scpeters
Copy link
Contributor Author

scpeters commented Sep 8, 2020

Steps required for enabling GitHub Actions CI on a new package (starting with https://github.com/ToyotaResearchInstitute/maliput-dragway)

  1. If it doesn't already exist, create a dedicated GitHub user account
  2. If it doesn't already exist, create a Personal Access Token for that dedicated GitHub user account with repo scope. Be sure to store it somewhere secure because you can only view it immediately after creation.
  3. Grant the dedicated GitHub user account read access to all relevant private repositories. (Strictly speaking, the dedicated GitHub user account only needs read access to the dependencies of the repositories to be tested. For example, if we were only planning to enable Actions CI for maliput-dragway, then it would only need read access to maliput. If you want to set up permissions and secrets once and not have to keep editing them, it's probably easiest to just give read-access to dsim-repos-index and all private repositories in the dsim.repos file).
  4. Add the Personal Access Token created in step 2 as a secret to the repository for which Actions CI is to be enabled with a consistent name like MALIPUT_TOKEN or MALIPUT_READONLY_TOKEN. (For example, this can be added to maliput-dragway at https://github.com/ToyotaResearchInstitute/maliput-dragway/settings/secrets.) maliput doesn't need this secret, but all packages that depend on it will need the secret in order to enable Actions CI.

cc @liangfok @stonier

@scpeters
Copy link
Contributor Author

@scpeters
Copy link
Contributor Author

@scpeters
Copy link
Contributor Author

experimenting with compiling maliput with both gcc and clang in maliput/maliput@b14c695, but it currently only has clang 6 installed. I need to copy the following logic in order to install the correct value of clang (currently version 8):

@scpeters
Copy link
Contributor Author

experimenting with compiling maliput with both gcc and clang in ToyotaResearchInstitute/maliput@b14c695, but it currently only has clang 6 installed. I need to copy the following logic in order to install the correct value of clang (currently version 8):

I submitted an issue ros-tooling/setup-ros#276 requesting a parameter to make it easy for a specific version of clang be installed. Now that I think about it; it might make more sense for this to be done as a separate action, though I don't have any experience making actions.

@scpeters
Copy link
Contributor Author

enabling Actions CI for dragway: maliput/maliput_dragway#9

@agalbachicar
Copy link
Collaborator

it might make more sense for this to be done as a separate action, though I don't have any experience making actions.

I've seen actions that install apt packages but haven't seen one that adds a new repo source. I would try to split this task into:

1- Installation
2- Verify the installation
3- Set the clang alternative
4- Use clang with colcon build.

What do you think?

@agalbachicar
Copy link
Collaborator

Also, I came across this PR: https://github.com/onqtam/doctest/pull/285/files that includes several compilers. It might help.

@agalbachicar
Copy link
Collaborator

agalbachicar commented Sep 29, 2020

@scpeters how difficult would it be to either setup a maliput-actions repository or put all the common stuff in here.

From reviewing all the PRs, I could see that the following is repeated:

  • Base image setup.
  • Checkout public repositories.
  • Checkout private repositories.
  • Dependency installation (mostly ros-related and build system tools).
  • gcc
    • Build.
    • Test.

And in the future we will add:

  • clang
    • Build
    • Test
  • Sanitizers ...

If we can pass the tokens and the repositories when checking out the private repositories and the package names when testing, I think our action files will be considerably reduced.

BTW, I don't think we need to address it now, I think it would be nice to have and understand what it would take.

@scpeters
Copy link
Contributor Author

scpeters commented Oct 1, 2020

@scpeters how difficult would it be to either setup a maliput-actions repository or put all the common stuff in here.

From reviewing all the PRs, I could see that the following is repeated:

  • Base image setup.

  • Checkout public repositories.

  • Checkout private repositories.

  • Dependency installation (mostly ros-related and build system tools).

  • gcc

    • Build.
    • Test.

And in the future we will add:

  • clang

    • Build
    • Test
  • Sanitizers ...

If we can pass the tokens and the repositories when checking out the private repositories and the package names when testing, I think our action files will be considerably reduced.

BTW, I don't think we need to address it now, I think it would be nice to have and understand what it would take.

I think it could be nice to generalize these actions if it can reduce duplication of the configuration and build scripts. At the moment, these actions are still under development and don't involve too much duplication, so I'd prefer to continue with the present setup for now but keep this in mind in case there is more duplication that we want to mitigate

@agalbachicar
Copy link
Collaborator

Status: all repositories that require a gcc build have Github Actions.

@scpeters
Copy link
Contributor Author

our Jenkins CI has some logic to switch branches for package dependencies if they have a branch name matching the current branch of the leaf package to be tested:

this is not enabled for GitHub Actions yet, but it could be. it would require setting fetch-depth: 0 in actions/checkout to checkout the full history of all branches and tags

@agalbachicar
Copy link
Collaborator

About scan_build:

  • Copy to each repository the script to parse the suppression list.
  • List all dependencies' suppression list to each leaf package.

@agalbachicar
Copy link
Collaborator

agalbachicar commented Nov 16, 2020

Per maliput/delphyne#717 (comment) investigation, we should:

  • Fix all the occurrences of uses: ros-tooling/setup-ros@0.0.25, to be:
    - uses: ros-tooling/setup-ros@0.0.25
      env:
        ACTIONS_ALLOW_UNSECURE_COMMANDS: true

See https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ for context about the change.

@scpeters
Copy link
Contributor Author

I think if we bump to ros-tooling/setup-ros@0.0.26, then we won't need the ACTIONS_ALLOW_UNSECURE_COMMANDS workaround. Testing in maliput/maliput#365

@scpeters
Copy link
Contributor Author

our Jenkins CI has some logic to switch branches for package dependencies if they have a branch name matching the current branch of the leaf package to be tested:

this is not enabled for GitHub Actions yet, but it could be. it would require setting fetch-depth: 0 in actions/checkout to checkout the full history of all branches and tags

due to maliput/maliput#368 requiring changes in some downstream packages, I'm adding logic for checking out matching branch names if available in those packages:

it still needs to be added to a few other repositories

@agalbachicar
Copy link
Collaborator

Implementation has finished.

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

No branches or pull requests

2 participants