Fix display tests and enable when possible #186

Martin-Idel-SI · 2018-01-30T09:20:03Z

This is a followup for #175 . It aims to fix all display tests and run them when possible.

In order to not introduce visibility control in rviz_default_plugins, we need to build the library both static and shared. Apparently, includes are handled differently in that way, as the static building generates a lot of OGRE and pluginlib warnings on both Windows and Linux.
To run tests, we try to decide whether OpenGL exists via CMake.

Martin-Idel-SI · 2018-01-30T09:23:26Z

First try with CI:

Linux
Linux-aarch64
macOS
Windows

Martin-Idel-SI · 2018-01-30T12:04:59Z

So it seems that disabling the tests if no display is available on Linux works.
I would assume that the SEH errors on Windows indicate there is no display available or there is an OpenGL problem. The others could just be a different symptom. Can you give more information about the Windows Jenkins @wjwwood ?
On Mac, there seems to be a different problem, I'll try to reproduce it locally tomorrow and see how far this goes.

wjwwood

I have a few nitpicks, I know it's still a WIP, but I thought I'd give some early feedback.

wjwwood · 2018-01-30T19:40:14Z

rviz_default_plugins/src/rviz_default_plugins/displays/camera/camera_display.cpp

+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Wunused-parameter"
+# pragma GCC diagnostic ignored "-Wpedantic"
+#endif


Why is this needed?

We should only disable warnings like this when they're in external dependencies. This looks like an issue within pluginlib?

I totally agree, I just wanted to see whether I could get it to compile if I did this.

I get the following warning:

/include/pluginlib/class_list_macros.hpp:49:59: error: extra ‘;’ [-Werror=pedantic] CLASS_LOADER_REGISTER_CLASS(class_type, base_class_type);

If I read this correctly, it complains that the ";" is superfluous in this line:

https://github.com/ros/pluginlib/blob/b456beba88564b746694dde68696d8bc4edc78c9/pluginlib/include/pluginlib/class_list_macros.hpp#L49

It seems to me that compiling the pluginlib with -Werror=pedantic should catch that problem.

Looks like this comment got moved to a new line of code or something... Right now it appears to only apply to Ogre includes, which is fine.

Just to reiterate, I'd say it's fine to suppress -Wpedantic for pluginlib, but not unused variables. That should instead be fixed upstream. Ideally, we'd use pedantic upstream in pluginlib too.

wjwwood · 2018-01-30T19:42:43Z

rviz_default_plugins/src/rviz_default_plugins/displays/image/ros_image_texture.cpp

+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Wunused-parameter"
+# pragma GCC diagnostic ignored "-Wpedantic"
+#endif
 #include <OgreTextureManager.h> // NOLINT: cpplint cannot handle include order


nitpick: please add a second space between the code and comment

wjwwood · 2018-01-30T19:43:09Z

rviz_default_plugins/src/rviz_default_plugins/displays/pointcloud/point_cloud_display.cpp

@@ -31,9 +31,15 @@

 #include <memory>
 #include <utility>
-


nitpick: leave the space here

wjwwood · 2018-01-30T19:43:20Z

rviz_default_plugins/src/rviz_default_plugins/displays/pointcloud/point_cloud_display.cpp

-
+#ifndef _WIN32
+# pragma GCC diagnostic pop
+#endif


Put a space after this.

Martin-Idel-SI · 2018-01-31T13:50:20Z

Thanks for the early review! Here is a new CI:

Linux
Linux-aarch64
macOS
Windows

Looking into the output, display tests can apparently run on Mac!
What's left to do?

I guess the Apple failures are due to missing environment variables? So far, I couldn't reproduce locally, but I've never built isolated, so I might get to it tomorrow
There are a lot of different failures on Windows, but since all tests fail, I'd say the display is not present. However, it might be a problem of isolated builds vs non-isolated builds. I'll investigate that one tomorrow.

Martin-Idel-SI · 2018-02-01T15:33:53Z

The failures on Mac (and some on Windows) are due to environment problems. The last commit fixes the ones for Mac:

Linux
Linux-aarch64
macOS
Windows

For Windows, I haven't tried, yet - but at least we now know that we can run all tests on Mac!

wjwwood · 2018-02-01T19:19:57Z

Looks like they are now running, but failing on Windows.

I'd expect our macOS and Windows machines to work since they are all dedicated machines and not headless servers like our linux machines.

Martin-Idel-SI · 2018-02-02T10:16:04Z

Yes, I expected the tests run successfully on Windows, too. I'm trying to reproduce the failures locally but so far I've been unsuccessful. I'll work on that next week after fixing the issues in the release candidate.

