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

Plain cmake #63

Merged
merged 19 commits into from
Jan 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
sudo: required
language: generic
dist: trusty
services:
- docker

env:
matrix:
- ROS_DISTRO="kinetic" ROS_REPO=ros NOT_TEST_INSTALL=true CATKIN_LINT=true
- ROS_DISTRO="kinetic" ROS_REPO=ros CATKIN_LINT=true
- ROS_DISTRO="dashing" ROS_REPO=ros

install:
- git clone --depth=1 https://github.com/ros-industrial/industrial_ci.git .ci_config
- git clone --depth=1 -b master https://github.com/ros-industrial/industrial_ci.git .ci_config

script:
- .ci_config/travis.sh
171 changes: 124 additions & 47 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,52 +1,70 @@
cmake_minimum_required(VERSION 2.8.12)
project(abb_libegm)
cmake_minimum_required(VERSION 3.5)

find_package(catkin REQUIRED)
# Read version from the package.xml file.
file(READ ${CMAKE_CURRENT_SOURCE_DIR}/package.xml package_xml_str)
if(NOT package_xml_str MATCHES "<version>([0-9]+.[0-9]+.[0-9]+)</version>")
message(FATAL_ERROR "Could not parse project version from package.xml. Aborting.")
endif()

# At this point we either have a proper version string, or we've errored
# out with a FATAL_ERROR above. So assume CMAKE_MATCH_1 contains our
# package's version.
project(abb_libegm VERSION ${CMAKE_MATCH_1} LANGUAGES CXX)

include(GNUInstallDirs)

if(WIN32)
set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use GenerateExportHeader instead of a custom visibility header? See also Create dlls on Windows without declspec() using new CMake export all feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! I will add it.

endif()

find_package(Boost REQUIRED COMPONENTS system thread)
#########################
## Boost C++ Libraries ##
#########################
find_package(Boost REQUIRED COMPONENTS regex system thread)
Copy link
Contributor

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.

Suggested change
find_package(Boost REQUIRED COMPONENTS regex system thread)
find_package(Boost REQUIRED COMPONENTS system thread)

Copy link
Contributor Author

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'

Copy link
Contributor

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.


#############################
## Google Protocol Buffers ##
#############################
find_package(Protobuf REQUIRED)

# work around Protobuf exporting 'lpthread' as a library: we
# export the dependency on pthread using the CFG_EXTRAS files
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
Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member

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 ...

Copy link
Contributor

@traversaro traversaro Jan 15, 2020

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. : )

# always seem to find the compiler without specifying a hint.
find_package(Protobuf REQUIRED HINTS ${_CMAKE_INSTALL_DIR}/tools/protobuf)
else()
find_package(Protobuf REQUIRED)
endif()

# Make sure protoc is present, as apparently the above find_package() doesn't check that.
find_program(Protobuf_PROTOC_LOC NAMES protoc)
if (NOT Protobuf_PROTOC_LOC)
if(NOT PROTOBUF_PROTOC_EXECUTABLE)
message(FATAL_ERROR "Cannot find required 'protoc', cannot process Protobuf files without it. Aborting.")
endif()

# Generate C++ for protocol classes (headers and sources get written to the CMAKE_CURRENT_BINARY_DIR location).
# Generate C++ for protocol classes (headers and sources
# get written to the CMAKE_CURRENT_BINARY_DIR location).
set(EgmProtoFiles proto/egm.proto proto/egm_wrapper.proto proto/egm_wrapper_trajectory.proto)
if (NOT QUIET)
if(NOT QUIET)
message(STATUS "Generating protobuf C++ for: ${EgmProtoFiles}")
endif()
protobuf_generate_cpp(EgmProtoSources EgmProtoHeaders ${EgmProtoFiles})

###################################
## catkin specific configuration ##
###################################
catkin_package(
INCLUDE_DIRS
include
LIBRARIES
${PROJECT_NAME}
CFG_EXTRAS
# this exports pthread dependency for us
abb_libegm-extras.cmake
DEPENDS
Boost
PROTOBUF
)
#############
## Threads ##
#############
find_package(Threads REQUIRED)

# Work around Protobuf exporting 'lpthread' as a library: we let the
# previous find_package(...) determine the system's thread library.
list(REMOVE_ITEM PROTOBUF_LIBRARIES "-lpthread")
Comment on lines +50 to +57
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we verified that Protobuf is happy with this in all cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how to verify for all cases.


###########
## Build ##
###########
set(SRC_FILES
if(NOT DEFINED BUILD_SHARED_LIBS)
option(BUILD_SHARED_LIBS "Build dynamically-linked binaries" ON)
endif()

set(
SRC_FILES
src/egm_base_interface.cpp
src/egm_common.cpp
src/egm_common_auxiliary.cpp
Expand All @@ -55,30 +73,89 @@ set(SRC_FILES
src/egm_logger.cpp
src/egm_udp_server.cpp
src/egm_trajectory_interface.cpp
${EgmProtoSources})

include_directories(include
${CMAKE_CURRENT_BINARY_DIR} # Contains protobuf generated sources
${Boost_INCLUDE_DIRS}
${PROTOBUF_INCLUDE_DIRS}
${catkin_INCLUDE_DIRS})
${EgmProtoSources}
)

add_library(${PROJECT_NAME} ${SRC_FILES})
target_include_directories(${PROJECT_NAME} PUBLIC ${CMAKE_CURRENT_BINARY_DIR})
target_link_libraries(${PROJECT_NAME} ${Boost_LIBRARIES} ${PROTOBUF_LIBRARIES} ${catkin_LIBRARIES})
add_library(${PROJECT_NAME}::${PROJECT_NAME} ALIAS ${PROJECT_NAME})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: let's be a little more creative with the namespace and exported targets:

Suggested change
add_library(${PROJECT_NAME}::${PROJECT_NAME} ALIAS ${PROJECT_NAME})
add_library(ABB::libegm ALIAS ${PROJECT_NAME})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd need to something like this as well to do this though:

set_target_properties(${PROJECT_NAME} PROPERTIES EXPORT_NAME libegm)

Copy link
Contributor

@traversaro traversaro Nov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not feel strongly about this, but most CMake package that I know and almost all Find***.cmake scripts in CMake, have the namespace that corresponds to the package name, i.e. if you have find_package(<package>), then your imported targets are <package>::lib1, <package>::lib2 . I don't think this is documented or recommended in any CMake docs, but I feel that changing this would go against the " principle of least astonishment" , https://en.wikipedia.org/wiki/Principle_of_least_astonishment .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like @traversaro's argument, but @gavanderhoorn's approach would "harmonize" better with e.g. abb_librws.

So, I am a little bit split about which to use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@traversaro: least astonishment is certainly something to keep in mind. However, I always have to lookup what names a Find*.cmake defines, as even for 'old style' CMake, there are only conventions and quite a few libraries (even those for which find scripts are distributed with the 'official' CMake distribution) violate all of them (lower case, all upper case, snake-case, with or without dashes, etc, etc).

Proper documentation should be added in all cases I believe.

Capitalising the namespace was something I saw in pabloariasal/modern-cmake-sample (from this article). Now that is n==1 of course, so hardly convincing from that perspective, but I must say I like it.

Another example of where this is used is Boost. The main namespace is Boost, so we get Boost::system. Not Boost_system::Boost_system.

Having written that: it's just a name. If @jontje prefers least-astonishment, let's go with abb_libegm::abb_libegm.

Copy link
Contributor

@traversaro traversaro Nov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capitalising the namespace was something I saw in pabloariasal/modern-cmake-sample (from this article). Now that is n==1 of course, so hardly convincing from that perspective, but I must say I like it.

Another example of where this is used is Boost. The main namespace is Boost, so we get Boost::system. Not Boost_system::Boost_system.

I am probably missing something, but as far as I can see, both example respect the convention that a package found by find_package(<package>) always exports CMake targets in the form of <package>::<libname> , in particular:

<package> <libname> <package>::<libname>
JSONUtils jsonutils JSONUtils::jsonutils
Boost system Boost::system

This convention would be violated if the abb_libegm CMake package exported the imported target ABB::libegm .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having written that: it's just a name. If @jontje prefers least-astonishment, let's go with abb_libegm::abb_libegm.

Same for me, bikeshedding is fun but I would feel fine with either decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with abb_libegm::abb_libegm.


include(GenerateExportHeader)
generate_export_header(${PROJECT_NAME})

target_include_directories(${PROJECT_NAME} PUBLIC
"$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include;${CMAKE_CURRENT_BINARY_DIR}>"
$<INSTALL_INTERFACE:$<INSTALL_PREFIX>/include>
${PROTOBUF_INCLUDE_DIRS}
)

target_link_libraries(${PROJECT_NAME} PUBLIC
Boost::regex
Boost::system
Boost::thread
${PROTOBUF_LIBRARIES}
Threads::Threads
)

gavanderhoorn marked this conversation as resolved.
Show resolved Hide resolved
if(NOT BUILD_SHARED_LIBS)
target_compile_definitions(${PROJECT_NAME} PUBLIC "ABB_LIBEGM_STATIC_DEFINE")
endif()

#############
## Install ##
#############
install(TARGETS ${PROJECT_NAME}
LIBRARY DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION})
install(
DIRECTORY include/
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
)

