colcon-package-selection is old

kenji-miyake commented 3 years ago

Description

Since colcon-package-selection in setup-ros-docker is old, some options like --packages-above-and-dependencies are not supported.

Expected Behavior

Users can use as many options as possible that the latest version has.

Actual Behavior

Users can't use --packages-above-and-dependencies.

To Reproduce

As I've tested on CI, please see the following results.

Current version (0.2.6)

osrf/ros:galactic-desktop passed (because it's using apt-version of colcon) but setup-ros-docker failed.

https://github.com/kenji-miyake/setup-ros-test-colcon-version/pull/1

After manually updating colcon-package-selection from 0.2.6 to 0.2.10

Both osrf/ros:galactic-desktop and setup-ros-docker passed. https://github.com/kenji-miyake/setup-ros-test-colcon-version/pull/2

System (please complete the following information)

OS: Ubuntu 20.04
ROS 2 Distro: Galactic

kenji-miyake commented 3 years ago

I'll send a PR so that it will be equal to https://github.com/ros-tooling/setup-ros/pull/375.

kenji-miyake commented 3 years ago

Just FYI, I've tested to use requirements.txt and dependabot for this repository. https://github.com/kenji-miyake/setup-ros-docker/pull/1

With this, I think we can keep packages up-to-date easily.

@emersonknapp @christophebedard How do you feel about this approach?

christophebedard commented 3 years ago

I do think it's a good idea. I think the versions were pinned to try to guarantee stability, but at some point we should bump the versions. Given the potential for breaking existing workflows (especially since these docker images aren't versioned), I'm just wondering how we can mitigate it. One solution would be to add proper tests in this repo that use action-ros-ci to check that bumping something doesn't break typical workflows. But I'm wondering what @emersonknapp thinks about this.

christophebedard commented 3 years ago

We had a short discussion about this in the 2021-08-27 tooling WG meeting. Just like I mentioned above, we definitely want to update the versions periodically, but we also don't want to break existing workflows.

