build with openCV4

[doisyg]

I needed image_view on foxy/ubuntu focal, so I needed image_pipeline to build with OpenCV4.
As I understand it the version of openCV for foxy is 4, with no back support of earlier versions. See for instance the vision_opencv packages https://github.com/ros-perception/vision_opencv/tree/ros2

I have only tested image_view which works fine.

[doisyg]

CI is failing but I guess it is because it is testing for dashing.
Shouldn't the ros2 branch target the next release (ie foxy) as in most package ?
"ros2: points to the next unreleased ROS 2 turtle, currently Foxy. "

[SteveMacenski]

@doisyg can you update CI to run Foxy / ROS2 master?

[doisyg]

Sure but I am not familiar with CircleCI, should changing eloquent for foxy in image_pipeline/.circleci/config.yml be enough? I seems that it uses the turtle-ros-base docker, but is there a foxy-ros-base out there already ?

[SteveMacenski]

Well, right now its Dashing 😆 https://github.com/ros-perception/image_pipeline/blob/ros2/.circleci/config.yml#L5 but more or less. Right now your PR fails because openCV4 isn't in dashing. There's also a ROS2 nightly dockerfile that could be used as well instead.

[doisyg]

Indeed i just found out about osrf/ros2:nightly, but image_pipeline depends on image_common for image_transport and image_geometry and it is not released yet ... though there must be a way

[SteveMacenski]

Typically how that could be dealt with is via building on nightly, but in the circle CI file, have it build this repo with those others repos using a dependencies.repos file (e.x. https://github.com/ros-planning/navigation2/blob/master/tools/ros2_dependencies.repos) for temporary use. Or we could just wait until a docker image is available.

[JWhitleyWork]

@doisyg / @SteveMacenski I've created some Foxy Docker images which you can use. Just use the image jwhitleywork/ros:foxy-ros-base-focal. However, this isn't just a CI issue - There were significant changes (especially in linting) between Dashing, Eloquent, and Foxy. Because of this, we will need to maintain different branches for these three.

@SteveMacenski I feel like we've discussed this before on this repo. Thoughts on targeting foxy with the ros2 branch while branching dashing and eloquent at the current state?

[SteveMacenski]

Yes, ros2 is targeting ros2 master, which is currently foxy. Before this PR goes in, we should branch the current state of ros2 as dashing -devel to cover dashing and eloquent (I don't think anything should be an issue with those as long as there's no TF... which I don't think there is).

[doisyg]

Last commits for CI. Use jwhitleywork/ros:foxy-ros-base-focal and naively pull the image_common and vision_opencv packages in the same ws. Strangely I had to install with apt libgtk2.0-dev which was not picked up by rosdep.
Compilation is okay, it will need more work for the tests (and I have no time nor the proper expertise for it)

[SteveMacenski]

@doisyg I think that's because GTK3.0 is what's used now. Can you run ament_uncrustify --reformat <path> over the repo? That will fix the linter errors (or at least many of them).

@JWhitleyWork Should any of the patches from Noetic be made here for python3? I'm seeing some test failures.

[JWhitleyWork]

@SteveMacenski The GTK 3 change was only made on the Noetic branch. It should be applied here too.

[doisyg]

Switched to gtk3 in CMakeLists.txt and package.xml as in noetic and removed the almost empty ifdef. It seems gtk is not used at all in the code, should we remove it completely ? Or should we left it if it is needed to implement this in the future in Foxy:

image_pipeline/image_view/src/nodes/stereo_view.cpp

Lines 52 to 62 in f13c10f

	#ifdef HAVE_GTK
	#include <gtk/gtk.h>
	// Platform-specific workaround for #3026: image_view doesn't close when
	// closing image window. On platforms using GTK+ we connect this to the
	// window's "destroy" event so that image_view exits.
	static void destroy(GtkWidget *widget, gpointer data)
	{
	ros::shutdown();
	}
	#endif

Or maybe this workaround is not needed anymore on Foxy platforms

Also ran ament_uncrustify --reformat

[SteveMacenski]

There are still a lot of test failures in CI - It looks like to me * that all of those errors are from image_common or the other repos we pulled in to do CI. I think that can be resolved by adding --packages-select camera_calibration depth_image_proc image_proc image_pipeline image_publisher image_rotate image_view stereo_image_proc to the colcon test line. That way it skips running tests over our dependencies which appear to not be linted (and broken)

This would be a nice place for special metapackage depth rules; test all in metapackage recursively for depth 1

[doisyg]

There are still a lot of test failures in CI - It looks like to me * that all of those errors are from image_common or the other repos we pulled in to do CI. I think that can be resolved by adding --packages-select camera_calibration depth_image_proc image_proc image_pipeline image_publisher image_rotate image_view stereo_image_proc to the colcon test line. That way it skips running tests over our dependencies which appear to not be linted (and broken)

Indeed, looks like it passes the CI now

[JWhitleyWork]

@SteveMacenski dashing (dropped -devel consistent with newer branches on this repo) has been created from the current state of ros2. Somehow, we never released image_pipeline for Dashing - do you want me to go ahead and do that?

Regardless, this MR can be merged.

[SteveMacenski]

Sure, lets release it

[doisyg]

Would be nice to release it for Foxy too

[SteveMacenski]

@doisyg definitely for foxy, its only been in the last month or so we've had a full port. There's still cleanup to do, but I think its in a state that can be called "good enough for now". Still some functionality like reconfiguring parameters and only publishing when someone is subscribed that aren't implemented right now.

We still need at minimum vision_opencv and image_common to be released before we can in Foxy

[ivanpauno]

@SteveMacenski Is image_pipeline supporting old ros2 distros (Dashing and Eloquent)?

Ubuntu 18.04 only have debians for OpenCV 3, and installing OpenCV 4 on it is an extra effort.
I don't see a branch compatible with OpenCV 3.

[ivanpauno]

I don't see a branch compatible with OpenCV 3.

I'm sorry, I see there's a Dashing branch. Is that one compatible with Eloquent, or should one branch for it be created?

[SteveMacenski]

Dashing branch serves both dashing and eloquent

[ivanpauno]

Dashing branch serves both dashing and eloquent

Thanks!

[JWhitleyWork]

Sure, lets release it

Would you mind pushing the release? I'm busy as hell right now.

[SteveMacenski]

I'm pretty sure we already did.......

.... or was that Noetic? jeeze 2 different releases at once is really not a good habit to get into. OK, I'll figure it out and add to my mountain of releases I was planning on doing on Friday.

[SteveMacenski]

ros/rosdistro#25137

[SteveMacenski]

Memory serves that failures due to

dpkg-buildpackage: error: debian/rules build subprocess returned exit status 2
E: Building failed
Traceback (most recent call last):
  File "/tmp/ros_buildfarm/ros_buildfarm/binarydeb_job.py", line 138, in build_binarydeb
    subprocess.check_call(cmd, cwd=source_dir)
  File "/usr/lib/python3.8/subprocess.py", line 364, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['apt-src', 'build', 'ros-foxy-image-view']' returned non-zero exit status 1.
# END SUBSECTION

are internal build farm things, correct? Just making sure I shouldn't be doing anything

[JWhitleyWork]

@SteveMacenski Can you link to the build where you're seeing this?

[SteveMacenski]

You mean you're not getting emails about it? Maybe we should add you to the package.xmls 😉 http://build.ros2.org/job/Fbin_uF64__image_view__ubuntu_focal_amd64__binary/2/console

[JWhitleyWork]

@SteveMacenski Check these out from the build log:

23:01:43 CMake Error at /usr/share/cmake-3.16/Modules/FindPackageHandleStandardArgs.cmake:146 (message):
23:01:43   Could NOT find Boost (missing: Boost_INCLUDE_DIR)
23:01:43 Call Stack (most recent call first):
23:01:43   /usr/share/cmake-3.16/Modules/FindPackageHandleStandardArgs.cmake:393 (_FPHSA_FAILURE_MESSAGE)
23:01:43   /usr/share/cmake-3.16/Modules/FindBoost.cmake:2179 (find_package_handle_standard_args)
23:01:43   CMakeLists.txt:22 (find_package)

grep-ing through the code (in a one-horse open slay), it looks like there are still a few references to Boost in some of the files but none of them have a Boost dependency in package.xml.

[SteveMacenski]

how did that work for the last release....

.... that's right, we haven't done a release yet for ROS2. This is going to be a really long process that I can't take on this week. It'll have to wait for next week. I think image_view is the only one failing though, or at least the only one I'm getting emails about.

[SteveMacenski]

I just checked, its only image view failing of the builds that have processed (some bin builds havent triggered yet). So it might not be as bad as I thought. I'll handle next week.

[dirk-thomas]

The variable opencv_4_components doesn't seem to be set anywhere?