Plain cmake

[jontje]

Draft PR that turns the library into a plain cmake package, and it aims to solve issue https://github.com/ros-industrial/abb_libegm/issues/3.

The main reason for this is to be able to build and use the library regardless if e.g.:

• Ubuntu or Windows
• ROS or ROS2

[gavanderhoorn]

A high-level comment: we'll have to decide which versions of OS we'd like to support here: Ubuntu Xenial (still supported till April 2021) comes with CMake 3.5.1 by default.

While CMake can be upgraded -- even using a Kitware supported apt repository -- this doesn't solve the problems of dependencies not necessarily being up-to-speed with modern CMake.

This PR in its current form would already exclude use of Xenial, due to the cmake_minimum_required(VERSION 3.6).

[gavanderhoorn]

@jontje: I've just added 1576b74 which adds a build for Dashing.

[traversaro]

A high-level comment: we'll have to decide which versions of OS we'd like to support here: Ubuntu Xenial (still supported till April 2021) comes with CMake 3.5.1 by default. This PR in its current form would already exclude use of Xenial, due to the cmake_minimum_required(VERSION 3.6).

The fact that Xenial was already excluded is the reason why I suggest requiring CMake 3.9 . If we want instead to keep compatibility with Xenial and CMake 3.5, probably it would be too much effort to be prepare a relocatable package.

While CMake can be upgraded -- even using a Kitware supported apt repository -- this doesn't solve the problems of dependencies not necessarily being up-to-speed with modern CMake.

If this was related to the Protobuf or Boost, what needs to be recent to have imported targets is not the Protobuf or Boost version, but the FindBoost.cmake and FindProtobuf.cmake shipped with CMake, and in their case the CMake imported targets are defined there and not in any Protobuf/Boost file.

[jontje]

A high-level comment: we'll have to decide which versions of OS we'd like to support here: Ubuntu Xenial (still supported till April 2021) comes with CMake 3.5.1 by default.

I was not considering that. The reason I put 3.6 is that I thought FindProtobuf set variables as Protobuf_ instead of PROTOBUF_, according to 3.5 and 3.6 respectively.

But I tested with cmake 3.10.2 just now, and then it seems that both Protobuf_ and PROTOBUF_ variables are populated. Is this usually the case between cmake versions? I.e. for backwards-compatibility?

Regardless, then I think it would be good to still support Ubuntu Xenial from the get-go.

[traversaro]

Regardless, then I think it would be good to still support Ubuntu Xenial from the get-go.

Ack, then feel free to drop all the comments about non-relocatable packages, as the complexity of doing that with CMake <= 3.9 is probably not worth the gain.

[gavanderhoorn]

Interestingly the ROS 2 build succeeded. The ROS 1 build didn't.

I haven't looked into it yet.

[jontje]

Interestingly the ROS 2 build succeeded. The ROS 1 build didn't.

I haven't looked into it yet.

As far as I could see, then it is the cmake version 3.6 requirement that made the ROS 1 build fail, due to ROS Kinetic distribution.

[gavanderhoorn]

Interestingly the ROS 2 build succeeded. The ROS 1 build didn't. I haven't looked into it yet.

As far as I could see, then it is the cmake version 3.6 requirement that made the ROS 1 build fail, due to ROS Kinetic distribution.

yes, looks like it.

[jontje]

@traversaro and @gavanderhoorn, I think I have addressed most of your comments (except the ones related to relocation).

Thanks a lot for your valuable remarks!

[jontje]

@traversaro thanks for your latest comments, I think I have considered them all.

The library builds fine and I have run RobotStudio simulations for both Ubuntu and Windows applications, so I will mark this PR ready for review.

[traversaro]

@traversaro thanks for your latest comments, I think I have considered them all.

Yes, thanks a lot for the effort!

[traversaro]

Just to understand, for Windows you used protobuf installed from the ROS's Chocolatey distribution, right?

