Support for CI on top of ros2 nightly images by mikaelarguedas · Pull Request #454 · ros-industrial/industrial_ci

mikaelarguedas · 2019-12-19T17:15:24Z

Hi all, I created a way simpler and stupider version of ici for ROS 2 a while back (ros2ci). Main purpose was to be able to run CI on both released ROS 2 distros and ROS 2 nightly builds to catch breakage of packages against upcoming versions of ROS 2.

Now that ICI supports colcon + ROS 2 (hat tip @ipa-mdl 👍🎉), I'd like to integrate the support for nightly builds instead of maintaining something on the side.

I'm opening this to start discussion of supporting nightly ROS 2 CI in ICI.
Feature description:

run jobs based on OSRF's nightly ros2 docker images (Linux only)
build a "middle-lay" with missing dependencies
build and test packages on top of it

Is it a use case ICI would be interested in supporting?

Content of this PR:

bb03f84: Modify ICI to forward declare the ROS 2 distros as soon as they are announced / used in the nightly builds, with a blanket empty "DEFAULT_DOCKER_IMAGE"
c61ecab Extend instead of overriding ROS_PACKAGE_PATH to keep the one set in the upstream image (This should not be needed or merged IMO as thiscan modify behavior for all users and there are likely good reasons to override. This is a hack to work around a current rosdep/ROS2 issue Revert "rever ROS_PACKAGE_PATH hack allowing rosdep to find the ros2 packages" osrf/docker_images#328)

Users wanting to run CI based on nightly builds could modify their travis config as follows:

 env:
   matrix:
     - ROS_DISTRO="dashing" ROS_REPO=main
     - ROS_DISTRO="eloquent" ROS_REPO=main
+    - DOCKER_IMAGE="osrf/ros2:nightly" UPSTREAM_WORKSPACE="additional_repos.repos"

osrf/ros2:nightly being the docker image provided daily by osrf with the content of the latest nightly build
additional_repos.repos the list of upstream packages needed to compile the repository that are NOT part of the nightly image

Example of repo using this and travis job

mathias-luedtke

Thank you very much!
All in all I don't see any problem with merging this 👍

However, I'd like to discuss these two items first.

mathias-luedtke · 2019-12-19T23:23:21Z

industrial_ci/src/env.sh

        ;;
+    "foxy")
+        ros2_defaults "bionic"
+        DEFAULT_DOCKER_IMAGE=


Why not

Suggested change

DEFAULT_DOCKER_IMAGE=

DEFAULT_DOCKER_IMAGE=osrf/ros2:nightly

This way, one can specify DOCKER_IMAGE=osrf/ros2:nightly (for rolling nightly version) or ROS_DISTRO=foxy (for next release only)

Or DOCKER_IMAGE=osrf/ros2:nightly for now?

This way, one can specify DOCKER_IMAGE=osrf/ros2:nightly (for rolling nightly version) or ROS_DISTRO=foxy (for next release only)

I am not sure I understood what the intended result is from these 2 different configurations.
What are the 2 scenarios we are trying to cover with this change?
What would the .travis.yml line would look like for each?

Or DOCKER_IMAGE=osrf/ros2:nightly for now?

This will work more consistently than DEFAULT_DOCKER_IMAGE=osrf/ros2:nightly as the DEFAULT_DOCKER_IMAGE gets overwritten for some ROS_REPO values (including the default one)

A few hidden assumptions / expectations from my part that came up:

Clear at first glance
It should be clear to user which job is a nightly build vs a build against a released distribution
in ros2ci it was done by naming the job nightly

- env: JOB_TYPE=nightly script: - cp additional_repos.repos .ros2ci/ - .ros2ci/travis.bash $JOB_TYPE

Not sure what would be the best for ICI. Maybe having the DOCKER_IMAGE=osrf/ros2:nightly at the beginning of the line is enough 🤷‍♂️

Minimal effort to set up
Should be a one liner for user to set up in their CI system e.g.:

env: matrix: - ROS_DISTRO="dashing" ROS_REPO=main - ROS_DISTRO="eloquent" ROS_REPO=main + - DOCKER_IMAGE="osrf/ros2:nightly" UPSTREAM_WORKSPACE="additional_repos.repos"

Zero maintenance from users
Once added the line should never need to be updated to keep tracking "ROS 2 master"

line should be ROS_DISTRO-free

Clearly separated from release jobs
The job defined will never track a released ROS_DISTRO from debians, if users want to add a job for a new distro they should

env: matrix: - ROS_DISTRO="dashing" ROS_REPO=main - ROS_DISTRO="eloquent" ROS_REPO=main + - ROS_DISTRO="foxy" ROS_REPO=main - DOCKER_IMAGE="osrf/ros2:nightly" UPSTREAM_WORKSPACE="additional_repos.repos"

I am not sure I understood what the intended result is from these 2 different configurations.

test against night build (DOCKER_IMAGE="osrf/ros2:nightly" UPSTREAM_WORKSPACE="additional_repos.repos)

test against ROS2 foxy (ROS_DISTRO="foxy" ROS_REPO=main). Could point to the nightly build right now and can be switched to the testing/main repo as they become available.

Thanks for clarifying 👍

Yes it should work.
Users with all their dependencies in ros2.repos will be fine, the ones with extra dependencies will need to remove the UPSTREAM_WORKSPACE once the packages are available in the distro

ROS_DISTRO="foxy" ROS_REPO=main UPSTREAM_WORKSPACE="additional_repos.repos"

to

ROS_DISTRO="foxy" ROS_REPO=main

mathias-luedtke · 2019-12-19T23:24:24Z

industrial_ci/src/workspace.sh

    fi
    set -o pipefail # fail if rosdep install fails
-    ROS_PACKAGE_PATH=$cmake_prefix_path ici_exec_in_workspace "$extend" "." rosdep install "${rosdep_opts[@]}" | { grep "executing command" || true; }
+    ROS_PACKAGE_PATH=$cmake_prefix_path:${ROS_PACKAGE_PATH} ici_exec_in_workspace "$extend" "." rosdep install "${rosdep_opts[@]}" | { grep "executing command" || true; }


${ROS_PACKAGE_PATH} should be empty at this point

In the case of the nightly image it is not empty osrf/docker_images#328.
But it is a hack because of a rosdep issue, so I dont think this commit should be merged in industrial_ci, this is only propagating the workaround to have a working example for this discussion

I see. Shouldn't this path be included in CMAKE_PREFIX_PATH? I stumbled upon this issue as well and CMAKE_PREFIX_PATH did the trick in my tests. However, I haven't tested it against the nightly build yet.

hmm I think rosdep does only look in ROS_PACKAGE_PATH and AMENT_PREFIX_PATH.
If all the packages in the nightly archive were registered in the ament index it would work (ongoing discussion at ros2/tinyxml_vendor#14)

hmm I think rosdep does only look in ROS_PACKAGE_PATH

That's why I set it to CMAKE_PREFIX_PATH, this works (or worked?) for catkin and ament :)

only use AMENT_PREFIX_PATH

catkin packages do not set AMENT_PREFIX_PATH (?)

So the ROS2 docker images (e.g. ros:dashing) do not set CMAKE_PREFIX_PATH

I know, and they should not set it. I am reading it from setup.bash
This is a temporary hack until rosdep can crawl the ament index.

catkin packages do not set AMENT_PREFIX_PATH (?)

not that I know of. Even if a tool build tool like colcon did it, I wouldn't expect catkin packages to register themselves in the index.

This is a temporary hack until rosdep can crawl the ament index.

This is now a thing ros-infrastructure/rosdep#699
The nightly image sets the ROS_PACKAGE_PATH only because rosdep cannot find non-ament packages like ros2/tinyxml_vendor#14

TL;DR this line can stay as is if we remove the COLCON_IGNORE

I just tested it, the COLCON_IGNORE is breaking the work-around.

If rosdep now support AMENT_PREFIX_PATH, then we don't need to set ROS_PACKAGE_PATH.

#456 now reverts the work-around properly.

I just tested it, the COLCON_IGNORE is breaking the work-around.

In what scenario does it break? Can you clarify how it breaks?

If removing it has a negative effect, I'll reject osrf/docker_images#345

This reverts commit 09618f4.

industrial_ci/src/env.sh

mathias-luedtke

Please rebase this to #456 and commit my suggestion.
If the new version still passes your tests, I will merge both PRs.

As a follow-up, I'd like to rewrite env.sh to allow arbitrary Docker images.
For this to work, the variables have to be parsed in the correct order/phase and not twice as of now.

mikaelarguedas · 2019-12-21T16:08:55Z

Please rebase this to #456 and commit my suggestion.
If the new version still passes your tests, I will merge both PRs.

Haven't tried this yet but I expect #456 to break the use of nightly images.
If the nightly images set the ROS_PACKAGE_PATH it's because some packages like ros2/tinyxml_vendor#14 cannot be found using the AMENT_PREFIX_PATH approach at the moment

So for the nightly builds to work we would have to keep populating the ROS_PACKAGE_PATH here for now

mathias-luedtke · 2019-12-21T16:29:47Z

Haven't tried this yet but I expect #456 to break the use of nightly images.

I tested it locally with https://github.com/Karsten1987/confbot_robot/blob/ca85f937cf51b597477837c4a636dad88fc39a20/.travis.yml

It seems to work (all tests pass), but it displays a lot of connext-related errors/warnings.

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

mikaelarguedas · 2019-12-21T16:49:40Z

It seems to work (all tests pass), but it displays a lot of connext-related errors/warnings.

Oh yeah, it will work because the ROS_PACKAGE_PATH work-around hard coded in the docker image is not being overwritten anymore 👍

I'll rebase on #456 and force-push on this branch

mikaelarguedas · 2019-12-21T16:51:31Z

but it displays a lot of connext-related errors/warnings.

Yeah this is another issue with the OR nightly archive, unrelated to this PR

mathias-luedtke

Oh yeah, it will work because the ROS_PACKAGE_PATH work-around hard coded in the docker image is not being overwritten anymore

Exactly :)

I am not sure if we should list foxy in the list of supported distros.

mikaelarguedas · 2019-12-21T17:17:40Z

I am not sure if we should list foxy in the list of supported distros.

Not sure either. As long as Foxy is not released (or in beta phase) the statement would be misleading as testing against the last nightly != testing against officially released Foxy...

An alternative is to add "ROS 2 nightly builds" to the list of supported distros. It does not qualify as a distro per se but would be more accurate.
I you go that route, linking to an example of usage would be beneficial as it differs from other distros

mathias-luedtke · 2019-12-21T18:20:49Z

An alternative is to add "ROS 2 nightly builds" to the list of supported distros

I'd like to support something like ROS_DISTRO=nightly.

I you go that route, linking to an example of usage would be beneficial as it differs from other distros

For now we should just merge it as-is.
We can update the documentation, once we have tested it further.

Please let me know if your test repo works with this version. I will postpone the merge until then.

mikaelarguedas · 2019-12-21T19:18:31Z

I'd like to support something like ROS_DISTRO=nightly.

Yeah me too, there were some blockers on the docker image side for a while. It may be possible now, would need to check again.

Please let me know if your test repo works with this version.

It works for me!
Thanks for the support on getting this PR ready for merge 👍

mathias-luedtke reviewed Dec 19, 2019

View reviewed changes

mikaelarguedas mentioned this pull request Dec 20, 2019

remove COLCON_IGNORE at root of AMENT_PREFIX_PATH osrf/docker_images#345

Closed

Revert "work-around for chained colcon workspaces"

715284f

This reverts commit 09618f4.

mathias-luedtke mentioned this pull request Dec 21, 2019

Revert "work-around for chained colcon workspaces" #456

Closed

mathias-luedtke reviewed Dec 21, 2019

View reviewed changes

industrial_ci/src/env.sh Outdated Show resolved Hide resolved

mathias-luedtke requested changes Dec 21, 2019

View reviewed changes

add support for foxy / nightly images

2cf0fe4

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

mikaelarguedas force-pushed the nightly_foxy branch from 61a5ccd to 2cf0fe4 Compare December 21, 2019 16:52

mathias-luedtke approved these changes Dec 21, 2019

View reviewed changes

mathias-luedtke merged commit 08435a5 into ros-industrial:master Dec 21, 2019

mikaelarguedas deleted the nightly_foxy branch December 21, 2019 20:18

mathias-luedtke mentioned this pull request Feb 7, 2020

Dependencies of pkgs in underlays do not seem to be discovered/installed during rosdep install #475

Closed

mathias-luedtke mentioned this pull request Apr 7, 2020

work-around for chained colcon workspaces with cmake-only packages #496

Merged

2 tasks

Conversation

mikaelarguedas commented Dec 19, 2019

Uh oh!

mathias-luedtke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mathias-luedtke left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaelarguedas commented Dec 21, 2019

Uh oh!

mathias-luedtke commented Dec 21, 2019

Uh oh!

mikaelarguedas commented Dec 21, 2019

Uh oh!

mikaelarguedas commented Dec 21, 2019

Uh oh!

mathias-luedtke left a comment

Choose a reason for hiding this comment

Uh oh!

mikaelarguedas commented Dec 21, 2019

Uh oh!

mathias-luedtke commented Dec 21, 2019

Uh oh!

mikaelarguedas commented Dec 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mathias-luedtke left a comment •

edited

Loading