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

Convert package to a pure CMake package #285

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Convert package to a pure CMake package #285

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3025939
Convert package to a pure CMake package
cottsay Jul 3, 2019
380c4e4
compile with -fPIC and bump cmake version
moriarty Jun 14, 2023
c9da89d
cmake: Use cmake >= 3.5, add project setup.
leamas Sep 22, 2020
61da1e2
cmake: Add defined so-version and public header to lib.
leamas Sep 22, 2020
40d2950
cmake: option BUILD_SHARED_LIBS ON
moriarty Jun 15, 2023
48f14ce
cmake: CMAKE_POSITION_INDEPENDENT_CODE
moriarty Jun 15, 2023
7b7f62b
cmake minimum 3.16
moriarty Jun 16, 2023
2a76e1a
remove PROJ_SOVERSION variable
moriarty Jun 16, 2023
2528dcf
set project language to CXX
moriarty Jun 16, 2023
f60d49d
Depend on GTest via its imported targets
moriarty Jun 17, 2023
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
45 changes: 19 additions & 26 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,11 @@
cmake_minimum_required(VERSION 2.8.3)
cmake_minimum_required(VERSION 2.8.12)
project(serial)

# Find catkin
find_package(catkin REQUIRED)

if(APPLE)
find_library(IOKIT_LIBRARY IOKit)
find_library(FOUNDATION_LIBRARY Foundation)
endif()

if(UNIX AND NOT APPLE)
# If Linux, add rt and pthread
set(rt_LIBRARIES rt)
set(pthread_LIBRARIES pthread)
catkin_package(
LIBRARIES ${PROJECT_NAME}
INCLUDE_DIRS include
DEPENDS rt pthread
)
else()
# Otherwise normal call
catkin_package(
LIBRARIES ${PROJECT_NAME}
INCLUDE_DIRS include
)
endif()