[jontje]

Just to understand, for Windows you used protobuf installed from the ROS's Chocolatey distribution, right?

Yes, that is right.

[traversaro]

Just to let you know, we are having problems on Windows with Protobuf 3.10 (ROS repos have 3.6), but this probably due to an issue in Protobuf, not due to something wrong in abb_libegm , cc @AlbertoRemusIIT .

[traversaro]

Just to let you know, we are having problems on Windows with Protobuf 3.10 (ROS repos have 3.6), but this probably due to an issue in Protobuf, not due to something wrong in abb_libegm , cc @AlbertoRemusIIT .

We investigate a bit more about this, and the issue is confirmed also when using the Chocolatey-installed protobuf used in ROS for Windows. If abb_libegm is compiled as a shared library, as soon as you try to link it to an executable, we get the following linker error:

abb_libegm_test.obj : error LNK2019: unresolved external symbol "class abb::egm::wrapper::CartesianDefaultTypeInternal abb::egm::wrapper::_Cartesian_default_instance_" (?_Cartesian_default_instance_@wrapper@egm
@abb@@3VCartesianDefaultTypeInternal@123@A) referenced in function "void __cdecl ____C_A_T_C_H____T_E_S_T____11(void)" (?____C_A_T_C_H____T_E_S_T____11@@YAXXZ) [C:\ros-ws\idjl-software\build\src\libraries\idjl-
testsuite\idjl-testsuite.vcxproj]
abb_libegm_test.obj : error LNK2019: unresolved external symbol "class abb::egm::wrapper::CartesianPoseDefaultTypeInternal abb::egm::wrapper::_CartesianPose_default_instance_" (?_CartesianPose_default_instance_
@wrapper@egm@abb@@3VCartesianPoseDefaultTypeInternal@123@A) referenced in function "void __cdecl ____C_A_T_C_H____T_E_S_T____11(void)" (?____C_A_T_C_H____T_E_S_T____11@@YAXXZ) [C:\ros-ws\idjl-software\build\src
\libraries\idjl-testsuite\idjl-testsuite.vcxproj]
abb_libegm_test.obj : error LNK2019: unresolved external symbol "class abb::egm::wrapper::CartesianSpaceDefaultTypeInternal abb::egm::wrapper::_CartesianSpace_default_instance_" (?_CartesianSpace_default_instan
ce_@wrapper@egm@abb@@3VCartesianSpaceDefaultTypeInternal@123@A) referenced in function "void __cdecl ____C_A_T_C_H____T_E_S_T____11(void)" (?____C_A_T_C_H____T_E_S_T____11@@YAXXZ) [C:\ros-ws\idjl-software\build
\src\libraries\idjl-testsuite\idjl-testsuite.vcxproj]
abb_libegm_test.obj : error LNK2019: unresolved external symbol "class abb::egm::wrapper::EulerDefaultTypeInternal abb::egm::wrapper::_Euler_default_instance_" (?_Euler_default_instance_@wrapper@egm@abb@@3VEule
rDefaultTypeInternal@123@A) referenced in function "void __cdecl ____C_A_T_C_H____T_E_S_T____11(void)" (?____C_A_T_C_H____T_E_S_T____11@@YAXXZ) [C:\ros-ws\idjl-software\build\src\libraries\idjl-testsuite\idjl-t
estsuite.vcxproj]
abb_libegm_test.obj : error LNK2019: unresolved external symbol "class abb::egm::wrapper::FeedbackDefaultTypeInternal abb::egm::wrapper::_Feedback_default_instance_" (?_Feedback_default_instance_@wrapper@egm@ab
b@@3VFeedbackDefaultTypeInternal@123@A) referenced in function "void __cdecl ____C_A_T_C_H____T_E_S_T____11(void)" (?____C_A_T_C_H____T_E_S_T____11@@YAXXZ) [C:\ros-ws\idjl-software\build\src\libraries\idjl-test
suite\idjl-testsuite.vcxproj]
abb_libegm_test.obj : error LNK2019: unresolved external symbol "class abb::egm::wrapper::HeaderDefaultTypeInternal abb::egm::wrapper::_Header_default_instance_" (?_Header_default_instance_@wrapper@egm@abb@@3VH
eaderDefaultTypeInternal@123@A) referenced in function "void __cdecl ____C_A_T_C_H____T_E_S_T____11(void)" (?____C_A_T_C_H____T_E_S_T____11@@YAXXZ) [C:\ros-ws\idjl-software\build\src\libraries\idjl-testsuite\id
jl-testsuite.vcxproj]
abb_libegm_test.obj : error LNK2019: unresolved external symbol "class abb::egm::wrapper::RobotDefaultTypeInternal abb::egm::wrapper::_Robot_default_instance_" (?_Robot_default_instance_@wrapper@egm@abb@@3VRobo
tDefaultTypeInternal@123@A) referenced in function "void __cdecl ____C_A_T_C_H____T_E_S_T____11(void)" (?____C_A_T_C_H____T_E_S_T____11@@YAXXZ) [C:\ros-ws\idjl-software\build\src\libraries\idjl-testsuite\idjl-t
estsuite.vcxproj]
C:\ros-ws\idjl-software\build\bin\Release\idjl-testsuite.exe : fatal error LNK1120: 7 unresolved externals [C:\ros-ws\idjl-software\build\src\libraries\idjl-testsuite\idjl-testsuite.vcxproj]

