ros-interactive-manipulation / sql_database

Provides an easy way to use SQL databases from the ROS environment.
1 stars 7 forks source link

Add support for YAML-CPP 0.5+. #5

Closed cottsay closed 10 years ago

cottsay commented 10 years ago

The new yaml-cpp API removes the "node >> outputvar;" operator, and it has a new way of loading documents. There's no version hint in the library's headers, so I'm getting the version number from pkg-config.

This patch is a port of the ones created by @ktossell for map_server and other packages.

bchretien commented 10 years ago

:+1:

@mateiciocarlie is it possible to have a new release with this patch? This would allow me to package sql_database for Arch Linux.

cottsay commented 10 years ago

@bchretien: Worth noting is that even when this is merged, I've hit a snag on how to handle dependent packages. One of the header files for this package needs HAVE_NEW_YAMLCPP defined, and that header is referenced by other packages that do not directly need yaml-cpp.

This led me to file https://github.com/ros/catkin/issues/610.

This PR gets THIS package compiling, but not it's downstream packages.

Do either of you have any ideas on how we could solve this (besides https://github.com/ros/catkin/issues/610)? Maybe we could use an "extras" file like @wjwwood suggested?

dirk-thomas commented 10 years ago

rosconsole adds a definition in its CMake extra file: https://github.com/ros/ros_comm/blob/indigo-devel/tools/rosconsole/cmake/rosconsole-extras.cmake.in You wanna do something similar here.

bchretien commented 10 years ago

Well, lots of functions in postgresql_database.h could be moved to postgresql_database.cpp, couldn't they? Especially:

inline void operator>>(const YAML::Node& node, PostgresqlDatabaseConfig &options)

where the yaml-cpp conflict happens. Something like https://github.com/bchretien/sql_database/commit/52cf565aa95785ccb3dbfb949639dd4c9211e6b1 (but a possibly safer version that prevents violating the ODR if another library also has these symbols, maybe by putting the operator in an anonymous namespace for both yaml-cpp APIs). Thus, the header does not propagate the fix, and other packages should be fine.

(note: 1AM here so I may be missing something obvious)

bchretien commented 10 years ago

Quick analysis on my patch: I was wondering whether the compiler was using a weak symbol for the operator>>, and apparently it does:

$ nm -CD devel/lib/libpostgresql_database.so | grep "YAML::operator>>"
00000000000266ad W void YAML::operator>><std::string>(YAML::Node const&, std::string&)

So I don't even think that having another library doing the exact same thing would cause any problem if both libraries are linked with the same code. But if there's a safer route, we should go for it.

mateiciocarlie commented 10 years ago

Alright, thanks all for fixes. Merged pull request from @vrabaud, will release later today.

mateiciocarlie commented 10 years ago

Released 0.4.8 in both hydro and indigo.

mateiciocarlie commented 10 years ago

Looks like a bunch of builds are failing... Builds called "Hsrc" seem to work, but Hbin\ fail with the error below. Any thoughts what is going on?

CMake Error at /usr/share/cmake-2.8/Modules/FindPkgConfig.cmake:350 (message):
  None of the required 'yaml-cpp' found
Call Stack (most recent call first):
  CMakeLists.txt:17 (pkg_search_module)
vrabaud commented 10 years ago

This works on image_pipeline here: https://github.com/ros-perception/image_common/blob/hydro-devel/camera_calibration_parsers/CMakeLists.txt#L15

cottsay commented 10 years ago

I can reproduce this if pkg-config or yaml-cpp is not installed. Obviously yaml-cpp gets installed as it is in the log, but I'm not seeing pkg-config get installed. Maybe we should try adding a build-depend on pkg-config and making pkg-config required in cmake.

cottsay commented 10 years ago

Looks like camera_calibration_parsers has a build_depend on pkg-config, so I'm fairly confident now that https://github.com/ros-interactive-manipulation/sql_database/pull/7 will fix the problem.

cottsay commented 10 years ago

Build farm seems to be stable again on G, H and I. https://github.com/ros-interactive-manipulation/sql_database/pull/7 was the issue. Thanks, folks!

mateiciocarlie commented 10 years ago

Thanks a lot all for the help!

Hooray for community support! :+1: