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.

Sign up for GitHub

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

Jump to bottom

Plain cmake #63

Merged
merged 19 commits into from
Jan 22, 2020
Merged

Plain cmake #63

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
3394f0a
Updated package.xml
jontje Nov 8, 2019
0a3368a
Added visibility control header
jontje Nov 8, 2019
4ad7d8b
Added cmake config input files
jontje Nov 8, 2019
d70c144
Updated CMakeList.txt file for Ubuntu
jontje Nov 8, 2019
b65bc9d
Updated CMakeList.txt file for Windows
jontje Nov 11, 2019
4ff987f
Included visibility control
jontje Nov 11, 2019
1576b74
travis: add Dashing build
gavanderhoorn Nov 14, 2019
e79f08b
travis: use master branch for ROS2 support
gavanderhoorn Nov 14, 2019
53e9e28
travis: NOT_TEST_INSTALL is not needed any more.
gavanderhoorn Nov 15, 2019
de25ff0
Improved and cleaned-up CMakeLists.txt
jontje Nov 15, 2019
43a88c7
Updated visibility control
jontje Nov 15, 2019
e4c3ce2
Update CMakeLists.txt
jontje Nov 18, 2019
51d95a0
Added alias and reworked exporting thread dependency
jontje Nov 18, 2019
e3ab8ff
Update CMakeLists.txt
jontje Nov 21, 2019
3aba401
Update CMakeLists.txt
jontje Nov 21, 2019
6e3eae4
Update CMakeLists.txt
jontje Nov 21, 2019
7325505
Update package.xml
jontje Nov 21, 2019
a78ac54
Address review comments
jontje Nov 21, 2019
3ad2428
Use GenerateExportHeader instead of custom implementation
jontje Nov 25, 2019
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
7 changes: 4 additions & 3 deletions .travis.yml
View file
Open in desktop
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="dashing" ROS_REPO=ros NOT_TEST_INSTALL=true

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
151 changes: 105 additions & 46 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,52 +1,55 @@
cmake_minimum_required(VERSION 2.8.12)
cmake_minimum_required(VERSION 3.6)
project(abb_libegm)

find_package(catkin REQUIRED)
set(${PROJECT_NAME}_MAJOR_VERSION 1)
set(${PROJECT_NAME}_MINOR_VERSION 0)
set(${PROJECT_NAME}_PATCH_VERSION 0)
set(${PROJECT_NAME}_VERSION ${${PROJECT_NAME}_MAJOR_VERSION}.${${PROJECT_NAME}_MINOR_VERSION}.${${PROJECT_NAME}_PATCH_VERSION})
gavanderhoorn marked this conversation as resolved.
Show resolved Hide resolved

find_package(Boost REQUIRED COMPONENTS system thread)
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()

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

#############################
## Google Protocol Buffers ##
#############################
find_package(Protobuf REQUIRED)
if(WIN32)
# The protobuf compiler appears to be default installed at ${_CMAKE_INSTALL_DIR}/tools/protobuf,
Copy link
Contributor

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.

# and find_package() doesn't always seem to find it without specifying a hint.
find_package(Protobuf REQUIRED HINTS ${_CMAKE_INSTALL_DIR}/tools/protobuf)
else()
find_package(Protobuf REQUIRED)
endif()

# 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")
# Work around Protobuf exporting 'lpthread' as a library: we
# export the dependency on pthread using the configuration files.
list(REMOVE_ITEM Protobuf_LIBRARIES "-lpthread")

# 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
)

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

include_directories(include
${CMAKE_CURRENT_BINARY_DIR} # Contains protobuf generated sources
${Boost_INCLUDE_DIRS}
${PROTOBUF_INCLUDE_DIRS}
${catkin_INCLUDE_DIRS})
add_library(${PROJECT_NAME} SHARED ${SRC_FILES})
Copy link
Contributor

@traversaro traversaro Nov 13, 2019 •
edited
Loading

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