## Sources
set(serial_SRCS
src/serial.cc
Expand All @@ -48,6 +28,9 @@ endif()

## Add serial library
add_library(${PROJECT_NAME} ${serial_SRCS})
Copy link

Choose a reason for hiding this comment

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

Suggested change
add_library(${PROJECT_NAME} ${serial_SRCS})
add_library(serial ${serial_SRCS})

It aids in readability and grepability if you simply spell out the variable name.

Copy link

Choose a reason for hiding this comment

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

👍 to this recommendation as it makes the cmake file much easier to read and understand. One of the hardest parts of cmake is reading the config files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree here, but I'll wait for @wjwwood to chime in on this PR in general.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am concerned this PR gets too big and goes the way of any of the other open PRs

Copy link
Owner

Choose a reason for hiding this comment

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

It's fine with me, but it deviates from what it common in the ROS ecosystem.

Copy link

Choose a reason for hiding this comment

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

It's fine with me, but it deviates from what it common in the ROS ecosystem.

Not only the ROS ecosystem, but the whole cmake community I would say. For that reason I think this could be left as-is

Copy link

Choose a reason for hiding this comment

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

@ChrisThrasher wrote

#285 (comment)

No. This should IMHO be done like in e1dabd8, taking into account whether to include v8stdint.h or not.

set_target_properties(${PROJECT_NAME} PROPERTIES
POSITION_INDEPENDENT_CODE ON)
Copy link

@traversaro traversaro Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Minor comment: this make it impossible to compile the library without -fPIC flag. An alternative that still defaults to enable -fPIC unless someone explicitly disable it is to add:

if (NOT DEFINED CMAKE_POSITION_INDEPENDENT_CODE)
  set(CMAKE_POSITION_INDEPENDENT_CODE ON)
endif()

before the line add_library(${PROJECT_NAME} ${serial_SRCS})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I agree this shouldn’t be merged as is with this it’s too invasive. I picked this out of the ros2 branch we’ve been using.

See comment above.

Copy link

Choose a reason for hiding this comment

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

If you build the library as a shared library you don't need this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I picked two commits from @leamas PR #231 (to set the so version) and then set BUILD_SHARED_LIBS ON.

Then tested that out of the box it works fine in a colcon workspace...
But if you do colcon build --cmake-args "-DBUILD_SHARED_LIBS=OFF" then you'll get back to ld errors

--- stderr: robotiq_driver                             
/usr/bin/ld: /home/alex/ros/h/robotiq/install/serial/lib/libserial.a(unix.cc.o): warning: relocation against `_ZTVN6serial15SerialExceptionE' in read-only section `.text._ZN6serial15SerialExceptionD2Ev[_ZN6serial15SerialExceptionD5Ev]'
/usr/bin/ld: /home/alex/ros/h/robotiq/install/serial/lib/libserial.a(serial.cc.o): relocation R_X86_64_PC32 against symbol `_ZTVN6serial6SerialE' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: bad value

If I additionally add the suggestion from @traversaro then it seems to work in both cases:
colcon build --cmake-args "-DBUILD_SHARED_LIBS=OFF" will build a ./install/serial/lib/libserial.a which can be used without linking errors

@wjwwood any opinion or preference on this?


if(APPLE)
target_link_libraries(${PROJECT_NAME} ${FOUNDATION_LIBRARY} ${IOKIT_LIBRARY})
elseif(UNIX)
Expand All @@ -66,16 +49,26 @@ include_directories(include)

## Install executable
install(TARGETS ${PROJECT_NAME}
ARCHIVE DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION}
LIBRARY DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION}
RUNTIME DESTINATION ${CATKIN_GLOBAL_BIN_DESTINATION}
ARCHIVE DESTINATION lib
LIBRARY DESTINATION lib
RUNTIME DESTINATION bin
)

## Install headers
install(FILES include/serial/serial.h include/serial/v8stdint.h
DESTINATION ${CATKIN_GLOBAL_INCLUDE_DESTINATION}/serial)
DESTINATION include/serial)

## Install CMake config
install(FILES cmake/serialConfig.cmake
DESTINATION share/serial/cmake)


## Install package.xml
install(FILES package.xml
DESTINATION share/serial)
Comment on lines +80 to +89
Copy link
Owner

Choose a reason for hiding this comment

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

This might be a good time to change to exported targets, and also consider putting the headers in a separate subfolder, which we've been doing in ROS 2 for a while now to promote better header isolation in a merged install space.

I.e. we'd put the headers in include/serial/serial and the exported include flags would be like -I/path/to/include/serial with the code still doing #include "serial/serial.h".


## Tests
if(CATKIN_ENABLE_TESTING)
include(CTest)
if(BUILD_TESTING)
add_subdirectory(tests)
endif()
4 changes: 2 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ test:
@mkdir -p build
cd build && cmake $(CMAKE_FLAGS) ..
ifneq ($(MAKE),)
cd build && $(MAKE) run_tests
cd build && $(MAKE) all test
else
cd build && make run_tests
cd build && make all test
endif
3 changes: 3 additions & 0 deletions cmake/serialConfig.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
get_filename_component(SERIAL_CMAKE_DIR "${CMAKE_CURRENT_LIST_FILE}" PATH)
set(SERIAL_INCLUDE_DIRS "${SERIAL_CMAKE_DIR}/../../../include")
find_library(SERIAL_LIBRARIES serial PATHS ${SERIAL_CMAKE_DIR}/../../../lib/serial)
Copy link

@ChrisThrasher ChrisThrasher Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If we're adding a config module, we should write a config module that exports a target instead of doing the old school approach of setting some variables that downstream projects have to use. That means we need to create an export set for the library target and install that export set then this file becomes a one liner that will look something like this:

include(${CMAKE_CURRENT_LIST_DIR}/serial-targets.cmake)

Copy link

Choose a reason for hiding this comment

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

@ChrisThrasher can you point to helpful documentation and examples on how to do this well?

Copy link

@ChrisThrasher ChrisThrasher Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/ChrisThrasher/argon/blob/9a2982c5471f06ca1d97f8d2d8a1aab3055ba456/CMakeLists.txt#L29-L42

I'm biased but this installation code I wrote I quite like. It's goes above and beyond to be as robust and idiomatic as possible. You don't need to copy everything it does but it should do a good job laying our the basic steps.

  1. Install all public headers. I do this by installing the entire include/ directory.
  2. Create export set. This happens to only include a single library target but may include more if need be. It uses variables from the GNUInstallDirs module to ensure all files are installed in platform-appropriate locations that can be easily modified by the user should they want to install the library in a different location. I include the package name and version in the install tree but you can omit that.
  3. Install said export set. This is where you can (and should) specify a namespace for all exported targets.
  4. Create a version config file. This ensures that users can ask for a specific version and that will be checked against the version that is installed.
  5. Install the config module and the version config file.

Copy link

Choose a reason for hiding this comment

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

instead of doing the old school approach of setting some variables that downstream projects have to use.

This is not old-school. The normal usage pattern would be something like below, assuming that the serial library lives in a subdirectory

include(libs/serial)
target_link_libraries(${PROJECT_NAME} PRIVATE wjserial::serial)

This would just require a one-liner in CMakeLists.txt defining a ALIAS target. It relies on cmake's support for transitive dependencies. Adding such a target would simplify documentation and overall usage a lot.

Copy link

@ChrisThrasher ChrisThrasher Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that's what I'm saying. We're in agreement. The variable-based approach is old school because targets are the modern way of doing things.

Copy link

Choose a reason for hiding this comment

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

It aids in readability and grepability if you simply spell out the variable name.

This is actually a personal preference... I actually agree on the personal level, but let's face it: the use of ${PROJECT_NAME} is common enough in the cmake community to leave in place.

Copy link

Choose a reason for hiding this comment

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

Don't mistake commonality for it being a good idea. Lots of bad practices are still popular but that should not be used as an argument in favor of perpetuating bad ideas.

9 changes: 8 additions & 1 deletion package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@
<author email="[email protected]">William Woodall</author>
<author email="[email protected]">John Harrison</author>

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

<!-- boost is only needed for test/proof_of_concepts which are not enabled by default -->
<!--test_depend>boost</test_depend-->
<test_depend>gtest</test_depend>

<export>
<build_type>cmake</build_type>
</export>
</package>
15 changes: 11 additions & 4 deletions tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
if(UNIX)
catkin_add_gtest(${PROJECT_NAME}-test unix_serial_tests.cc)
target_link_libraries(${PROJECT_NAME}-test ${PROJECT_NAME})
find_package(GTest REQUIRED)
include_directories(${GTEST_INCLUDE_DIRS})
moriarty marked this conversation as resolved.
Show resolved Hide resolved

add_executable(${PROJECT_NAME}-test unix_serial_tests.cc)
target_link_libraries(${PROJECT_NAME}-test ${PROJECT_NAME} ${GTEST_LIBRARIES})
if(NOT APPLE)
target_link_libraries(${PROJECT_NAME}-test util)
endif()
add_test("${PROJECT_NAME}-test-gtest" ${PROJECT_NAME}-test)

if(NOT APPLE) # these tests are unreliable on macOS
catkin_add_gtest(${PROJECT_NAME}-test-timer unit/unix_timer_tests.cc)
target_link_libraries(${PROJECT_NAME}-test-timer ${PROJECT_NAME})
add_executable(${PROJECT_NAME}-test-timer unit/unix_timer_tests.cc)
target_link_libraries(${PROJECT_NAME}-test-timer ${PROJECT_NAME} ${GTEST_LIBRARIES})
add_test("${PROJECT_NAME}-test-timer-gtest" ${PROJECT_NAME}-test-timer)
endif()
# Boost is only required for tests/proof_of_concepts which are not enabled by default
# find_package(Boost REQUIRED)
endif()