strands-project / mongodb_store

MongoDB tools for storing and analysing runs of ROS systems.
BSD 3-Clause "New" or "Revised" License
49 stars 72 forks source link

[fix] Kinetic devel mongocxx c++11 #224

Closed bbferka closed 6 years ago

bbferka commented 6 years ago

@furushchev @hawesie it seems like #223 did not completely solve the problem. I added ros-shadow-fixed to my sources and installed libmongocxx but I still end up with the undefined behaviour. This is weird because building from source workes just fine, so I've been looking through the Jenkins build logs on build.ros.org:

http://build.ros.org/job/Kbin_uX64__libmongocxx_ros__ubuntu_xenial_amd64__binary/14/consoleFull

It looks like it built without C++11 support (search for c++11 in the log) on the build farm. The reason is that during this build there is no ROS environment being sourced, so the check for ROS_DISTRO does not work.

I've changed this now to check if the variable is set, and if it's not check for the ubuntu version; What do you think?

hawesie commented 6 years ago

lsb_release is not available on all platforms where this might be built, but I think this extra check is really only needed for the build farms which I assume will all have it. Building from source in a normal catkin ws shouldn't reach that part.

bbferka commented 6 years ago

@hawesie yes, I was afraid that lsb_release could be a problem on other platforms. If it does not exist the command will still run, and will simply build without c++11 support. Thanks for merging. I hope this fixes our error now.

EDIT: apparently they don't have lsb_release:

http://build.ros.org/job/Kbin_uX64__libmongocxx_ros__ubuntu_xenial_amd64__binary/15/consoleFull

I again built without C++11; Do you know if there is a simple way of testing this without sending PRs and releasing?

hawesie commented 6 years ago

Oh no! I don't know I'm afraid. Paging @marc-hanheide who might know a bit more about testing builds for the build farm. (he's away for a few days though)

marc-hanheide commented 6 years ago

Did you check the development builds on our own build farm: https://lcas.lincoln.ac.uk/buildfarm/job/Kdev__mongodb_store__ubuntu_xenial_amd64/

First check is to see if they build the way expect them.

bbferka commented 6 years ago

@marc-hanheide yes, those builds are correct. It is interesting, that even on the ROS buildfarm, when looking at:

http://build.ros.org/job/Kbin_uX64__mongodb_store__ubuntu_xenial_amd64__binary/16/consoleFull

, it builds with the correct flag, finds that it is kinetic, uses the c++11 flag;

It is only when building the deb for libmongocxx, that it fails at every check and ends up building without C++11 support.

marc-hanheide commented 6 years ago

Yeah, the debs only build with a bare minimum of dependencies, i.e. likely without lsb_release installed.

bbferka commented 6 years ago

hmm, so do you see any other CMake magic options for building without C++11 on indigo or could we create a separate indigo-devel branch where build is without c++11 and the kinetic-devel branch has it turned on.

furushchev commented 6 years ago

I'm grasping at a straw, how about checking /etc/lsb-release file manually in CMakeLists.txt then?

bbferka commented 6 years ago

@furushchev good point; I will try with that; Although it would be really nice to be able to see the behaviour without the need for a new release :).

@marc-hanheide yes it seems to be with really the bare minimum. What struck me odd is that libmongocxx_ros is a catkin package, but the deb is built without a ROS environment sourced (otherwise ROS_DISTRO would be defined)

bbferka commented 6 years ago

@furushchev after looking at this for a while, it seems like /etc/os-release is the recommended file to look at. One problem though: the content of it is not well defined, i.e.: Ubuntu has VERSION_CODENAME= but Debian does not :(. I am fine with looking only at Ubuntu releases, although this is not an elegant solution.

Different idea that a colleague just gave me. What about putting lsb-release as a buildtool-dependency? This way it would get installed in the docker on the build farm. @hawesie You wrote that lsb_release is not available for all platforms that the build is performed for. Which platforms do you think would don't have it ? We only looked at Denian and Ubuntu, and those looked fine.

hawesie commented 6 years ago

@bbferka I build on OSX, but in a catkin environment so the distro version is available.

bbferka commented 6 years ago

@hawesie true, in that case putting lsb-release in the package.xml is not a good idea. I presume it would break building from source on OSX. I will open a PR with the suggestion from Yuki to read /etc/os-release from CMake and lookup the codename from there. I really hope that solves the build on the ros farm.

dirk-thomas commented 6 years ago

The reason is that during this build there is no ROS environment being sourced, so the check for ROS_DISTRO does not work.

This is not correct. When looking through the build log you will find the following line:

if [ -f "/opt/ros/kinetic/setup.sh" ]; then . "/opt/ros/kinetic/setup.sh"; fi && \

So the environment is being set up (if the ROS package depends on catkin).

The binary jobs for the other packages have actually been built with c++11. The reason is that mongodb_log depends on roslib which depends on ros_environment which brings in the ROS_DISTRO environment variable. The same for mongodb_store: it depends on rostest which depends on roslaunch which depends on roslib which depends on ros_environment.

Only libmongocxx_ros didn't have that environment variable set and therefore built without c++11 becasue it doesn't have a (recursive) dependency on ros_environment. If you add this dependency to the package it will enable c++11 just fine.

bbferka commented 6 years ago

@dirk-thomas thank you for your detailed explanation. This explains everything. I saw that all the other packages were built correctly, but I did not know about the souerce of ROS_DISTRO. I will definitely make the necessary changes and send a new PR, such that the release is up to the standards.