To do this, we can append a version number to the docker image name, like setup-ros-docker-ubuntu-$UBUNTU_VERSION-ros-$ROS_DISTRO-{ros-base,desktop}-v0-N (or another way to write v0.N that's compatible). Similar to action-ros-ci@master, I think we can also have a -latest suffix for images with dependencies that are always up-to-date. We'll have to keep the existing images as-is but still keep building them in case GPG keys change, etc.

I'm not sure how we can setup the repo to keep building all of these images: if we use a branch for each version, I don't think we can run nightly/scheduled CI jobs to build the images on all of those branches. It looks like scheduled jobs only run on the default branch: https://docs.github.com/en/actions/reference/events-that-trigger-workflows#schedule. So maybe we can simply keep everything on master and use directories for the different versions and have a requirements.txt file in each of those directories.

Tests become less important in our goal to avoid breaking existing workflows if we create "versioned" images, but they're still a good idea. We should put tests using action-ros-ci in the workflow in between building the image and pushing it. This way, if the tests fail, the image won't be pushed.

This will definitely require a bit of work and I unfortunately don't have much time these days. However, we can start by setting up -latest images for Rolling & Galactic to keep it simple and add tests later on. This would solve your problem @kenji-miyake.

emersonknapp commented 3 years ago

use directories for the different versions

Yes, this seems sensible

christophebedard commented 3 years ago

However, we can start by setting up -latest images for Rolling & Galactic to keep it simple

is this something you'd like to do @kenji-miyake? If not, I can probably do it quickly.

kenji-miyake commented 3 years ago

@christophebedard Hmm, I'd like to work on that, but I can't imagine the task well. It seems the images are already named -latest for DockerHub and :master for GitHub Packages. :thinking: I think what we need now is to add versioned images and ask users (who want stability rather than features) to switch to the versioned images.

kenji-miyake commented 3 years ago

One solution would be to add proper tests in this repo that use action-ros-ci to check that bumping something doesn't break typical workflows.

Also, I'd like to know this in a bit more detail to contribute.

Could you list up some tests? What problems do you want to prevent? Is it only https://github.com/ros-tooling/action-ros-ci/issues/615 for now?

christophebedard commented 3 years ago

It seems the images are already named -latest for DockerHub and :master for GitHub Packages.

argh you're right! This doesn't leave us a lot of options if we want to keep similar names. I'm not sure why the name/tag naming isn't the same on Docker Hub and GitHub.

I think what we need now is to add versioned images and ask users (who want stability rather than features) to switch to the versioned images.

I agree that we should consider this. It would be a one-time "stability break" probably only for the people using an old version of action-ros-ci affected by https://github.com/ros-tooling/action-ros-ci/issues/615 and it would allow us to set up a better versioning/naming system.

If we really want to avoid stability breaks, we could also abandon the -latest/:master names (i.e., leave the dependencies as-is but keep building the images periodically) and switch to another word that means the same thing to represent the real "latest" images with dependencies that are updated regularly. Maybe main?

..unless we swap the names by changing the Docker Hub images to -master and the GitHub images to :latest :laughing:

@emersonknapp what do you think?

Could you list up some tests? What problems do you want to prevent? Is it only ros-tooling/action-ros-ci#615 for now?

I don't think we should specifically try to prevent https://github.com/ros-tooling/action-ros-ci/issues/615 because it shouldn't happen anymore with newer versions of action-ros-ci. And I don't think we should test older versions of action-ros-ci, because we know it requires some older versions of colcon-* packages from setup-ros*, and if users modify their CI config I would expect them to bump all their actions/Docker images.

We can simply take & adapt some of the tests from action-ros-ci that represent typical use-cases relating to colcon/vcs/rosdep: https://github.com/ros-tooling/action-ros-ci/blob/master/.github/workflows/test.yml. That should prevent most problems.

emersonknapp commented 3 years ago

I agree that we should leave the existing names as-is. No breaking existing workflows.

If we're going to change names, I have some ideas about updating the scheme:

${OS}-${ROS_DISTRO}-${ROS_VARIANT}_v${VERSION}

For OS, skip the distro and just use ROS distro mapping
For variants I would like to provide all 3 plus a "noinstall" option that doesn't install a variant. (open to changes on the naming). This would replace the images called e.g. ubuntu-xenial-latest. Also note that this means ubuntu-foxy-noinstall and ubuntu-galactic-noinstall and (for the moment right now) ubuntu-rolling-noinstall would be (nearly?) identical. I think this is an OK tradeoff for readability/writeability of the tags. Realistically every build job is targeting one ROS distro even if there aren't binaries preinstalled.
For version, I think for this type of product it makes sense to skip minor versions and just use a single increasing number. For a rolling one, i'd be fine with devel, main, newest, or even something like vX to just say it's "variable".

This would give us e.g.

ubuntu-foxy-noinstall_v1
ubuntu-foxy-ros-core_v1
ubuntu-foxy-ros-base_v1
ubuntu-foxy-desktop_v1
ubuntu-galactic-noinstall_devel
ubuntu-rolling-desktop_devel

With this change, we should definitely add a README explaining how to use it, and noting the "legacy" names.

kenji-miyake commented 3 years ago

@emersonknapp The naming rule generally looks good to me! :+1: But I have some questions/suggestions.

Are there any specific reasons for using _ before versions instead of -?
How about bare or base instead of noinstall?
For version, how do you manage/express the correspondence with setup-ros?

christophebedard commented 3 years ago

${OS}-${ROS_DISTRO}-${ROS_VARIANT}_v${VERSION}

That looks good!

How about bare or base instead of noinstall?

base might be confusing (because of the ros-base variant), but I think bare would be good.

emersonknapp commented 3 years ago

How about bare or base instead of noinstall?

bare feels less clear to me personally - i'm still partial to noinstall because it means "no installed variant binaries". Maybe novariant? Maybe empty, but that sounds misleading too

For version, how do you manage/express the correspondence with setup-ros?

I'm not sure - that's a good point. I wonder if we should also move setup-ros to a simple integer version too :)

ros-tooling / setup-ros-docker