target_include_directories(${PROJECT_NAME} PUBLIC
"$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include;${CMAKE_CURRENT_BINARY_DIR}>"
$<INSTALL_INTERFACE:include>
jontje marked this conversation as resolved.
Show resolved Hide resolved
PUBLIC ${Boost_INCLUDE_DIRS}
gavanderhoorn marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@gavanderhoorn gavanderhoorn Nov 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The FindBoost.cmake from CMake 3.5 actually seems to export Boost:: namespaced exported targets.

We should be able to just target_link_libraries(.. Boost::system etc) instead of using Boost_LIBRARIES and Boost_INCLUDE_DIRS.

PUBLIC ${Protobuf_INCLUDE_DIRS}
)
target_link_libraries(${PROJECT_NAME}
Copy link
Contributor

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 .

Copy link
Contributor Author

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?

Copy link
Contributor

@traversaro traversaro Nov 14, 2019 •
edited
Loading

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.

${Boost_LIBRARIES}
${Protobuf_LIBRARIES}
)

gavanderhoorn marked this conversation as resolved.
Show resolved Hide resolved
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})
# Causes the visibility macros to use dllexport rather than dllimport,
# which is appropriate when building the dll but not consuming it.
target_compile_definitions(${PROJECT_NAME} PRIVATE "ABB_LIBEGM_BUILDING_LIBRARY")

#############
## 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}
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/${PROJECT_NAME}
)

install(FILES ${EgmProtoHeaders}
DESTINATION ${CATKIN_PACKAGE_INCLUDE_DESTINATION})
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 ${EgmProtoFiles}
DESTINATION ${CATKIN_PACKAGE_INCLUDE_DESTINATION})
# Export targets.
set(export_targets ${export_targets};${PROJECT_NAME})
export(
EXPORT export_${PROJECT_NAME}
FILE "${PROJECT_BINARY_DIR}/export_${PROJECT_NAME}.cmake"
)

# Create the ${PROJECT_NAME}Config.cmake.
set(CONF_INCLUDE_DIRS "${CMAKE_INSTALL_PREFIX}/include")
Copy link
Contributor

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.

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.
configure_file(
${CMAKE_CURRENT_SOURCE_DIR}/cmake/${PROJECT_NAME}ConfigVersion.cmake.in
"${PROJECT_BINARY_DIR}/${PROJECT_NAME}ConfigVersion.cmake" @ONLY
)

# Create the ${PROJECT_NAME}-extras.cmake.
configure_file(
${CMAKE_CURRENT_SOURCE_DIR}/cmake/${PROJECT_NAME}-extras.cmake.in
"${PROJECT_BINARY_DIR}/${CMAKE_FILES_DIRECTORY}/${PROJECT_NAME}-extras.cmake" @ONLY
)

install(
FILES
"${PROJECT_BINARY_DIR}/${CMAKE_FILES_DIRECTORY}/${PROJECT_NAME}Config.cmake"
"${PROJECT_BINARY_DIR}/${PROJECT_NAME}ConfigVersion.cmake"
"${PROJECT_BINARY_DIR}/${CMAKE_FILES_DIRECTORY}/${PROJECT_NAME}-extras.cmake"
DESTINATION "share/${PROJECT_NAME}/cmake"
jontje marked this conversation as resolved.
Show resolved Hide resolved
COMPONENT dev
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this? Or is this a cargo-cult copy-pasta?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not needed, I guess, it originates from the ROS2 cmake template.

)

install(
EXPORT export_${PROJECT_NAME}
DESTINATION "share/${PROJECT_NAME}/cmake"
jontje marked this conversation as resolved.
Show resolved Hide resolved
FILE export_${PROJECT_NAME}.cmake
COMPONENT dev
Copy link
Member

Choose a reason for hiding this comment

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

Needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, from the ROS2 cmake template. I will remove it.

)
16 changes: 16 additions & 0 deletions cmake/abb_libegmConfig.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# - Config file for the abb_libegm package
# It defines the following variables
# abb_libegm_INCLUDE_DIRS - include directories for FooBar
# abb_libegm_LIBRARIES - libraries to link against
# abb_libegm_EXECUTABLE - the bar executable
gavanderhoorn marked this conversation as resolved.
Show resolved Hide resolved

set(abb_libegm_INCLUDE_DIRS "@CONF_INCLUDE_DIRS@")
Copy link
Contributor

@traversaro traversaro Nov 13, 2019 •
edited
Loading

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.


# Our library dependencies (contains definitions for IMPORTED targets)
include("${abb_libegm_DIR}/export_abb_libegm.cmake")
Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

See https://cmake.org/cmake/help/v3.3/manual/cmake-packages.7.html#creating-a-package-configuration-file .

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 see, thanks for the pointer!


# These are IMPORTED targets created by abb_libegmTargets.cmake
set(abb_libegm_LIBRARIES abb_libegm)