Martin-Idel-SI · 2018-02-07T09:30:00Z

I did a rebase and had a closer look again.
I tried to reproduce the errors on our Windows machine in several ways, but couldn't

Just build rviz locally in its own repository (clean console, ros2 sourced)
Build rviz isolated in its own repository (clean console, ros2 sourced)
Build ros2 with this rviz branch locally using --build-tests --isolated (clean console, no source) and then ament.py test as on your CI farm.
All tests always ran. I just can't reproduce.

There are many different kinds of errors. I've seen most of them before in some version - they turned out to be mostly environment related.

Some observations I made:

For the tests in rviz_default_plugins, the failure with some big number is very probably due to dlls not being found
The tests for PointCloud2Display run, so it seems that the Qt dlls are found
We once had an issue when curl wasn't found, this might be the case
For rviz_rendering tests, it seems that ogre can be found, but doesn't work correctly. It can't be a problem of the display being locked (this is a problem with GUI tests), we tested that
Including AMENT_PREFIX_PATH and PATH doesn't really change anything in the errors produced (see the errors in point_cloud_renderable_test, where I added the paths and billboard_line_test, where I didn't), so it doesn't seem to be a problem with those parts of the environment

What I could try is to get CMake to print out all environment variables and hope that I can then tweak the environment locally to reproduce the test failures, but that would probably mean quite a few jobs on ros2 CI. Do you have any better suggestion?

Martin-Idel-SI · 2018-02-12T11:45:54Z

I can't reproduce the errors locally, whatever I do. That means it's really difficult to debug, so for now, I disabled the tests on Windows. They can be reenabled sending a CMake flag, so one could easily debug the problem when having access to the physical machines.

It remains to fix the pluginlib so that we don't need the the pragmas for that.

CI with display tests disabled on Windows:

Linux
Linux-aarch64
macOS
Windows

The failed tests don't have anything to do with this.

greimela-si · 2018-03-06T15:16:06Z

Rebased after changes on ros2 branch.

New CI:

Linux
Linux-aarch64
macOS
Windows

Martin-Idel-SI · 2018-03-12T10:23:19Z

rviz_common/CMakeLists.txt

@@ -280,6 +283,7 @@ ament_export_dependencies(
  tf2_geometry_msgs
  tf2_ros
 )
+ament_export_include_directories(include)


What I don't understand is why this suddenly becomes necessary. If I only build dynamically, the call to ament_export_interfaces(rviz_common) is enough (although the rviz_common_INCLUDE_DIRS are not properly populated), but once I try to build the object files, I can't find include files from rviz_rendering.
I can fix this here no problem, but I have the same issue with resource_retriever, where I can't add the call to ament_export_include_directories.
Sorry to bother you @wjwwood , but can you explain this phenomenon?

ament_export_interfaces(rviz_common) exports the cmake target rviz_common, so when using it in another package (as rviz_common::rviz_common), it brings with it the include dirs, link flags, etc...

In this mode of operation (target exporting) all settings are target specific, so things like rviz_common_INCLUDE_DIRS are not used.

If you want that directory to be included in the rviz_common target you need some target_include_directories magic, see:

https://github.com/bsinno/rviz/blob/6f27b4dfa42b1adc6c8692fd866600fb5282b32d/rviz_common/CMakeLists.txt#L236-L242

Which is already happening, so you should only need to link what ever you're compiling that needs those headers against rviz_common::rviz_common to get the right stuff.

Thank you for the explanation.
That's actually a problem, because if you compile OBJECT libraries like I do in rviz_default_plugins (to later compile to shared libraries for the real application and to a static library to link against tests), you can't specify link targets.

More specifically: In CMakeLists of rviz_default_plugins, I'm doing

add_library(rviz_default_plugins_object OBJECT ${rviz_default_plugins_headers_to_moc} ${rviz_default_plugins_source_files} )

If I don't have the ament_export_include_directories in rviz_common, I can't find include directories of resource_retriever specified via target_include_directories(rviz_default_plugins_object PUBLIC ${rviz_common_INCLUDE_DIRS})
what I'd have to do is say
target_link_libraries(rviz_default_plugins_object PUBLIC rviz_common::rviz_common)
or
ament_target_dependencies(rviz_default_plugins_object rviz_common).
However, neither is possible, because you mustn't link against OBJECT libraries.
That explains why I need a ament_export_include_directories. For rviz_common that's no problem, for resource_retriever I can't do this, but I asked whether this could be added (ros/resource_retriever#21).

Martin-Idel-SI · 2018-03-12T10:24:46Z

I'm trying to rebase on master, but I'm having trouble with the new resource_retriever dependency in rviz_default_plugins.

wjwwood · 2018-03-12T18:25:15Z

Also, with respect to the Windows issues, one problem might be that we run the jenkins jobs as the SYSTEM user. You can get a cmd prompt as this user by using the psexec tool, see: https://blogs.technet.microsoft.com/askds/2008/10/22/getting-a-cmd-prompt-as-system-in-windows-vista-and-windows-server-2008/

Martin-Idel-SI · 2018-03-13T16:06:06Z

Using the system user might of course change a few things. Maybe I can reproduce the failures with that.

Martin-Idel-SI · 2018-03-14T16:51:18Z

Let's see where we are at:

Linux
Linux-aarch64
macOS
Windows

Still missing: I haven't enabled the tests on Windows, yet. Building as system user fails atm due to other problems, so I couldn't reproduce the errors yet.

If we want to fix that later, we should be "good to go" (if CI agrees).

Martin-Idel-SI · 2018-03-15T13:13:39Z

I had to disable a few more tests and introduce some Mac pragmas:

Linux
Linux-aarch64
macOS
Windows

Martin-Idel-SI · 2018-03-29T14:02:30Z

CI after rebase:

Linux
Linux-aarch64
macOS
Windows

- initialize must have been called before the window exists - this can only happen when rendering

- Offer a CMake variable to enable them via commandline

- Warnings only concern Ogre

greimela-si · 2018-04-05T15:09:13Z

I fixed all the merge conflicts and included your review comments.

New CI run:

Linux
Linux-aarch64
macOS
Windows

wjwwood

lgtm

dirk-thomas · 2018-04-11T23:53:55Z

rviz_default_plugins/CMakeLists.txt

+  ${Qt5Widgets_INCLUDE_DIRS}
+  ${resource_retriever_INCLUDE_DIRS}
+  ${TinyXML_INCLUDE_DIRS}
+  ${urdf_INCLUDE_DIRS}


This patch broke the build again when urdf is installed on the system. See #243 for a temp. workaround.

wjwwood reviewed Jan 30, 2018

View reviewed changes

wjwwood added enhancement New feature or request in progress Actively being worked on (Kanban column) labels Jan 30, 2018

Martin-Idel-SI force-pushed the feature/enable_display_tests branch from 812c3f1 to 5f2d785 Compare January 31, 2018 10:38

Martin-Idel-SI force-pushed the feature/enable_display_tests branch from 81e2a08 to 5f2d785 Compare January 31, 2018 15:46

Martin-Idel-SI force-pushed the feature/enable_display_tests branch from 8a62673 to 290c305 Compare February 7, 2018 09:29

Martin-Idel-SI force-pushed the feature/enable_display_tests branch 2 times, most recently from 440c6cc to 193c43e Compare February 12, 2018 11:43

Martin-Idel-SI changed the title ~~[WIP] Fix display tests and enable when possible~~ Fix display tests and enable when possible Feb 12, 2018

greimela-si force-pushed the feature/enable_display_tests branch from 193c43e to 6f27b4d Compare March 6, 2018 15:10

Martin-Idel-SI commented Mar 12, 2018

View reviewed changes

Martin-Idel-SI changed the title ~~Fix display tests and enable when possible~~ [WIP] Fix display tests and enable when possible Mar 13, 2018

Martin-Idel-SI force-pushed the feature/enable_display_tests branch from 6f27b4d to 365edd2 Compare March 14, 2018 16:47

Martin-Idel-SI force-pushed the feature/enable_display_tests branch from 6d2b77c to 5631713 Compare March 26, 2018 11:23

Martin-Idel-SI changed the title ~~[WIP] Fix display tests and enable when possible~~ Fix display tests and enable when possible Mar 29, 2018

Martin-Idel-SI and others added 19 commits April 5, 2018 10:57

Add check to fix display tests

2692a01

- initialize must have been called before the window exists - this can only happen when rendering

Disable display tests per default on Win32

d361195

- Offer a CMake variable to enable them via commandline

Fix rviz_rendering includes

ad156b2

Fix pragmas for selection_manager

5cac7ce

Fix pragmas in robot_model_display

3ad306c

Fix include problems for urdf and tinyxml

da3ee19

Introduce pragmas for pose display

8f3f224

Remove pragmas from pluginlib imports

956169b

Fix AMENT_PREFIX_PATH for robot_model_test

323621f

Disable rviz_rendering_tests on Windows

7b56b77

Fix clang warnings by introducing pragmas

f5171fc

- Warnings only concern Ogre

Add missing pragmas for marker and pose array display

17b4b84

Fix MarkerDisplay pragma for Apple

85321b8

Add missing pragmas and fix cmake

05f40a5

Fix tests

aecdf2b

Add laser_geometry include dir

f414af5

Add nav_msgs include dirs

5c403ea

Fix PATH in robot_test

8188c09

Remove obsolete pragmas

c958281

greimela-si force-pushed the feature/enable_display_tests branch from 87cc36b to c958281 Compare April 5, 2018 15:05

wjwwood approved these changes Apr 10, 2018

View reviewed changes

wjwwood merged commit 80ab353 into ros2:ros2 Apr 10, 2018

wjwwood removed the in progress Actively being worked on (Kanban column) label Apr 10, 2018

greimela-si deleted the feature/enable_display_tests branch April 11, 2018 06:58

greimela-si mentioned this pull request Apr 11, 2018

Disable Ogre deprecation warning on Windows #242

Merged

dirk-thomas mentioned this pull request Apr 11, 2018

hack to fix urdf include dirs again #243

Merged

dirk-thomas reviewed Apr 11, 2018

View reviewed changes

greimela-si mentioned this pull request Apr 12, 2018

Assert triggering in nightly MacOS debug builds #245

Closed

nuclearsandwich mentioned this pull request Apr 16, 2018

Debug mode windows unable to load dll. #247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix display tests and enable when possible #186

Fix display tests and enable when possible #186

Martin-Idel-SI commented Jan 30, 2018

Martin-Idel-SI commented Jan 30, 2018

Martin-Idel-SI commented Jan 30, 2018 •

edited

Loading

wjwwood left a comment

wjwwood Jan 30, 2018

wjwwood Jan 30, 2018

Martin-Idel-SI Jan 31, 2018 •

edited

Loading

wjwwood Apr 2, 2018

wjwwood Jan 30, 2018

wjwwood Jan 30, 2018

wjwwood Jan 30, 2018

Martin-Idel-SI commented Jan 31, 2018 •

edited

Loading

Martin-Idel-SI commented Feb 1, 2018

wjwwood commented Feb 1, 2018

Martin-Idel-SI commented Feb 2, 2018

Martin-Idel-SI commented Feb 7, 2018 •

edited

Loading

Martin-Idel-SI commented Feb 12, 2018 •

edited

Loading

greimela-si commented Mar 6, 2018

Martin-Idel-SI Mar 12, 2018

wjwwood Mar 12, 2018

Martin-Idel-SI Mar 13, 2018 •

edited

Loading

Martin-Idel-SI commented Mar 12, 2018 •

edited

Loading

wjwwood commented Mar 12, 2018

Martin-Idel-SI commented Mar 13, 2018

Martin-Idel-SI commented Mar 14, 2018 •

edited

Loading

Martin-Idel-SI commented Mar 15, 2018

Martin-Idel-SI commented Mar 29, 2018

greimela-si commented Apr 5, 2018

wjwwood left a comment

dirk-thomas Apr 11, 2018

Fix display tests and enable when possible #186

Fix display tests and enable when possible #186

Conversation

Martin-Idel-SI commented Jan 30, 2018

Martin-Idel-SI commented Jan 30, 2018

Martin-Idel-SI commented Jan 30, 2018 • edited Loading

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martin-Idel-SI Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martin-Idel-SI commented Jan 31, 2018 • edited Loading

Martin-Idel-SI commented Feb 1, 2018

wjwwood commented Feb 1, 2018

Martin-Idel-SI commented Feb 2, 2018

Martin-Idel-SI commented Feb 7, 2018 • edited Loading

Martin-Idel-SI commented Feb 12, 2018 • edited Loading

greimela-si commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martin-Idel-SI Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Martin-Idel-SI commented Mar 12, 2018 • edited Loading

wjwwood commented Mar 12, 2018

Martin-Idel-SI commented Mar 13, 2018

Martin-Idel-SI commented Mar 14, 2018 • edited Loading

Martin-Idel-SI commented Mar 15, 2018

Martin-Idel-SI commented Mar 29, 2018

greimela-si commented Apr 5, 2018

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martin-Idel-SI commented Jan 30, 2018 •

edited

Loading

Martin-Idel-SI Jan 31, 2018 •

edited

Loading

Martin-Idel-SI commented Jan 31, 2018 •

edited

Loading

Martin-Idel-SI commented Feb 7, 2018 •

edited

Loading

Martin-Idel-SI commented Feb 12, 2018 •

edited

Loading

Martin-Idel-SI Mar 13, 2018 •

edited

Loading

Martin-Idel-SI commented Mar 12, 2018 •

edited

Loading

Martin-Idel-SI commented Mar 14, 2018 •

edited

Loading