As soon as abb_libegm is compiled as a static library, everything works fine. So, unless I am missing something, the library was never tested by linking to an executable when compiled as shared in Windows?

Some investigation in the issue revealed that the problems is that the Protobuf generated code include some static variables, that need to be exported manually with the visibility macro as they are not automatically exported by the WINDOWS_EXPORT_ALL_SYMBOLS target property, as mentioned in https://blog.kitware.com/create-dlls-on-windows-without-declspec-using-new-cmake-export-all-feature/ .

Fortunately, the protobuf generator of C++ files permits to declare a decorator to add to all the symbols to export via the dllexport_decl command line option, that is also exposed as a EXPORT_MACRO in the protobuf_generate_cpp CMake function ( https://cmake.org/cmake/help/v3.9/module/FindProtobuf.html ).

Unfortunately, if we add in the CMake build system the following line:

protobuf_generate_cpp(EgmProtoSources EgmProtoHeaders EXPORT_MACRO ABB_LIBEGM_PUBLIC ${EgmProtoFiles})

the compilation fails, because in the the .pb.h and in the .pb.cc files, there is no option to specify an additional include header, in our case <abb_libegm/visibility_control.hh> or <visibility_control.hh> (see https://github.com/protocolbuffers/protobuf/issues/4198).

I checked a bit how other projects deal with this, and Gazebo and Ignition Msgs actually implement their own C++ protobuf-based code generator to include the visibility headers (see https://bitbucket.org/osrf/gazebo/src/804410860234af97c5e309896dc007e8cde04ba8/gazebo/msgs/generator/GazeboGenerator.cc#lines-72 and https://bitbucket.org/ignitionrobotics/ign-msgs/src/18640ef6c31777e25953ae44b6ad10ed68c4993e/src/Generator.cc#lines-106), but that seems definitely an overkill for this project.

There may be other solutions, such as do a post-processing of the protobuf generated headers to add the include headers via CMake, but I feel they are out of the scope of this PR, and for the time being the easiest solution is to force the library to be compiled as static in Windows.

[jontje]

As soon as abb_libegm is compiled as a static library, everything works fine. So, unless I am missing something, the library was never tested by linking to an executable when compiled as shared in Windows?

I have linked the libary as shared to an executable in Windows and got it to work. But I have tested a bit more and if I use the mutable_<field> accessors, then it works but not with the <field> version. Then I get similar errors as you posted.

E.g.:

abb::egm::wrapper::Output output;
output.mutable_robot()->mutable_joints()->mutable_position()->values_size(); // Works
output.robot().joints().position().values_size(); // Doesn't work

Fortunately, the protobuf generator of C++ files permits to declare a decorator to add to all the symbols to export via the dllexport_decl command line option, that is also exposed as a EXPORT_MACRO in the protobuf_generate_cpp CMake function ( https://cmake.org/cmake/help/v3.9/module/FindProtobuf.html ).

Is this only for CMake 3.9 and newer? If I looked at older documentation that option doesn't seem to exist. Would it be possible to add some if-case for Windows which uses a macro that expands to either __declspec(dllexport) or __declspec(dllimport)? Does it even make sense?

There may be other solutions, such as do a post-processing of the protobuf generated headers to add the include headers via CMake, but I feel they are out of the scope of this PR, and for the time being the easiest solution is to force the library to be compiled as static in Windows.

Yes, that might be the easiest solution for now. So something like this? To make static as default on windows.

if(NOT DEFINED BUILD_SHARED_LIBS)
  if(WIN32)
    option(BUILD_SHARED_LIBS "Build dynamically-linked binaries" OFF)
  else()
    option(BUILD_SHARED_LIBS "Build dynamically-linked binaries" ON)
  endif()
endif()

[traversaro]

I have linked the libary as shared to an executable in Windows and got it to work. But I have tested a bit more and if I use the mutable_<field> accessors, then it works but not with the <field> version. Then I get similar errors as you posted.

That is interesting, and in that executable you included the .pb.h headers of the generated protobuf messages? (Edit: clearly you included the headers to use the mutable accessors). However, this may suggest to it could make sense to add a small smoke compilation test to the repo only when BUILD_TESTING is defined, to make sure that we don't have regression of this kind (the test can also just be used to verify that the linking works fine, and not do anything when executed).

[traversaro]

Yes, that might be the easiest solution for now. So something like this? To make static as default on windows.

if(NOT DEFINED BUILD_SHARED_LIBS)
  if(WIN32)
    option(BUILD_SHARED_LIBS "Build dynamically-linked binaries" OFF)
  else()
    option(BUILD_SHARED_LIBS "Build dynamically-linked binaries" ON)
  endif()
endif()

As BUILD_SHARED_LIBS is broken on Windows, I would probably just avoid to set it as an option, so simply:

 if(NOT DEFINED BUILD_SHARED_LIBS)
   if(WIN32)
       # Add a reference to an issue on the problem with protobuf
     set(BUILD_SHARED_LIBS  OFF)
   else()
     set(BUILD_SHARED_LIBS "Build dynamically-linked binaries" ON)
   endif()
endif()

[traversaro]

Fortunately, the protobuf generator of C++ files permits to declare a decorator to add to all the symbols to export via the dllexport_decl command line option, that is also exposed as a EXPORT_MACRO in the protobuf_generate_cpp CMake function ( https://cmake.org/cmake/help/v3.9/module/FindProtobuf.html ).

Is this only for CMake 3.9 and newer? If I looked at older documentation that option doesn't seem to exist. Would it be possible to add some if-case for Windows which uses a macro that expands to either __declspec(dllexport) or __declspec(dllimport)? Does it even make sense?

Yes, I think it make sense to have a check on the CMake version that is greater then 3.9 to add that option. Furthremore, it is also possible to force that the cmake minimum required version is 3.9 on Windows, as typically Windows user do not have any requirement on being compatible with older CMake versions.

[traversaro]

Based on the answer in https://stackoverflow.com/questions/17980467/protocol-buffers-generate-non-inline-accessors , I was able to fix the linking problem for shared library in MSVC by adding the following CMake code:

if(MSVC)
  cmake_minimum_required(VERSION 3.9)
else()
  cmake_minimum_required(VERSION 3.5)
endif()

# ... 

if(MSVC)
  protobuf_generate_cpp(EgmProtoSources EgmProtoHeaders EXPORT_MACRO ABB_LIBEGM_PUBLIC ${EgmProtoFiles})
else()
  protobuf_generate_cpp(EgmProtoSources EgmProtoHeaders ${EgmProtoFiles})
endif()

# ...

if(MSVC)
  target_compile_options(${PROJECT_NAME} PUBLIC "/FIabb_libegm/visibility_control.h")
endif()

The /FI is used to force the inclusion of a given header for all the compilation units that link abb_libegm. The solution is a bit brittle, but for sure it is more simple then implementing a custom generator or a protoc wrapper as done in Chromium ( https://github.com/protocolbuffers/protobuf/issues/4198#issuecomment-557508506 ).

[gavanderhoorn]

This is not directed at anyone here, but really, how ugly and convoluted can we make a build script?

And then people complain that Catkin / Ament are unnecessary / convoluted / too complex.

I'm actually happy that we have a set of CMake functions/macros that takes care of some of the boilerplate stuff when exporting and importing dependencies.

[traversaro]

This is not directed at anyone here, but really, how ugly and convoluted can we make a build script?

And then people complain that Catkin / Ament are unnecessary / convoluted / too complex.

I'm actually happy that we have a set of CMake functions/macros that takes care of some of the boilerplate stuff when exporting and importing dependencies.

Related to that, there are some related issues in upstream CMake on how to simplify the process of exporting CMake packages, that if implemented would reduce drastically the amount of boilerplate code: https://gitlab.kitware.com/cmake/cmake/issues/18634 .

[jontje]

However, this may suggest to it could make sense to add a small smoke compilation test to the repo only when BUILD_TESTING is defined, to make sure that we don't have regression of this kind (the test can also just be used to verify that the linking works fine, and not do anything when executed).

That's a good idea, but maybe do it in another PR?

[jontje]

This is not directed at anyone here, but really, how ugly and convoluted can we make a build script?

Any suggestion for solving the issue?

Maybe require CMake 3.9 anyway? Is it "bad practice" to hide away code in a cmake module? If not, perhaps that would make the CMakeLists file clearer.

To me it seems a bit tricky to get the library to build properly, without some unfortunate special cases. Regardless, I really appreciate all the time you guys (@gavanderhoorn and @traversaro) have spent on this PR so far.

[traversaro]

That's a good idea, but maybe do it in another PR?

Yes totally, also for the Windows+Shared I think it make sense to do that in another PR.

[jontje]

I have created a new branch that I think would clean-up the CMake files, as well as added @traversaro's example for fixing the linking issue on Windows. I didn't add it to this PR yet, since I has been waiting for some comments from @gavanderhoorn, but I think this will have to wait until next year.

[gavanderhoorn]

Getting back to this I think we can merge this as-is.

One comment: this will break peoples workspaces when they are currently using abb_libegm and building it with catkin_make (as plain catkin doesn't support plain cmake packages).

We should add a section to the readme explaining this is a pure CMake package and that ROS users should use catkin_tools to build it (or colcon).

[gavanderhoorn]

I'll leave the merge to you @jontje.

Could you also update the readme?

We should also post about this breaking change on ROS Discourse. I can do that if you'd like. Let me know.

[jontje]

Thanks for the reviews @traversaro and @gavanderhoorn! 👍

I'll leave the merge to you @jontje.

Could you also update the readme?

We should also post about this breaking change on ROS Discourse. I can do that if you'd like. Let me know.

I will update the readme, and you are more than welcome to post on ROS Discourse this time @gavanderhoorn 😄

[gavanderhoorn]

Done: Conversion to plain CMake of abb_libegm (and abb_librws).

[gavanderhoorn]

You will update the readme?

[jontje]

You will update the readme?

Yes, I will do that.