# Extras for our library (exports the dependency on pthread)
include("${abb_libegm_DIR}/abb_libegm-extras.cmake")
11 changes: 11 additions & 0 deletions cmake/abb_libegmConfigVersion.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
set(PACKAGE_VERSION "@abb_libegm_VERSION@")

# Check whether the requested PACKAGE_FIND_VERSION is compatible
if("${PACKAGE_VERSION}" VERSION_LESS "${PACKAGE_FIND_VERSION}")
set(PACKAGE_VERSION_COMPATIBLE FALSE)
else()
set(PACKAGE_VERSION_COMPATIBLE TRUE)
if ("${PACKAGE_VERSION}" VERSION_EQUAL "${PACKAGE_FIND_VERSION}")
set(PACKAGE_VERSION_EXACT TRUE)
endif()
endif()
20 changes: 11 additions & 9 deletions include/abb_libegm/egm_common.h
View file
Open in desktop
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 "visibility_control.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_PUBLIC 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_PUBLIC 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
35 changes: 35 additions & 0 deletions include/abb_libegm/visibility_control.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#ifndef ABB_LIBEGM__VISIBILITY_CONTROL_H_
#define ABB_LIBEGM__VISIBILITY_CONTROL_H_

// This logic was borrowed (then namespaced) from the examples on the gcc wiki:
// https://gcc.gnu.org/wiki/Visibility

#if defined _WIN32 || defined __CYGWIN__
#ifdef __GNUC__
#define ABB_LIBEGM_EXPORT __attribute__ ((dllexport))
#define ABB_LIBEGM_IMPORT __attribute__ ((dllimport))
#else
#define ABB_LIBEGM_EXPORT __declspec(dllexport)
#define ABB_LIBEGM_IMPORT __declspec(dllimport)
#endif
#ifdef ABB_LIBEGM_BUILDING_LIBRARY
#define ABB_LIBEGM_PUBLIC ABB_LIBEGM_EXPORT
#else
#define ABB_LIBEGM_PUBLIC ABB_LIBEGM_IMPORT
#endif
#define ABB_LIBEGM_PUBLIC_TYPE ABB_LIBEGM_PUBLIC
#define ABB_LIBEGM_LOCAL
#else
#define ABB_LIBEGM_EXPORT __attribute__ ((visibility("default")))
#define ABB_LIBEGM_IMPORT
#if __GNUC__ >= 4
#define ABB_LIBEGM_PUBLIC __attribute__ ((visibility("default")))
#define ABB_LIBEGM_LOCAL __attribute__ ((visibility("hidden")))
#else
#define ABB_LIBEGM_PUBLIC
#define ABB_LIBEGM_LOCAL
#endif
#define ABB_LIBEGM_PUBLIC_TYPE
#endif

#endif // ABB_LIBEGM__VISIBILITY_CONTROL_H_
Copy link
Contributor

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.

Copy link
Contributor Author

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!

Copy link
Contributor

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!

22 changes: 10 additions & 12 deletions package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,20 @@
<?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>Package for easing the use of ABB's Externally Guided Motion (EGM) product</description>
jontje marked this conversation as resolved.
Show resolved Hide resolved
<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 email="jon.tjerngren@se.abb.com">Jon Tjerngren</author>
Copy link
Member

Choose a reason for hiding this comment

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

The maintainer already has your email address. We typicaly only list it there. If you ever stop maintaining, it's convenient to just have to remove the maintainer tag, leave the author one and everything is immediately up-to-date (as opposed to having to edit/remove the email address in two places).


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

<build_depend>boost</build_depend>
<build_depend>protobuf-dev</build_depend>
<depend>boost</depend>
<depend>protobuf-dev</depend>
Copy link
Member

Choose a reason for hiding this comment

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

If I interpret the rosdep entries for protobuf correctly, I believe protobuf-dev would only be a build_depend and build_export_depend, and protobuf would still be the exec_depend.

The former resolves to the development package (providing the headers and compiler), while the latter contains just the library.

Headers are only needed for building things. The library is always needed.

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 took a peek at how Google Cartographer's package.xml looked like for a newer package format and assumed it was the "appropriate" way.

But I think your way is more reasonable.

Copy link
Member

Choose a reason for hiding this comment

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

Well .. it is Google, but I'm not sure that is correct.

As I wrote: if I interpret things correctly.


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