Adding a docker file to help test for missing package.xml dependencies · Pull Request #606 · ros-navigation/navigation2

ghost · 2019-03-13T19:47:57Z

Description of contribution in a few bullet points

Added a docker file that builds ROS2, the navstack dependencies and navigation 2 in one big workspace without sourcing any setup.bash files. This catches any missing ROS2 dependencies in our package.xml files.
Also added a Pre-release checklist doc.

Future work that may be required in bullet points

There is a lot of duplication among our docker files that needs to be fixed. I don't know how to do that yet.
This docker file starts from crystal-ros-core to get the correct apt sources file and the basic dependencies that ROS2 needs, however crystal-ros-core installs a bunch of ros2 packages we don't need since we are building everything from source anyhow.
The crystal-ros-core base image probably needs to be an argument, so we can change it based on which branch we are targeting.

codecov · 2019-03-13T20:16:35Z

Codecov Report

Merging #606 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #606      +/-   ##
==========================================
- Coverage   21.12%   21.08%   -0.05%     
==========================================
  Files         184      184              
  Lines        9448     9448              
  Branches     2317     2317              
==========================================
- Hits         1996     1992       -4     
  Misses       6311     6311              
- Partials     1141     1145       +4

Impacted Files	Coverage Δ
src/navigation2/nav2_navfn_planner/src/navfn.cpp	`58.19% <0%> (-0.85%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a892011...31dc6aa. Read the comment docs.

ruffsl · 2019-03-13T21:31:53Z

Added a docker file that builds ROS2, the navstack dependencies and navigation 2 in one big workspace without sourcing any setup.bash files. This catches any missing ROS2 dependencies in our package.xml files.

I'm not sure I understand. This seem to be using rosdep to install all dependencies declared in workspace packages, how does using one workspace instead of a layered workspace for upstream packaged dependencies change things? I think we would need to call rosdep for only the navigation packages, and the upstream ros2 packages that and dependent, not necessary for all packages in the ros2 master .repo.

This docker file starts from crystal-ros-core to get the correct apt sources file and the basic dependencies that ROS2 needs, however crystal-ros-core installs a bunch of ros2 packages we don't need since we are building everything from source anyhow.

Is this big issue? I could probably add a new tag like osrf/ros2:devel that only configures that sources.list and common build tools. But I think ros2 is missing a ros install generator like ros1 had that could help resolve your issue at hand. Also, perhaps this would build faster if we limited the build by using --pacakges-up-to <list of navigation2 packages>

There is a lot of duplication among our docker files that needs to be fixed. I don't know how to do that yet.

I think introducing another Dockerfile would exacerbates this. I've recommend templating our Dockerfile pipeline using docker build arguments to generalize against any release, specially for the use case in our CI and target release branch PRs. I'm little blocked however in doing this by upstream issues however:

https://discuss.circleci.com/t/conditioning-docker-image-via-base-branch-name-from-pr/28426/6

ghost · 2019-03-13T23:01:29Z

This seem to be using rosdep to install all dependencies declared in workspace packages, how does using one workspace instead of a layered workspace for upstream packaged dependencies change things?

Let me make a simple example to explain what I believe happens. Let's say ROS2 consists of 2 packages: alpha and beta and Nav2 consists of the package gamma.

Our typical build structure looks like this:

We normally build ros2_ws and then source ros2_ws/install/setup.bash before building the nav2_ws. When we source that file, we get every package in ros2_ws added to the environment. eg. CMAKE_PREFIX_PATH=/nav2_build/ros2_ws/install/alpha:/nav2_build/ros2_ws/install/beta
All other build related environment variables are also set in a similar way.

Let's say gamma depends on alpha and beta but we forgot to include beta in gamma's package.xml. gamma frequently still builds correctly because CMake finds all relevant dependencies in the search path.

In this new docker file, it's the equivalent of building at the nav2_build level.

In this case, we never source a setup.bash before running colcon, so when it tries to build gamma, the build fails because CMake can't find the beta dependencies.

I believe colcon creates a temporary environment for each package it builds by taking the current environment, and then adding on the paths to any dependencies listed in package.xml. In the second case above, we fail because we start with a clean environment and colcon only adds in the alpha dependency.

In the first case, we succeed because alpha and beta are already in the environment, so it doesn't matter what colcon adds or doesn't add.

This seem to be using rosdep to install all dependencies declared in workspace packages

There are two classes of dependencies here: ROS2 dependencies and third party dependencies. The one workspace build presented here is only meant to catch missing ROS2 dependencies. We frequently miss the ROS2 dependencies in the package.xml files because they were already installed as part of the ros2_ws build. Then we fail when OSRF does their nightly build because they are building each package with an independent environment.

Is this big issue? I could probably add a new tag like osrf/ros2:devel

It's not a big problem. We are just pulling more into the docker image than really is needed.

My real concern is that some pre-installed packages will leak into the environment causing the build to succeed when it should really fail. I don't think this is the case right now, but could happen because I'm not using the docker the way it is really intended

Also, perhaps this would build faster if we limited the build by using --pacakges-up-to

Yes. I believe you're right. Making that change would require human time, whereas just building everything only takes computer time, so I didn't bother. However, if we incorporate this extra type of build into CI, we should do that.

I'm not sure if this type of build should go in CI, of if it should just stay a manual verification step done prior to release.

Does this explain what I'm trying to do here?

ruffsl · 2019-03-19T03:28:26Z

Does this explain what I'm trying to do here?

Thanks for the thorough explanation! I think understand now: the checking for missing ros2 depnacies.

I'm not sure if this type of build should go in CI, of if it should just stay a manual verification step done prior to release.

I think we could either have it a regular cron job for master/release branches or we could have it be a final check in PRs, e.g. before merging, user merging PR first manual approves the branch of the workflow for the PR that tests this, so that not every commit triggers this length build.

https://circleci.com/docs/2.0/workflows/#holding-a-workflow-for-a-manual-approval

mkhansenbot

Either I don't understand the documentation or the commands for build and test are incorrect. I would think there's supposed to be one to build nav2:master and another to build nav2:crystal.

mkhansenbot · 2019-05-14T19:12:48Z

doc/process/PreReleaseChecklist.md

+There is a docker file to do that, so run
+
+```bash
+sudo docker build -t nav2:crystal_full  --build-arg http_proxy=http://myproxy.example.com:80  --build-arg https_proxy=http://myproxy.example.com:80 --build-arg ROS2_BRANCH=crystal -f Dockerfile.full_ros_build ./


Did you get this command and the other one swapped? This one says ROS2_BRANCH=crystal, which I think should be in the next example?

Both dockerfiles need a ROS2_BRANCH argument now. It represents the upstream branch we are targeting or verifying against.

mkhansenbot · 2019-05-14T19:14:16Z

doc/process/PreReleaseChecklist.md

+For newer releases, run
+
+```bash
+sudo docker run nav2:crystal src/navigation2/tools/run_test_suite.bash


should this be nav2:master?

It's just a tag for the docker build. We could leave it out entirely. I'll change the tags to be branch agnostic. I'll make them nav2:full_build and nav2:rosdep_build

mkhansenbot

Sorry I don't know what state this is in but I just reviewed it and found a couple minor things. I think the FROM ros:$BRANCH should be FROM ros:$ROS2_BRANCH shouldn't it?

mkhansenbot · 2019-06-21T00:55:41Z

Dockerfile.full_ros_build

+ARG CMAKE_BUILD_TYPE=Debug
+RUN colcon build \
+      --symlink-install \
+      # --packages-up-to nav2_bringup \


Is this commented out --packages-up-to option needed?

It's a possible performance optimization. I'd like to use it, but I don't know for sure if it is safe (as in, we'll truly catch all problems.) If we add a package that doesn't get referenced in some way from nav2_bringup, then we'd miss any dependency problems with that package.

I left the comment in so I didn't need to look up how to do this again, if we decided we wanted to enable the optimization, however, I'll delete it for now.

mkhansenbot · 2019-06-21T00:58:36Z

Dockerfile.release_branch

@@ -7,17 +7,9 @@
 # docker build -t nav2:crystal --build-arg BUILD_SYSTEM_TESTS


probably should update the example command line from 'Dockerfile.crystal' to 'Dockerfile.release_branch'

mkhansenbot · 2019-06-21T00:59:35Z

Dockerfile.release_branch

-      python3-vcstool \
-      wget \
-    && rm -rf /var/lib/apt/lists/*
+FROM ros:$BRANCH


shouldn't this be ros:$ROS2_BRANCH

Yes. Good catch. I guess I didn't test enough after I consolidated the two variables.

This catches missing dependencies in the package.xml files.

ghost mentioned this pull request Mar 18, 2019

CI should build the navstack in the same workspace as ROS2 and dependencies #496

Closed

ruffsl mentioned this pull request Mar 19, 2019

Adding Template for Ros2 nightly osrf/docker_templates#49

Closed

ghost requested review from bpwilcox, mhpanah and mkhansenbot May 7, 2019 17:21

ghost added this to the D Turtle Release milestone May 13, 2019

mkhansenbot reviewed May 14, 2019

View reviewed changes

ghost mentioned this pull request Jun 10, 2019

Update docker files to work with Dashing branches #824

Closed

ghost added the 1 - High High Priority label Jun 10, 2019

ghost self-assigned this Jun 10, 2019

ghost changed the title ~~Adding a docker file to help test for missing package.xml dependencies~~ [WIP] Adding a docker file to help test for missing package.xml dependencies Jun 11, 2019

ghost changed the title ~~[WIP] Adding a docker file to help test for missing package.xml dependencies~~ Adding a docker file to help test for missing package.xml dependencies Jun 14, 2019

ghost requested a review from mkhansenbot June 17, 2019 16:27

bpwilcox approved these changes Jun 18, 2019

View reviewed changes

mkhansenbot reviewed Jun 21, 2019

View reviewed changes

mjeronimo approved these changes Jun 25, 2019

View reviewed changes

Carl Delsey added 10 commits June 25, 2019 10:12

Adding a dockerfile that builds all of ROS2 in a single workspace.

442a3a6

This catches missing dependencies in the package.xml files.

Adding some comments on choice of docker starting image.

29f7b25

Adding pre-release checklist.

65d2bec

Removing unneeded build code

06104e3

Update to dashing branches.

3f6028b

Change name to better reflect reality

9a25729

Make full_build dockerfile configurable

80e0ac5

ARG before FROM doesn't propogate to lines after FROM

156bb7b

Updating docs and addressing review content

5fc802f

More doc fixes.

faa46e8

Fixing review comments

31dc6aa

ghost merged commit 5cfcb35 into ros-navigation:master Jun 25, 2019

This pull request was closed.

		@@ -7,17 +7,9 @@
		# docker build -t nav2:crystal --build-arg BUILD_SYSTEM_TESTS

Conversation

ghost commented Mar 13, 2019

Description of contribution in a few bullet points

Future work that may be required in bullet points

Uh oh!

codecov bot commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ruffsl commented Mar 13, 2019

Uh oh!

ghost commented Mar 13, 2019

Uh oh!

ruffsl commented Mar 19, 2019

Uh oh!

mkhansenbot 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!

mkhansenbot 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Mar 13, 2019 •

edited

Loading