Fix display tests and enable when possible#186
Conversation
|
First try with CI:
|
|
wjwwood
left a comment
There was a problem hiding this comment.
I have a few nitpicks, I know it's still a WIP, but I thought I'd give some early feedback.
There was a problem hiding this comment.
We should only disable warnings like this when they're in external dependencies. This looks like an issue within pluginlib?
There was a problem hiding this comment.
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:
It seems to me that compiling the pluginlib with -Werror=pedantic should catch that problem.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
nitpick: please add a second space between the code and comment
812c3f1 to
5f2d785
Compare
|
Thanks for the early review! Here is a new CI:
Looking into the output, display tests can apparently run on Mac!
|
81e2a08 to
5f2d785
Compare
|
The failures on Mac (and some on Windows) are due to environment problems. The last commit fixes the ones for Mac:
For Windows, I haven't tried, yet - but at least we now know that we can run all tests on Mac! |
|
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. |
|
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. |
8a62673 to
290c305
Compare
|
I did a rebase and had a closer look again.
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:
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? |
440c6cc to
193c43e
Compare
|
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:
The failed tests don't have anything to do with this. |
193c43e to
6f27b4d
Compare
|
Rebased after changes on ros2 branch. New CI:
|
rviz_common/CMakeLists.txt
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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).
|
I'm trying to rebase on master, but I'm having trouble with the new resource_retriever dependency in rviz_default_plugins. |
|
Also, with respect to the Windows issues, one problem might be that we run the jenkins jobs as the |
|
Using the system user might of course change a few things. Maybe I can reproduce the failures with that. |
6f27b4d to
365edd2
Compare
|
Let's see where we are at:
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). |
|
I had to disable a few more tests and introduce some Mac pragmas:
|
6d2b77c to
5631713
Compare
|
CI after rebase:
|
- 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
87cc36b to
c958281
Compare
|
I fixed all the merge conflicts and included your review comments. New CI run:
|
| ${Qt5Widgets_INCLUDE_DIRS} | ||
| ${resource_retriever_INCLUDE_DIRS} | ||
| ${TinyXML_INCLUDE_DIRS} | ||
| ${urdf_INCLUDE_DIRS} |
There was a problem hiding this comment.
This patch broke the build again when urdf is installed on the system. See #243 for a temp. workaround.
This is a followup for #175 . It aims to fix all display tests and run them when possible.
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.