-
Notifications
You must be signed in to change notification settings - Fork 54
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
Plain cmake #63
Plain cmake #63
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments. Probably none of those is blocking the merge, but I think you may find some of them interesting.
CMakeLists.txt
Outdated
${Boost_INCLUDE_DIRS} | ||
${PROTOBUF_INCLUDE_DIRS} | ||
${catkin_INCLUDE_DIRS}) | ||
add_library(${PROJECT_NAME} SHARED ${SRC_FILES}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is any specific reason why you won't leave the user the possibility of selecting the type of the library (SHARED
or STATIC
) using the standard BUILD_SHARED_LIBS
variable? If you want to default to use SHARED
, you can simply declare the BUILD_SHARED_LIBS
variable as an option whose default value is ON
, as suggested in CMake docs itself: https://cmake.org/cmake/help/v3.6/variable/BUILD_SHARED_LIBS.html . This would simplify packaging this lib for vcpkg (https://docs.microsoft.com/en-us/cpp/build/vcpkg?view=vs-2019).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason, it was like that in the generated cmake package template (from the ROS2 command line interface) that I used as a base. I.e. the template found here.
I like your arguments for making it an option instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason, it was like that in the generated cmake package template (from the ROS2 command line interface) that I used as a base. I.e. the template found here.
I like your arguments for making it an option instead.
Thanks, I know where to open an issue to ask to make it an option then. :D
CMakeLists.txt
Outdated
PUBLIC ${Boost_INCLUDE_DIRS} | ||
PUBLIC ${Protobuf_INCLUDE_DIRS} | ||
) | ||
target_link_libraries(${PROJECT_NAME} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Boost and Protobuf headers are used in the public headers of the libraries, I suggest to mark them as PUBLIC
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for my lack of knowledge, but could you explain how to mark them as PUBLIC
? Doesn't lines 68-69 already do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for my lack of knowledge, but could you explain how to mark them as
PUBLIC
? Doesn't lines 68-69 already do that?
Lines 68-69 tells the preprocessor of the downstream compilation unit that uses abb_libegm
the include directories of Boost and Protobuf need to be appended to the search path of includes (tipically on GCC this is done via the -I
compilation option).
Marking as PUBLIC (by doing target_link_libraries(${PROJECT_NAME} PUBLIC ${Boost_LIBRARIES} ${Protobuf_LIBRARIES}
) the linked libraries, tells the linker of the downstream library that uses abb_libegm
, to also link the Protobuf and Boost libraries when abb_libegm
is linked. This is typically done (in *nix) via the -l
linker option.
With CMake >= 3.9, you can just declare as PUBLIC
in target_link_libraries
the linking of the Boost's and Protobuf's imported targets, and that includes both passing the right library to the linker, and the right include dirs to the preprocessor, thanks to the use of so-called "Usage Requirements", see https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#build-specification-and-usage-requirements . If you want a more in-depth discussion, the best reference for this that I ever found is the book https://crascit.com/professional-cmake/ , but it is not available free of charge.
CMakeLists.txt
Outdated
) | ||
|
||
# Create the ${PROJECT_NAME}Config.cmake. | ||
set(CONF_INCLUDE_DIRS "${CMAKE_INSTALL_PREFIX}/include") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will point to the wrong include dirs if during configuration the user change the GNUInstallDirs's variable CMAKE_INSTALL_INCLUDEDIR
. Furthermore, the include directories are already included in the imported target, so there are more maintainable ways of exporting this info ensuring that the package remains relocatable.
cmake/abb_libegmConfig.cmake.in
Outdated
# abb_libegm_LIBRARIES - libraries to link against | ||
# abb_libegm_EXECUTABLE - the bar executable | ||
|
||
set(abb_libegm_INCLUDE_DIRS "@CONF_INCLUDE_DIRS@") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are already exporting properly configured CMAke exported targets, there is little use in having an explicit variable ${abb_libegm_INCLUDE_DIRS}
, as any use of this variable in downstream package will make them non-relocatable. However, if you want to include it, you can extract it directly from the abb_libegm
imported target:
get_target_property(abb_libegm_INCLUDE_DIRS abb_libegm INTERFACE_INCLUDE_DIRECTORIES)
clearly this line should go after the inclusion of the export_abb_libegm.cmake
file.
cmake/abb_libegmConfig.cmake.in
Outdated
set(abb_libegm_INCLUDE_DIRS "@CONF_INCLUDE_DIRS@") | ||
|
||
# Our library dependencies (contains definitions for IMPORTED targets) | ||
include("${abb_libegm_DIR}/export_abb_libegm.cmake") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The classical trick is to use ${CMAKE_CURRENT_LIST_DIR}
to import file that are in the same directory,to avoid assuming that a variable called abb_libegm_DIR
is present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for the pointer!
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 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 |
The fact that Xenial was already excluded is the reason why I suggest requiring CMake 3.9 .
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 |
I was not considering that. The reason I put But I tested with cmake 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. |
#define ABB_LIBEGM_PUBLIC_TYPE | ||
#endif | ||
|
||
#endif // ABB_LIBEGM__VISIBILITY_CONTROL_H_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess also this comes from the ros2 template, but unfortunately this template is based on the wrong section of the Wiki page https://gcc.gnu.org/wiki/Visibility . This is based on the section "How to use the new C++ visibility support" that is meant to be a quick overview of the functionality, while the template to be used is the one under "Step-by-step guide" . The difference are not enormous, but unfortunatly this version will probably create problems when compiling as static library in Windows, see ament/ament_cmake#201 (comment) . Again, I do not care too much about building as a static library per se, but supporting that case is useful to easily submit a port of a C++ library to https://github.com/microsoft/vcpkg , that is a great way to manage C++ libraries on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it comes from the ROS2 template and I will make an update to address this.
Also, thanks for all the insights! They are much appreciated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it comes from the ROS2 template and I will make an update to address this.
I imagined, I will try in the future to report this problems also there!
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 |
yes, looks like it. |
@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! |
CMakeLists.txt
Outdated
|
||
############################# | ||
## Google Protocol Buffers ## | ||
############################# | ||
find_package(Protobuf REQUIRED) | ||
if(WIN32) | ||
# The protobuf compiler appears to be default installed at ${_CMAKE_INSTALL_DIR}/tools/protobuf, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be worth to mention in the comment "appears to be default installed" refers to Chocolatey-based ROS/ROS2 installs.
cmake/abb_libegmConfig.cmake.in
Outdated
include("${CMAKE_CURRENT_LIST_DIR}/abb_libegmTargets.cmake") | ||
|
||
# Extras for our library (exports the dependency on pthread) | ||
include("${CMAKE_CURRENT_LIST_DIR}/abb_libegmExtras.cmake") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that list(APPEND CMAKE_CXX_FLAGS -pthread)
contained in this file will probably break any downstream user of the package on Windows/MSVC that calls find_package(abb_libegm REQUIRED)
, as -pthread
is not a valid C++ flag in Windows/MSVC (cc @AlbertoRemusIIT) . I guess the CI is not testing this, so we did not realize that.
Probably the CMake idiomatic way of dealing with this is to add find_package(Threads REQUIRED)
and a target_link_libraries(${PROJECT_NAME} PUBLIC Threads::Threads)
, and then add:
include(CMakeFindDependencyMacro)
find_dependency(Threads REQUIRED)
here in the abb_libegmConfig.cmake.in
before the inclusion of the abb_libegmTargets.cmake
file.
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 E.g.:
Is this only for
Yes, that might be the easiest solution for now. So something like this? To make static as default on windows.
|
|
As 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() |
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. |
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 |
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 . |
That's a good idea, but maybe do it in another PR? |
Any suggestion for solving the issue? Maybe require 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. |
Yes totally, also for the Windows+Shared I think it make sense to do that in another PR. |
######################### | ||
## Boost C++ Libraries ## | ||
######################### | ||
find_package(Boost REQUIRED COMPONENTS regex system thread) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that the regex
component is not used at all, and it was not present in the previous build system.
find_package(Boost REQUIRED COMPONENTS regex system thread) | |
find_package(Boost REQUIRED COMPONENTS system thread) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't build the libary on Windows (at least with ROS and catkin_make_isolated
) if I don't specify regex
. I don't really know why and I haven't had time to look into it.
I get this linking error:
LINK Pass 1: command "C:\PROGRA~2\MICROS~3\2017\PROFES~1\VC\Tools\MSVC\1416~1.270\bin\Hostx64\x64\link.exe /nologo @CMakeFiles\abb_libegm.dir\objects1.rsp /out:abb_libegm.dll /implib:abb_libegm.lib /pdb:C:\win_ros\ros\win_catkin_ws\build_isolated\abb_libegm\devel\abb_libegm.pdb /dll /version:0.0 /machine:x64 /debug /INCREMENTAL /DEF:CMakeFiles\abb_libegm.dir\exports.def C:\opt\rosdeps\x64\lib\boost_system-vc141-mt-x64-1_66.lib C:\opt\rosdeps\x64\lib\boost_thread-vc141-mt-x64-1_66.lib C:\opt\rosdeps\x64\lib\libprotobuf.lib C:\opt\rosdeps\x64\lib\boost_chrono-vc141-mt-x64-1_66.lib C:\opt\rosdeps\x64\lib\boost_system-vc141-mt-x64-1_66.lib C:\opt\rosdeps\x64\lib\boost_date_time-vc141-mt-x64-1_66.lib C:\opt\rosdeps\x64\lib\boost_atomic-vc141-mt-x64-1_66.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:CMakeFiles\abb_libegm.dir/intermediate.manifest CMakeFiles\abb_libegm.dir/manifest.res" failed (exit code 1104) with the following output:
LINK : fatal error LNK1104: cannot open file 'boost_regex-vc141-mt-x64-1_66.lib'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I had no idea, probably there is some non-declared internal boost dependency, in that case indeed leaving regex is indeed a nice workaround.
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. |
Getting back to this I think we can merge this as-is. One comment: this will break peoples workspaces when they are currently using We should add a section to the readme explaining this is a pure CMake package and that ROS users should use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can merge this.
Let's fix other issues in follow-up PRs.
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. |
list (REMOVE_ITEM PROTOBUF_LIBRARIES "-lpthread") | ||
if(WIN32) | ||
# For a Chocolatey-based ROS/ROS2 installation, then it appears that the protobuf compiler is | ||
# by default installed at ${_CMAKE_INSTALL_DIR}/tools/protobuf, and find_package() doesn't |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed this: _CMAKE_INSTALL_DIR
.
Is that actually a variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, even if a non-documented/unsupported one (see https://gitlab.kitware.com/cmake/cmake/blob/v3.16.2/Modules/Platform/WindowsPaths.cmake#L49). It works fine only because in Chocolatey-based ROS/ROS2 installations CMake and protobuf are installed in the same install prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even if a non-documented/unsupported one
hm ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even if a non-documented/unsupported one
hm ...
Not a big fan of it either, especially as it does not work with a vcpkg-installed protobuf. : )
FILES_MATCHING PATTERN "*.h") | ||
install( | ||
FILES | ||
${EgmProtoFiles} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these go to CMAKE_INSTALL_DATAROOTDIR
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if there is any policy/best practice for this, but for example the ignition-msgs package installs them in the same include directories of C++ include headers, see https://packages.debian.org/sid/amd64/libignition-msgs-dev/filelist . Perhaps @j-rivero knows about some policy regarding this.
Thanks for the reviews @traversaro and @gavanderhoorn! 👍
I will update the readme, and you are more than welcome to post on ROS Discourse this time @gavanderhoorn 😄 |
You will update the readme? |
Yes, I will do that. |
Draft PR that turns the library into a plain cmake package, and it aims to solve issue #3.
The main reason for this is to be able to build and use the library regardless if e.g.: