Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move headers to include/xmlrpcpp #962

Merged
merged 2 commits into from Jan 21, 2017

Conversation

Projects
None yet
5 participants
@jspricke
Copy link
Member

commented Jan 18, 2017

No description provided.

@jspricke

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2017

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 mikepurvis changed the title move heaers to include/xmlrpcpp move headers to include/xmlrpcpp Jan 18, 2017

@mikepurvis

This comment has been minimized.

Copy link
Member

commented Jan 18, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2017

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

@mikepurvis

This comment has been minimized.

Copy link
Member

commented Jan 18, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 18, 2017

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

@jspricke

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2017

thanks guys, added :).

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Jan 18, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2017

@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

This comment has been minimized.

Copy link
Member

commented Jan 18, 2017

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

@jspricke jspricke force-pushed the jspricke:move_xmlrpcpp branch from ed70d43 to d04da46 Jan 18, 2017

@jspricke

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2017

@dirk-thomas got it, changed.

@@ -0,0 +1 @@
list(APPEND @PROJECT_NAME@_INCLUDE_DIRS ${@PROJECT_NAME@_DIR}/../../../@CATKIN_GLOBAL_INCLUDE_DESTINATION@/xmlrpcpp)

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Jan 18, 2017

Member

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

@jspricke jspricke force-pushed the jspricke:move_xmlrpcpp branch from d04da46 to 838e824 Jan 18, 2017

@jspricke

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2017

@mikepurvis

This comment has been minimized.

Copy link
Member

commented Jan 21, 2017

LGTM.

@mikepurvis mikepurvis merged commit a8c5f9e into ros:kinetic-devel Jan 21, 2017

1 check passed

Kpr__ros_comm__ubuntu_xenial_amd64 Build finished.
Details

@jspricke jspricke deleted the jspricke:move_xmlrpcpp branch Jan 22, 2017

ggallagher01 added a commit to clearpathrobotics/ros_comm that referenced this pull request Jan 26, 2017

Move headers to include/xmlrpcpp (ros#962)
* Move headers to include/xmlrpcpp

* Export old include dir for compatibility

hubot pushed a commit to fawkesrobotics/fawkes that referenced this pull request Mar 4, 2017

clips-ros: adapt to moved XmlRpc header
This is due to ros/ros_comm#962, i.e., a
breakage in a minor version update to ROS Kinetic.
@gavanderhoorn

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2017

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

@jspricke

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2017

Opened #1248.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.