fix humble Dockerfiles - use humble, not rolling

[adlarkin]

Description

The humble Dockerfiles had the ROS distro set to rolling, causing issues in downstream repositories (for example, CI on moveit/moveit2_tutorials#644 would fail since there's a mismatch of ROS versions when trying to build the tutorials). This PR fixes the ROS version for the humble Dockerfiles, hopefully resolving any unexpected errors for users of these Dockerfiles as well.

Edit: The ci-testing Dockerfile uses osrf/ros2:testing as its base image. This base image has the ROS_DISTRO environment variable set to rolling. This PR updates the ci-testing Dockerfile to allow overwriting of the base image's ROS_DISTRO environment variable, so that the ci-testing can properly build both humble and rolling docker images (before this PR, humble-ci-testing images actually had rolling installed on them). This also fixes the humble-source images being built, since the source images use ci-testing as its base image.

Checklist

• Required by CI: Code is auto formatted using clang-format
• Extend the tutorials / documentation reference
• Document API changes relevant to the user in the MIGRATION.md notes
• Create tests, which fail without this PR reference
• Include a screenshot if changing a GUI
• While waiting for someone to review your request, please help review another open pull request to support the maintainers

[mergify]

Please target the main branch for development, we will backport the changes to humble for you if approved and if they don't break API.

[vatanaksoytezer]

@adlarkin Thank you for the PR. It defaults to rolling but is overridden by CI to humble so there should be nothing wrong with the dockerfiles. See https://github.com/ros-planning/moveit2/blob/main/.github/workflows/docker.yaml#L52. Is there any specific error you have encountered?

[adlarkin]

@adlarkin Thank you for the PR. It defaults to rolling but is overridden by CI to humble so there should be nothing wrong with the dockerfiles. See https://github.com/ros-planning/moveit2/blob/main/.github/workflows/docker.yaml#L52. Is there any specific error you have encountered?

Thanks for pointing me to this, I did not see that originally. I took a closer look, and found that if I 1) pulled each of the moveit humble Docker images, 2) started a container, and then 3) ran echo $ROS_DISTRO in the container, I get the following output for each image:

• moveit/moveit2:humble-source: rolling
• moveit/moveit2:humble-ci-testing: rolling
• moveit/moveit2:humble-release: humble
• moveit/moveit2:humble-ci: humble

I also verified that the ROS_DISTRO for each of the images above matches the ROS version in the binary packages that are installed (you can check this by running apt list | grep -i ros | grep installed in each container). So, it seems like the humble-release and humble-ci images are okay, but the humble-ci-testing and humble-source images are not. It looks like the issue with the humble-ci-testing image is that it uses osrf/ros2:testing as its base image, which has the ROS_DISTRO hardcoded to rolling (so, even though we are overwriting the ROS_DISTRO build arg, this does not change the fact that we are using a base image that has been configured for rolling). And since the humble-source image uses humble-ci-testing as its base image, this causes the ROS_DISTRO for humble-source to be rolling as well.

So, it appears that using the osrf/ros2:testing image as the base image for ${ROS_DISTRO}-ci-testing is the actual issue here. My initial thought for solving this would be to use ros:${ROS_DISTRO}-ros-base as the base image for moveit/moveit2:ci-testing, but I noticed that this appears to be problematic based on #1994. To confirm that osrf/ros2:testing seems to be the source of the issue here, I created my own Dockerfile that combined the contents of osrf/ros2:testing and moveit/moveit2:humble-ci-testing, but I set the ROS_DISTRO environment variable from osrf/ros2:testing to humble (I set the build arg for ROS_DISTRO to humble as well). After doing this, I launched a container with the image built from this Dockerfile and verified that ROS humble binary packages were installed, and that ROS_DISTRO was correctly set to humble.

@vatanaksoytezer hopefully everything I have explained here makes sense, but if not, feel free to ask any questions you may have. It seems like we should address the osrf/ros2:testing base image issue to get this resolved, but I'm not sure what the proper fix is. Do you have any thoughts on how we can resolve this? Once we come to a proper solution, I will close this PR since I now realize that the PR I have proposed here isn't the correct fix.

(@henningkayser you may want to take a look at the conversation being had here since you have some background context with #1994)

[henningkayser]

I haven't fully dug into this yet, but isn't the real problem that we are building humble images with our main job as well? I will look into this tomorrow to fully understand what's going on here.

[adlarkin]

isn't the real problem that we are building humble images with our main job as well?

I don't think that should be the problem - when humble images are being built with the main job, the corresponding Dockerfile's ROS_DISTRO arg is overwritten to humble, and the matching humble base image is used (for example, https://github.com/ros-planning/moveit2/blob/main/.docker/ci/Dockerfile#L4-L5). But, since the ci_testing Dockerfile uses osrf/ros2:testing as its base image, the overwritten ROS_DISTRO arg from the github workflow has no effect on the base image since the osrf/ros2:testing image is only configured for rolling.

Perhaps I am wrong, but that's how I understand things at the moment. Thanks for taking the time to look into it - let me know if you have any questions, or if there's anything else I can do to help!

[vatanaksoytezer]

@adlarkin very good catch! I think we need to start creating our own testing images again (maybe even from scratch), since osrf ones hardcoded and unavailable for humble. I would say nightly images might be a good start. Just copying https://github.com/osrf/docker_images/blob/master/ros2/testing/testing/Dockerfile and changing it to humble should be good enough.

[henningkayser]

I just had a brief look at the testing image again. It's just setting an env variable for ROS_DISTRO, but it doesn't install anything specific to rolling as far as I see. We are installing all the ROS basics in our own image. Can't we just switch the env variable first thing in our docker image?

[henningkayser]

Ok, so the issue really seems to be that testing is setting this ROS_DISTRO env variable only for running rosdep update. Nothing related to rolling is installed in the first place. The env variable takes precedence over our ROS_DISTRO arg here https://github.com/ros-planning/moveit2/blob/main/.docker/ci-testing/Dockerfile#L4 so our testing install is using $ROS_DISTRO=rolling. The best fix would be to override the env variable somehow using our arg, but I'm not sure if there is a straight-forward way to do that. Here is an issue that is very much related moby/moby#34494. I haven't found a working fix for this locally yet.
Also, if we fix our testing build, source should also be fixed because it's using testing as base.

[mergify]

This pull request is in conflict. Could you fix it @adlarkin?

[adlarkin]

Thanks @henningkayser; I found the following statement in https://docs.docker.com/engine/reference/builder/#using-arg-variables which appears to describe the issue we're facing here:

Environment variables defined using the ENV instruction always override an ARG instruction of the same name.

Combining the documentation stated above along with what's stated in https://docs.docker.com/engine/reference/builder/#understand-how-arg-and-from-interact, I came up with a potential solution for the ci-testing image (see e38be1d). I verified the changes proposed here work locally for me via the following steps:

1. build Docker images for both rolling and humble. This command needs to be ran from the root of the moveit2 repo:

docker build -t rolling_test --build-arg OUR_ROS_DISTRO=rolling --no-cache -f .docker/ci-testing/Dockerfile .

docker build -t humble_test --build-arg OUR_ROS_DISTRO=humble --no-cache -f .docker/ci-testing/Dockerfile .

2. start a container for each docker image, and check what ROS_DISTRO is set along with what ROS binary packages are installed. I verified that the rolling image has ROS_DISTRO=rolling and rolling binary packages installed, while the humble image has ROS_DISTRO=humble and humble binary packages installed.

I also updated this PR to target main (it was originally targeting the humble branch) and updated the description of this PR to reflect the root cause of the issue we discovered in the PR discussion.

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: -0.05 ⚠️

Comparison is base (2ebba5c) 50.89% compared to head (e38be1d) 50.84%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2035      +/-   ##
==========================================
- Coverage   50.89%   50.84%   -0.05%     
==========================================
  Files         391      391              
  Lines       32142    32142              
==========================================
- Hits        16357    16339      -18     
- Misses      15785    15803      +18     

see 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[adlarkin]

Hey @vatanaksoytezer, thanks for approving this PR. I am trying to understand when the updated docker images will actually be built and ready to be pulled from by downstream users - looking at https://github.com/ros-planning/moveit2/blob/main/.github/workflows/docker.yaml#L8-L10, does this mean that new docker builds will be triggered once this PR is merged into main? The MoveIt 2 tutorial humble docker image depends on this PR (moveit/moveit2_tutorials#646), so I would like to get this image built soon so that the tutorial image can updated as well.

[vatanaksoytezer]

does this mean that new docker builds will be triggered once this PR is merged into main?

That is correct. I will merge now.