install(DIRECTORY include/${PROJECT_NAME}/
DESTINATION ${CATKIN_PACKAGE_INCLUDE_DESTINATION}
FILES_MATCHING PATTERN "*.h")
install(
FILES
${EgmProtoFiles}
Copy link
Member

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?

Copy link
Contributor

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.

${EgmProtoHeaders}
${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}_export.h
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/${PROJECT_NAME}
)

install(
TARGETS ${PROJECT_NAME}
EXPORT export_${PROJECT_NAME}
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
)

install(FILES ${EgmProtoHeaders}
DESTINATION ${CATKIN_PACKAGE_INCLUDE_DESTINATION})
include(CMakePackageConfigHelpers)

install(FILES ${EgmProtoFiles}
DESTINATION ${CATKIN_PACKAGE_INCLUDE_DESTINATION})
# Create the ${PROJECT_NAME}Config.cmake.
configure_file(
${CMAKE_CURRENT_SOURCE_DIR}/cmake/${PROJECT_NAME}Config.cmake.in
"${PROJECT_BINARY_DIR}/${CMAKE_FILES_DIRECTORY}/${PROJECT_NAME}Config.cmake" @ONLY
)

# Create the ${PROJECT_NAME}ConfigVersion.cmake.
write_basic_package_version_file(
${PROJECT_BINARY_DIR}/${PROJECT_NAME}ConfigVersion.cmake
COMPATIBILITY AnyNewerVersion
)

install(
FILES
"${PROJECT_BINARY_DIR}/${CMAKE_FILES_DIRECTORY}/${PROJECT_NAME}Config.cmake"
"${PROJECT_BINARY_DIR}/${PROJECT_NAME}ConfigVersion.cmake"
DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/${PROJECT_NAME}
)

# Export targets.
set(export_targets ${export_targets};${PROJECT_NAME})
export(
EXPORT export_${PROJECT_NAME}
FILE "${PROJECT_BINARY_DIR}/${PROJECT_NAME}Targets.cmake"
NAMESPACE ${PROJECT_NAME}::
)

install(
EXPORT export_${PROJECT_NAME}
DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/${PROJECT_NAME}
FILE ${PROJECT_NAME}Targets.cmake
NAMESPACE ${PROJECT_NAME}::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my earlier comment about the namespace:

Suggested change
NAMESPACE ${PROJECT_NAME}::
NAMESPACE ABB::

)
10 changes: 0 additions & 10 deletions cmake/abb_libegm-extras.cmake.in

This file was deleted.

15 changes: 15 additions & 0 deletions cmake/abb_libegmConfig.cmake.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# - Config file for the abb_libegm package
# It defines the following variable
# abb_libegm_LIBRARIES - libraries to link against

