Drake: CI Test Suite Needs ROS Coverage Triggered by PR

[liangfok]

As we begin to add ROS support in Drake, we need to update the CI test suite to support ROS as well. I recommend adding a new column to the CI tests called "linux_ros" and only activate the gcc row of that column. The test environment should have ROS Indigo installed, ROS' environment variables configured by running source /opt/ros/indigo/setup.bash, and be running roscore prior to executing unit test ros_test.

[liangfok]

Related: https://github.com/gerkey/ros1_external_use/issues/12#issuecomment-211503961

Maybe instead of using ctest we should use rostest (http://wiki.ros.org/rostest) for the Drake unit tests that use ROS.

[jamiesnape]

We have to use ctest for all tests.

[jamiesnape]

We should be able to make a version of add_rostest that includes a ctest call easily enough.

Edit: Maybe it already does:

https://github.com/ros/catkin/blob/indigo-devel/cmake/test/tests.cmake

For future reference, add_rostest definition is here:

https://github.com/ros/ros_comm/blob/indigo-devel/tools/rostest/cmake/rostest-extras.cmake.em

[liangfok]

How can we get ROS Indigo installed and configured on the gcc + linux build server?

[jamiesnape]

Create a ticket.

[jwnimmer-tri]

@liangfok Start with the developer docs, saying how to install it on the platforms you intent to support it on. Then a ticket can get CI to replicate that in the cloud.

[liangfok]

By "ticket" do you mean a Github.com issue? If so, do I create the ticket in this repository or another one like https://github.com/RobotLocomotion/drake-ci?

Thanks!

[jamiesnape]

Yes and create the ticket in this repo.

[130s]

Just FYI, for ROS specific CI configurations, I'm maintaining a package that provides configs (currently Travis CI only) that can be commonly used so that package maintainers don't need to be bothered for maintaining CI configs. It's called industrial_ci.
Despite how the package name sounds, it can be and has been used in non industrial setting ROS packages. Please refer to README for more detail, and feel free to ask questions at its issue tracker.

[david-german-tri]

Reassigned to @liangfok - the information above should be sufficient to create ROS tests (and #2369 will cover the ROS CI startup).

[liangfok]

@jhoare figured out how to compile Drake as a ROS package within a Catkin workspace and run rostest executables provided by ROS packages within this workspace. This, I believe, is far better than faking a Catkin workspace.

The big question is: Can we add Drake CI coverage for a real Catkin workspace that includes Drake as one of its ROS packages?

I believe @jamiesnape may be in the best position to answer the above question and will thus assign this issue to him.

[jhoare]

For now, I am going to set this up to build drake + drake_ros in a catkin workspace in CI. This will have drake build with its default settings.

For the longer term, do we need to support different build profiles for drake in the ROS environment? (e.g. w/ and w/o matlab?)

[liangfok]

For the longer term, I believe having just the following two build profiles will be enough:

1. Drake + ROS
2. Drake + ROS + MATLAB

I'd like to maintain a build profile that includes MATLAB to ensure that the "full version" of Drake continues to work.

[jhoare]

@liangfok Does ROS has to be run on every PR? Do you want both gcc & clang builds? How about Release & Debug builds?

It is looking like having ROS builds run on every PR will necessitate a lot of Jenkins changes/updates.

[liangfok]

For now, until there are more users of Drake+ROS, not running ROS on every PR will be OK. I will follow up with any breakages of Drake+ROS code that slips through CI because of this decision.

Could we configure Jenkins to only perform the ROS CI tests on PRs labeled platform:ROS?

I think only gcc / Release is fine.

[jhoare]

Could we configure Jenkins to only perform the ROS CI tests on PRs labeled platform:ROS?

I think this may be more difficult than just having it run on every test. However, we could manually trigger the ROS test via comments, as was done in https://github.com/RobotLocomotion/drake/pull/2935#issuecomment-235252445 .

[liangfok]

OK. Given the difficulty of configuring Jenkins to run Drake+ROS CI on every PR, let's not run Drake+ROS on every PR, even for those labeled platform:ROS.

TODO: #2966

[jamiesnape]

If you can somehow move $ROOT/package.xml to $ROOT/ros/drake/package.xml and then relocate everything in $ROOT/ros to $ROOT/ros/drake or below, the CI issues would go away. Is that feasible?

[jamiesnape]

The issue is that there is shared configuration between builds that necessities checking the repository into the root of the workspace, and that is incompatible with the ROS builds.

[jamiesnape]

Looking at the repo, I guess it is probably not possible without a symlink.

[jamiesnape]

Putting $ROOT/package.xml in $ROOT/drake/package.xml and then everything else in $ROOT/drake/ros would work from a CI perspective too.

[liangfok]

OK I'm looking into this now.

I'm a little concerned about whether Drake will even compile using catkin if I move $ROOT/package.xml to $ROOT/drake/package.xml since then I believe none of the externals that are normally in Drake's super-build level will be present.

Regardless, trying it now...

[jamiesnape]

I would wait for the moment. @jhoare and I have been talking about this offline.

[liangfok]

OK I will wait. Thanks!

[jhoare]

I'm a little concerned about whether Drake will even compile using catkin if I move $ROOT/package.xml to $ROOT/drake/package.xml since then I believe none of the externals that are normally in Drake's super-build level will be present.

Right, I don't think it would work. However, what I suggested was we could also make the externals package with its own package.xml in it, which then drake depends on. This way catkin will build the drake-externals packge, the drake package, and then the drake_ros_integration stuff.

I believe that in order to do this we need to move almost all of the cmake logic from the root CMakeLists.txt into the externals folder, and make the root CMakeLists.txt be little more than a couple of calls to add_subdirectory.

That said I think these changes are just needed to get the ROS builds working in the experimental matrix (e.g. on pull requests) and we can get them running post-merge fairly soon, although @jamiesnape can correct me if I'm wrong.

[jamiesnape]

Nightlies should start tonight. Continuous early next week.

[liangfok]

I tested PR #2980 which appeared to trigger a pre-merge linux-gcc-experimental-ros test that's separate from the Jenkins matrix but nonetheless worked in terms of detecting a bug pre-merge. How did this test get added? I manually scheduled the test using the magic @drake-jenkins-bot linux-gcc-experimental-ros please comment. Could we simply add this "out-of-matrix" test to every PR?

[jamiesnape]

Unfortunately not. You will have to trigger it manually using a comment (for now).

[liangfok]

I would be supportive of moving the externals, which are currently in drake-distro to be in drake-distro/externals. I'm going to ask around to see if anyone has any problem with doing this.

[liangfok]

@jwnimmer-tri informs me that Drake should build (but in an slightly trimmed down state) even if none of the externals in the super-build are present. I shall thus test the idea of moving drake-distro/package.xml into drake-distro/drake/package.xml to see if it works.

[jwnimmer-tri]

What I don't understand yet is why having a drake-externals catkin package (package.xml) requires relocating lines of code from CMakeLists.txt. Ah... is it because <buildtool_depend>cmake</buildtool_depend> implies that the CMakeLists.txt must be in the same directory as the package.xml?

[jwnimmer-tri]

The issue is that there is shared configuration between builds that necessities checking the repository into the root of the workspace, and that is incompatible with the ROS builds.

I don't know what the quoted text above means, and would appreciate a more detailed explanation.

[jwnimmer-tri]

... even if none of the externals in the super-build are present ...

But there _is_ a minimally required set, which is Eigen and googletest.

[jamiesnape]

I don't know what the quoted text above means, and would appreciate a more detailed explanation.

There is only one checkout location for a matrix job. The individual entries in the matrix are not actually independent jobs, merely the setting of environment variables that parameterize a single job. ROS needs a different checkout location to the other matrix builds, which is not possible.

[jamiesnape]

... is it because <buildtool_depend>cmake</buildtool_depend> implies that the CMakeLists.txt must be in the same directory as the package.xml?

Yes.

[jhoare]

What I don't understand yet is why having a drake-externals catkin package (package.xml) requires relocating lines of code from CMakeLists.txt. Ah... is it because cmake implies that the CMakeLists.txt must be in the same directory as the package.xml?

Yes, catkin sees the package.xml and uses the CMakeLists.txt from that directory. A catkin package is effectively its own self-contained CMake project. So having CMake logic at a higher directory than the package.xml file is no good, because when catkin is used, that CMakeLists.txt file will not be looked at at all.

[jwnimmer-tri]

Confirming my understanding:

Part of the challenge here is that for example drake-distro/ros/drake_cars_examples is its own catkin package (as are its two siblings), so in that model we need drake-superbuild and drake as separate packages. Is that correct?

If we just had one single massive catkin package for all of drake-distro (including the externals, drake, drake_cars_examples, drake_ros_integration, drake_ros_systems) all under the one superbuild CMakeLists.txt, is that a viable option? What challenges would it have?

[jamiesnape]

Having the package.xml in the root is one of the issues.

[liangfok]

Part of the challenge here is that for example drake-distro/ros/drake_cars_examples is its own catkin package (as are its two siblings), so in that model we need drake-superbuild and drake as separate packages. Is that correct?

The current placement of drake_cars_examples and its two siblings _within_ drake-distro/ros is an artifact of the fact that there is a package.xml in drake-distro. If we can move drake-distro/package.xml into drake-distro/externals/package.xml, we can actually move drake_cars_examples and its two siblings to be in drake-distro and also eliminate the need for a symbolic link from drake_catkin_workspace/src/drake_ros_integration to drake_catkin_workspace/src/drake/ros.

It would be a win-win!

[jhoare]

we can actually move drake_cars_examples and its two siblings to be in drake-distro

You could, but you also wouldn't need to. You could clone drake-distro right into catkin_ws/src and it would work the way it should. We would be able to remove the weird symbolic link step that exists now.

[liangfok]

Good point. I forgot that drake-distro/ros does not have a package.xml file and thus the package.xml files within the sub-directories of drake-distro/ros will become visible to catkin.

[jhoare]

Downgraded priority of ticket since we now have some ROS CI abilities, but still not triggered on Pull Requests.

[jwnimmer-tri]

In #build we have talked about moving away from the Matrix plug-in, and instead just launching off ~6 required builds instead. Assuming we're doing that (and I think we are), could we just add an Experimental ROS build to the required set and launch it pre-merge, without trying to get the ROS vs git vs Matrix yak shaved?

[jwnimmer-tri]

(I ask because lacking ROS coverage pre-merge is a huge pain for me, because I forget to keep enabling it on every single PR ever.)

[jamiesnape]

Yes.

[jwnimmer-tri]

Could we add this new Required build now through a simple tickbox or something, without having to dematrixify the PR builds yet?

[david-german-tri]

Could we add this new Required build now through a simple tickbox or something, without having to dematrixify the PR builds yet?

:+1: - build/CI subteam just agreed to do this in weekly meeting.

[david-german-tri]

This appears to be done. Opened #3931 for dematrixification. Closing this issue.

[david-german-tri]

https://drake-jenkins.csail.mit.edu/view/Experimental/job/linux-gcc-experimental-ros/