link against tinyxml2 correctly

[Karsten1987]

closes #191
Connects to ros2/rosbag2#402

I don't know exactly why but it looks to me as if tinyxml2_vendor as well as TinyXML2 are not correctly linked to the interface target of pluginlib.

By explicitly linking to tinyx2ml::tinyxml2 - and not adding the vendor package to the targets - the error message in ros2/rosbag2#403 (comment) is being resolved.

As a general question, why should pluginlib export a dependency on the vendor package and not just directly transitively link to tinyxml2?

[Karsten1987]

@dirk-thomas do you mind having a look at this? Not sure exactly how to move forward.

[dirk-thomas]

Since the else case in the CMake module provided by the vendor package doesn't have an interface target tinyx2ml::tinyxml2 (see https://github.com/ros2/tinyxml2_vendor/blob/c89337439ffe9b5123ca310af0d746eb9cbfa110/cmake/Modules/FindTinyXML2.cmake#L180) I doubt the current approach works in all the cases.

[Karsten1987]

That makes total sense.

I think it would make sense to adapt that else logic in the vendor package with the following to make sure that there is a modern cmake target available:

  add_library(tinyxml2 STATIC IMPORTED)
  set_property(TARGET tinyxml2 PROPERTY IMPORTED_LOCATION ${TinyXML2_LIBRARIES})
  set_property(TARGET tinyxml2 PROPERTY INCLUDE_DIRECTORIES ${TinyXML2_INCLUDE_DIRS})
  list(APPEND tinyxml2_vendor_TARGET tinyxml2)

Alternatively, this code snippet could be placed inside the pluginlib's cmake logic and guarded with if(NOT TARGET tinyxml2).

Both approaches seem to work locally for me.

[clalancette]

I'm also running into issues building pluginlib and its dependencies on Ubuntu 18.04 because cmake 3.10 doesn't allow you to IMPORT libraries from other packages. The solution in ros2/tinyxml2_vendor#8 seems to fix it for me, along with removing the export from this package. I'll go ahead and review ros2/tinyxml2_vendor#8.

I also verified that with that fix in place and ros2/rosbag2#403 in place, I was able to build rosbag2 with no absolute paths in it (except for yaml_cpp_vendor, a different issue), and I was also able to build nav2_rviz_plugins (so the problem from #192 is still solved).

[clalancette]

I tried this PR out along with ros2/tinyxml2_vendor#8 and ros2/rosbag2#403 . That replicates the test I made when looking at #193 .

Unfortunately, with this PR, I fail to build. Where I fail is in trying to build rviz_common, with errors like:

[74.522s] librviz_common.so: undefined reference to `tinyxml2::XMLNode::NextSiblingElement(char const*) const'
[74.523s] librviz_common.so: undefined reference to `tinyxml2::XMLDocument::LoadFile(char const*)'
[74.523s] librviz_common.so: undefined reference to `tinyxml2::XMLElement::Attribute(char const*, char const*) const'
[74.524s] librviz_common.so: undefined reference to `tinyxml2::XMLElement::GetText() const'
[74.524s] librviz_common.so: undefined reference to `tinyxml2::XMLDocument::XMLDocument(bool, tinyxml2::Whitespace)'
[74.524s] librviz_common.so: undefined reference to `tinyxml2::XMLNode::Value() const'
[74.525s] librviz_common.so: undefined reference to `tinyxml2::XMLNode::FirstChildElement(char const*) const'
[74.525s] librviz_common.so: undefined reference to `tinyxml2::XMLDocument::~XMLDocument()'
[74.525s] collect2: error: ld returned 1 exit status
[74.526s] make[2]: *** [rviz_common_display_test] Error 1

rviz_common doesn't itself use TinyXML2, so it must be picking up the dependency from pluginlib. However, without pluginlib exporting the TinyXML2 dependency, there's nothing to link against.

That seems like some sort of bug, but I don't know what it is at the moment. I'm going to suggest that we go ahead with #190 and ros2/tinyxml2_vendor#8, which improves the situation. Then we can continue debugging what is going on with downstream consumers. @Karsten1987 what do you think?

[Karsten1987]

But pluginlib does export the dependency? Looking at the CI job here rviz built correctly.

Did you use https://github.com/ros2/rviz/pull/542/files?

[clalancette]

Did you use https://github.com/ros2/rviz/pull/542/files?

I had missed this one. With that as well, things built here.

[mjcarroll]

I also had to use this to resolve similar issues with image_transport