move headers to include/xmlrpcpp

[jspricke]

No description provided.

[jspricke]

I know that it breaks backward compatibility, but the old location clashes on Debian and fixing downstream code should be really easy. So maybe consider for lunar instead?

[mikepurvis]

IMO this could safely merge to Kinetic if it came with a CMakeLists change to maintain compatibility:

# The include/xmlrpcpp path is only for compatibility, to be removed in Lunar.
catkin_package(
  INCLUDE_DIRS include include/xmlrpcpp
  LIBRARIES xmlrpcpp
  CATKIN_DEPENDS cpp_common
)

# The include/xmlrpcpp path is only for compatibility, to be removed in Lunar.
include_directories(include include/xmlrpcpp ${catkin_INCLUDE_DIRS})

[jspricke]

Good idea. Why adding the include_directories? Shouldn't the INCLUDE_DIRS be enough?

[mikepurvis]

include_directories affects any binaries being built in the immediate directory or subdirectories of it: https://cmake.org/cmake/help/v3.0/command/include_directories.html

catkin_package (INCLUDE_DIRS) affects what goes into the find module and pkgconfig— it's what will be put on the include path for other packages which build_depend on this one, directly or transitively.

[jspricke]

What I'm saying is that it should be enough to add the INCLUDE_DIRS since I fixed all the include_directories in the this package.

[mintar]

Just for clarity, this is @jspricke's counter-proposal:

# The include/xmlrpcpp path is only for compatibility, to be removed in Lunar.
catkin_package(
  INCLUDE_DIRS include include/xmlrpcpp
  LIBRARIES xmlrpcpp
  CATKIN_DEPENDS cpp_common
)

include_directories(include ${catkin_INCLUDE_DIRS})

[mikepurvis]

Oh I see what you mean. Yes, that makes sense.

[jspricke]

thanks guys, added :).

[dirk-thomas]

I tried building the branch but the xmlrcpcpp package still only exports xmlrpcpp_INCLUDE_DIRS with <prefix>/include and doesn't expose the <prefix>/include/xmlrpccpp path. The catkin_package API assumes that all includes are later installed in an FHS compliant fashion and only <prefix>/include needs to be exported in the install case. I think the additional path needs to be exported using a CMake extra file.

This can easily be tested by not changing the #include directives to ensure the desired backward compatibility works.

[jspricke]

@dirk-thomas works fine over here:

$ grep _include_dirs devel/share/xmlrpcpp/cmake/xmlrpcppConfig.cmake
set(_include_dirs "ros_comm/utilities/xmlrpcpp/include;ros_comm/utilities/xmlrpcpp/include/xmlrpcpp"

(I've tested it by recompiling roscpp with the old includes as well)

[dirk-thomas]

The devel space works differently. Please check the install space. There it is missing.

[jspricke]

@dirk-thomas got it, changed.

[dirk-thomas]

The value should be enclosed in quotes - just in case some part of the path contains spaces.

[jspricke]

Done, @dirk-thomas can you add them here a well: http://answers.ros.org/question/93266/how-to-export-non-standard-include-directories-in-catkin/

[mikepurvis]

LGTM.

[gavanderhoorn]

I'm not sure I'm doing everything correctly, but while trying to build Kinetic desktop-full from sources on Ubuntu Xenial (amd64) I'm running into a header inclusion problem with XmlRpcValue.h in diagnostic_aggregator:

In file included from /catkin_ws/src/diagnostics/diagnostic_aggregator/src/analyzer_group.cpp:37:0:
/catkin_ws/src/diagnostics/diagnostic_aggregator/include/diagnostic_aggregator/analyzer_group.h:51:25: fatal error: XmlRpcValue.h: No such file or directory
compilation terminated.
make[2]: *** [CMakeFiles/diagnostic_aggregator.dir/src/analyzer_group.cpp.o] Error 1

This seems to be the same issue that is reported in ros/geometry#144 (comment).

It should probably be fixed in ros/diagnostics, but seeing as the change introduced in this PR was supposed to be bw-compatible, I figured I report it here as well.

Adding include/xmlrpcpp to the catkin_package(..) INCLUDE_DIRS in ros_com/xmlrpcpp/CMakeLists.txt:12 does seem to make the build continue.

I'm rather confused as to why this wasn't detected by the CI builds of the pkgs involved.

[gavanderhoorn]

Perhaps the extras file is not doing what it should be doing?

[jspricke]

Which ROS version of desktop-full do you compile? Do you have a minimal example for me to reproduce? Can you check if the xmlrpcpp-extras.cmake is there and used?
Also, can you do a pull request for diagnostics?

[jspricke]

ah, and if it's a real bug, please open a new issue, this one is closed and new commends tend to get lost.

[gavanderhoorn]

re: which version: should have mentioned that, edited my initial comment.

And I should probably also mention I'm building using catkin_tools.

Do you have a minimal example for me to reproduce?

minimal: no? But just following the Kinetic source installation procedure makes this pop up.

Can you check if the xmlrpcpp-extras.cmake is there and used?

It's there. Adding a debug message(..) to it makes it seem as if it's correctly being loaded / evaluated.

[gavanderhoorn]

Opened #1248.