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 all 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
66 changes: 40 additions & 26 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,18 @@
cmake_minimum_required(VERSION 2.8.3)
project(serial)
cmake_minimum_required(VERSION 3.16)

# Find catkin
find_package(catkin REQUIRED)
# General setup
project(serial
VERSION 1.2.1
LANGUAGES CXX
DESCRIPTION "Cross-platform, Serial Port library written in C++"
HOMEPAGE_URL "http://wjwwood.io/serial/"
)

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 @@ -47,7 +34,23 @@ else()
endif()

## Add serial library
set(serial_HEADERS
include/serial/serial.h
include/serial/v8stdint.h
)
Comment on lines +37 to +40
Copy link

Choose a reason for hiding this comment

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

Suggested change
set(serial_HEADERS
include/serial/serial.h
include/serial/v8stdint.h
)

You don't need to specify headers as source files if you don't want to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cmake: Add defined so-version and public header to lib.
Adding a so-version means defining an ABI level. This level is decoupled
from the ordinary version, even a major version change doesn't
necessarily mean that the so-version should change (and thus have all
dependencies to be rebuilt).

Adding the public header to clarify the setup.

Gbp-Pq: Name 0002-cmake-Add-defined-so-version-and-public-header-to-li.patch

This was added here ed0e389 which I cherry-picked 61da1e2

Copy link

Choose a reason for hiding this comment

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

v8stdint.h is actually problematic. It is now long time since I worked with this, but IIRC I think the situation is:

  • v8stdint.h is basically a band-aid for systems not having stdint.h available, notably windows.
  • The definitions in v8stdint.h clashes with other files on some systems.

That is, the correct solution would be to use cmake to probe for stdint.h and only include v8stdint.h if it's required. Needs to be double-checked, though. If it is indeed required anyway, cmake should probe for v8stdint.h and only include it if it's not available.

On Fedora, v8stdint.h is part of the v8 package.

Copy link

Choose a reason for hiding this comment

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

Of course, this is not only about including v8stdint.h or not in the package. It is also about conditionalizing the source files so they include the correct file, stdint.h or v8stdint.h. I made a quick hack for this in the Debian package, but it probably needs polish before being merged into the upstream package here.

Copy link

Choose a reason for hiding this comment

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

You don't need to specify headers as source files if you don't want to.

This is IMHO the wrong way to go. Recommended cmake best practice is certainly to include all used files in the project.

Copy link

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

I have seen a numbner of cmake projects, none of without the headers in the source lists. In this case, I think that you need to come up with some trustworthy source recommending not to include the headers.

You have already started the list of functionality which breaks without a complete list of source files, including headers. I can assure you there is more, and that we currently not use it is not much of an argument.

BTW: how would you install headers not being part of the sources?

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.

I think that you need to come up with some trustworthy source recommending not to include the headers.

I'd argue the burden of proof is on the person who wants to write more code.

You have already started the list of functionality which breaks without a complete list of source files

Nothing breaks for Visual Studio users. It's merely an inconvenience. Visual Studio users will also ask you use source_groups in spite of that not being strictly necessary.

how would you install headers not being part of the sources?

Same way as always (prior to FILE_SETs). You install the include directory or something similar. Adding headers as source files does not affect installation.

Copy link

Choose a reason for hiding this comment

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

I simply don't have time for this, sorry. It's up to the maintainer to judge.

Copy link
Owner

Choose a reason for hiding this comment

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

My experience has been that adding headers is not required at all. It does help some editors who want to list "files used in targets" without involving a compiler, but that's all. Whether or not it's recommended by CMake, I'm not sure about.

That being said, I don't care if they are included in this way. If it helps someone and they contribute it then I think it's fine.

Copy link

Choose a reason for hiding this comment

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

Fair enough. But, as you said, cstdint.h is the better alternative to v8stdint.h making this to be serial.h only

# Build, link and install main library
if(NOT DEFINED CMAKE_POSITION_INDEPENDENT_CODE)
set(CMAKE_POSITION_INDEPENDENT_CODE ON)
endif()
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
VERSION ${PROJECT_VERSION}
SOVERSION 1
PUBLIC_HEADER "${serial_HEADERS}"
)

target_include_directories(${PROJECT_NAME} PUBLIC include)

if(APPLE)
target_link_libraries(${PROJECT_NAME} ${FOUNDATION_LIBRARY} ${IOKIT_LIBRARY})
elseif(UNIX)
Expand All @@ -66,16 +69,27 @@ 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
PUBLIC_HEADER DESTINATION include/${PROJECT_NAME}
)

## 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="wjwwood@gmail.com">William Woodall</author>
<author email="ash.gti@gmail.com">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>
14 changes: 10 additions & 4 deletions tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
if(UNIX)
catkin_add_gtest(${PROJECT_NAME}-test unix_serial_tests.cc)
target_link_libraries(${PROJECT_NAME}-test ${PROJECT_NAME})
find_package(GTest REQUIRED)

add_executable(${PROJECT_NAME}-test unix_serial_tests.cc)
target_link_libraries(${PROJECT_NAME}-test ${PROJECT_NAME} GTest::GTest)
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::GTest)
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()