include(CMakeFindDependencyMacro)

# Find dependencies
find_dependency(Threads REQUIRED)
find_dependency(Boost REQUIRED COMPONENTS regex system thread)

# Our library dependencies (contains definitions for IMPORTED targets)
include("${CMAKE_CURRENT_LIST_DIR}/@PROJECT_NAME@Targets.cmake")

# These are IMPORTED targets created by @PROJECT_NAME@Targets.cmake
set(abb_libegm_LIBRARIES @PROJECT_NAME@::@PROJECT_NAME@)
20 changes: 11 additions & 9 deletions include/abb_libegm/egm_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
#ifndef EGM_COMMON_H
#define EGM_COMMON_H

#include "abb_libegm_export.h"

namespace abb
{
namespace egm
Expand Down Expand Up @@ -67,13 +69,13 @@ struct Constants
/**
* \brief Constants related to the robot controller system.
*/
struct RobotController
struct ABB_LIBEGM_EXPORT RobotController
{
/**
* \brief Lowest sample time [s] used in EGM communication.
*/
static const double LOWEST_SAMPLE_TIME;

/**
* \brief Default port number assumed for EGM communication.
*/
Expand All @@ -98,7 +100,7 @@ struct Constants
/**
* \brief Constants related to the conversions between units.
*/
struct Conversion
struct ABB_LIBEGM_EXPORT Conversion
{
/**
* \brief Conversion value from radians to degrees.
Expand Down Expand Up @@ -143,14 +145,14 @@ struct BaseConfiguration
use_logging(false),
max_logging_duration(60.0)
{}

/**
* \brief Value specifying if a six or seven axes robot is used.
*
* Note: If set to a seven axes robot, then an implicit mapping of joint values is performed.
*/
RobotAxes axes;

/**
* \brief Flag indicating if demo outputs should be used.
*
Expand All @@ -165,7 +167,7 @@ struct BaseConfiguration
* Note: If set to false, then no velocity values are sent (they are optional).
*/
bool use_velocity_outputs;

/**
* \brief Flag indicating if the interface should log data.
*/
Expand Down Expand Up @@ -223,12 +225,12 @@ struct TrajectoryConfiguration
base(base_configuration),
spline_method(Quintic)
{}

/**
* \brief The base configurations.
*/
BaseConfiguration base;

/**
* \brief Value specifying which spline method to use in the interpolation.
*/
Expand All @@ -238,4 +240,4 @@ struct TrajectoryConfiguration
} // end namespace egm
} // end namespace abb

#endif // EGM_COMMON_H
#endif // EGM_COMMON_H
22 changes: 11 additions & 11 deletions package.xml
Original file line number Diff line number Diff line change
@@ -1,22 +1,22 @@
<?xml version="1.0"?>
<package>
<?xml-model href="http://download.ros.org/schema/package_format3.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
<package format="3">
<name>abb_libegm</name>
<version>1.0.0</version>
<description>The abb_libegm package for easing the use of ABB Externally Guided Motion (EGM) product</description>

<description>A C++ library for interfacing with ABB robot controllers supporting Externally Guided Motion (689-1)</description>
<maintainer email="jon.tjerngren@se.abb.com">Jon Tjerngren</maintainer>

<license>BSD 3-Clause</license>

<author email="jon.tjerngren@se.abb.com">Jon Tjerngren</author>

<url type="repository">https://github.com/ros-industrial/abb_libegm</url>
<author>Jon Tjerngren</author>

<buildtool_depend>catkin</buildtool_depend>
<buildtool_depend>cmake</buildtool_depend>

<build_depend>boost</build_depend>
<depend>boost</depend>
<build_depend>protobuf-dev</build_depend>
<build_export_depend>protobuf-dev</build_export_depend>
<exec_depend>protobuf</exec_depend>

<run_depend>boost</run_depend>
<run_depend>protobuf</run_depend>
<export>
<build_type>cmake</build_type>
</export>
</package>