-
Notifications
You must be signed in to change notification settings - Fork 773
Improve handling of dependencies #1012
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
Changes from all commits
1d5d2ad
48ac8d7
0604135
910c05f
d2cffb9
d1a030b
f19b357
4d88af4
5118048
7eda756
50c24ad
48d81ed
d145e46
b063794
76b22a6
d317e69
833570e
97f2776
0bafb1b
38e4d2d
af954f8
995890d
824c807
45afb6c
021d80a
c4f1680
8a47185
c869c77
0f5d249
37e9134
5297135
ad3b950
083bbf3
7a3cd99
13fdbde
7402040
f858b08
f88b727
314bdd7
7d40d96
5230419
81bc7e4
b21cb25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
find_package(ZeroMQ REQUIRED) | ||
|
||
add_library(cppzmq INTERFACE) | ||
|
||
# This library doesn't use modern targets unfortunately. | ||
#add_library(cppzmq::cppzmq ALIAS cppzmq) | ||
|
||
target_include_directories(cppzmq | ||
INTERFACE | ||
${CMAKE_CURRENT_SOURCE_DIR} | ||
) | ||
|
||
if(TARGET libzmq-static) | ||
target_link_libraries(cppzmq INTERFACE libzmq-static) | ||
elseif(TARGET libzmq) | ||
target_link_libraries(cppzmq INTERFACE libzmq) | ||
else() | ||
message(FATAL_ERROR "Unknown zeromq target name") | ||
endif() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
add_library(flatbuffers INTERFACE) | ||
|
||
add_library(flatbuffers::flatbuffers ALIAS flatbuffers) | ||
|
||
target_include_directories(flatbuffers | ||
INTERFACE | ||
${CMAKE_CURRENT_SOURCE_DIR} | ||
) |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file was moved into a #include <flatbuffers/base.h>` Otherwise it would have to be included like this #include <base.h>` Which doesn't match the conan package. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
add_library(minicoro INTERFACE) | ||
|
||
add_library(minicoro::minicoro ALIAS minicoro) | ||
|
||
target_include_directories(minicoro | ||
INTERFACE | ||
${CMAKE_CURRENT_SOURCE_DIR} | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
add_library(minitrace STATIC | ||
minitrace.cpp | ||
) | ||
|
||
add_library(minitrace::minitrace ALIAS minitrace) | ||
|
||
target_include_directories(minitrace | ||
PUBLIC | ||
${CMAKE_CURRENT_SOURCE_DIR} | ||
) | ||
|
||
target_compile_definitions(minitrace | ||
PRIVATE | ||
MTR_ENABLED=True | ||
) | ||
|
||
set_property(TARGET minitrace | ||
PROPERTY | ||
POSITION_INDEPENDENT_CODE ON | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
add_library(tinyxml2 STATIC | ||
tinyxml2.cpp | ||
) | ||
|
||
add_library(tinyxml2::tinyxml2 ALIAS tinyxml2) | ||
|
||
target_include_directories(tinyxml2 | ||
PUBLIC | ||
${CMAKE_CURRENT_SOURCE_DIR} | ||
) | ||
|
||
set_property(TARGET tinyxml2 | ||
PROPERTY | ||
POSITION_INDEPENDENT_CODE ON | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,17 @@ option(USE_AFLPLUSPLUS "Use AFL++ instead of libFuzzer" OFF) | |
option(ENABLE_DEBUG "Enable debug build with full symbols" OFF) | ||
option(FORCE_STATIC_LINKING "Force static linking of all dependencies" OFF) | ||
|
||
option(USE_VENDORED_CPPZMQ "Use the bundled version of cppzmq" ON) | ||
option(USE_VENDORED_FLATBUFFERS "Use the bundled version of flatbuffers" ON) | ||
option(USE_VENDORED_LEXY "Use the bundled version of lexy" ON) | ||
option(USE_VENDORED_MINICORO "Use the bundled version of minicoro" ON) | ||
option(USE_VENDORED_MINITRACE "Use the bundled version of minitrace" ON) | ||
option(USE_VENDORED_TINYXML2 "Use the bundled version of tinyxml2" ON) | ||
|
||
set(BTCPP_LIB_DESTINATION lib) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code was repeated on both |
||
set(BTCPP_INCLUDE_DESTINATION include) | ||
set(BTCPP_BIN_DESTINATION bin) | ||
|
||
set(BASE_FLAGS "") | ||
|
||
if(ENABLE_DEBUG) | ||
|
@@ -61,12 +72,6 @@ if(USE_V3_COMPATIBLE_NAMES) | |
add_definitions(-DUSE_BTCPP3_OLD_NAMES) | ||
endif() | ||
|
||
#---- Find other packages ---- | ||
find_package(Threads REQUIRED) | ||
|
||
|
||
set(BEHAVIOR_TREE_LIBRARY ${PROJECT_NAME}) | ||
ericriff marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# Update the policy setting to avoid an error when loading the ament_cmake package | ||
# at the current cmake version level | ||
if(POLICY CMP0057) | ||
|
@@ -84,19 +89,57 @@ if ( ament_cmake_FOUND ) | |
include(cmake/ament_build.cmake) | ||
else() | ||
message(STATUS "------------------------------------------") | ||
message(STATUS "BehaviorTree is being built with conan.") | ||
message(STATUS "BehaviorTree is being built without AMENT.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've changed this message to better reflect what's going on. This code path doesn't necesarily means that conan is being used, it just means that ament is not used. On the docs it says that one can build without conan or ament if ZeroMQ and Sqlite3 are provided somehow (e.g. apt on ubuntu). If you do that, then you fall on this code path and the log wrongly says that conan is being used. |
||
message(STATUS "------------------------------------------") | ||
include(cmake/conan_build.cmake) | ||
endif() | ||
|
||
############################################################# | ||
# LIBRARY | ||
# Handle dependencies | ||
|
||
find_package(Threads REQUIRED) | ||
|
||
if(BTCPP_GROOT_INTERFACE) | ||
if(USE_VENDORED_CPPZMQ) | ||
add_subdirectory(3rdparty/cppzmq) | ||
else() | ||
find_package(cppzmq REQUIRED) | ||
endif() | ||
endif() | ||
|
||
if(BTCPP_SQLITE_LOGGING) | ||
find_package(SQLite3 REQUIRED) | ||
endif() | ||
|
||
if(USE_VENDORED_FLATBUFFERS) | ||
add_subdirectory(3rdparty/flatbuffers) | ||
else() | ||
find_package(flatbuffers REQUIRED) | ||
endif() | ||
|
||
add_subdirectory(3rdparty/lexy) | ||
if(USE_VENDORED_LEXY) | ||
add_subdirectory(3rdparty/lexy) | ||
else() | ||
find_package(lexy REQUIRED) | ||
endif() | ||
|
||
add_library(minitrace STATIC 3rdparty/minitrace/minitrace.cpp) | ||
target_compile_definitions(minitrace PRIVATE MTR_ENABLED=True) | ||
set_property(TARGET minitrace PROPERTY POSITION_INDEPENDENT_CODE ON) | ||
if(USE_VENDORED_MINICORO) | ||
add_subdirectory(3rdparty/minicoro) | ||
else() | ||
find_package(minicoro REQUIRED) | ||
endif() | ||
|
||
if(USE_VENDORED_MINITRACE) | ||
add_subdirectory(3rdparty/minitrace) | ||
else() | ||
find_package(minitrace REQUIRED) | ||
endif() | ||
|
||
if(USE_VENDORED_TINYXML2) | ||
add_subdirectory(3rdparty/tinyxml2) | ||
else() | ||
find_package(tinyxml2 REQUIRED) | ||
endif() | ||
|
||
list(APPEND BT_SOURCE | ||
src/action_node.cpp | ||
|
@@ -140,8 +183,6 @@ list(APPEND BT_SOURCE | |
src/loggers/bt_file_logger_v2.cpp | ||
src/loggers/bt_minitrace_logger.cpp | ||
src/loggers/bt_observer.cpp | ||
|
||
3rdparty/tinyxml2/tinyxml2.cpp | ||
) | ||
|
||
|
||
|
@@ -179,8 +220,13 @@ target_link_libraries(${BTCPP_LIBRARY} | |
PRIVATE | ||
Threads::Threads | ||
${CMAKE_DL_LIBS} | ||
$<BUILD_INTERFACE:foonathan::lexy> | ||
$<BUILD_INTERFACE:minitrace> | ||
foonathan::lexy | ||
minitrace::minitrace | ||
tinyxml2::tinyxml2 | ||
minicoro::minicoro | ||
flatbuffers::flatbuffers | ||
$<$<BOOL:${BTCPP_GROOT_INTERFACE}>:cppzmq> | ||
$<$<BOOL:${BTCPP_SQLITE_LOGGING}>:SQLite::SQLite3> | ||
PUBLIC | ||
${BTCPP_EXTRA_LIBRARIES} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still need to figure out this line. I think it is always empty now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's for optional libraries like zeromq and sqlite3, but I would not touch it as the project may add new dependencies in the future and will need to rework it in case it changes now. |
||
) | ||
|
@@ -190,8 +236,6 @@ target_include_directories(${BTCPP_LIBRARY} | |
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include> | ||
$<INSTALL_INTERFACE:include> | ||
PRIVATE | ||
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/3rdparty> | ||
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/3rdparty/lexy/include> | ||
${BTCPP_EXTRA_INCLUDE_DIRS} | ||
) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,5 +63,13 @@ else (ZeroMQ_LIBRARIES AND ZeroMQ_INCLUDE_DIRS) | |
# show the ZeroMQ_INCLUDE_DIRS and ZeroMQ_LIBRARIES variables only in the advanced view | ||
mark_as_advanced(ZeroMQ_INCLUDE_DIRS ZeroMQ_LIBRARIES) | ||
|
||
if(ZeroMQ_FOUND AND NOT TARGET libzmq) | ||
add_library(libzmq UNKNOWN IMPORTED) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using variables like |
||
set_target_properties(libzmq PROPERTIES | ||
IMPORTED_LOCATION "${ZeroMQ_LIBRARY}" | ||
INTERFACE_INCLUDE_DIRECTORIES "${ZeroMQ_INCLUDE_DIRS}" | ||
) | ||
endif() | ||
|
||
endif (ZeroMQ_LIBRARIES AND ZeroMQ_INCLUDE_DIRS) | ||
endif(ZeroMQ_FOUND) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,24 +1,19 @@ | ||
list(APPEND CMAKE_PREFIX_PATH "${CMAKE_BINARY_DIR}") | ||
|
||
if(BTCPP_GROOT_INTERFACE) | ||
find_package(ZeroMQ REQUIRED) | ||
# find_package(ZeroMQ REQUIRED) | ||
list(APPEND BTCPP_EXTRA_LIBRARIES ${ZeroMQ_LIBRARIES}) | ||
list(APPEND BTCPP_EXTRA_INCLUDE_DIRS ${ZeroMQ_INCLUDE_DIRS}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need to clean this up. |
||
message(STATUS "ZeroMQ_LIBRARIES: ${ZeroMQ_LIBRARIES}") | ||
endif() | ||
|
||
if(BTCPP_SQLITE_LOGGING) | ||
find_package(SQLite3 REQUIRED) | ||
# find_package(SQLite3 REQUIRED) | ||
list(APPEND BTCPP_EXTRA_LIBRARIES ${SQLite3_LIBRARIES}) | ||
list(APPEND BTCPP_EXTRA_INCLUDE_DIRS ${SQLite3_INCLUDE_DIRS}) | ||
message(STATUS "SQLite3_LIBRARIES: ${SQLite3_LIBRARIES}") | ||
endif() | ||
|
||
|
||
set( BTCPP_LIB_DESTINATION lib ) | ||
set( BTCPP_INCLUDE_DESTINATION include ) | ||
set( BTCPP_BIN_DESTINATION bin ) | ||
|
||
mark_as_advanced( | ||
BTCPP_EXTRA_LIBRARIES | ||
BTCPP_LIB_DESTINATION | ||
|
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.
Did you create these CMake files only to provide an alias? Have you checked if the upstream really provides that target alias? Still, you can use Conan
CMakeDeps.set_property
to avoid any extra cmake file here.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 created these CMakeLists.txt to make each vendored library be a CMake target. Then I matched the target name that conan exposes.
Then you can use this pattern to select where
someLibrary
is coming fromBut regardless of the origin of the package (vendored, package manager) you link it the same way
target_link_library(btcpp PRIVATE someLibraryTargetName)
This also allowed me to remove an ugly line that was exposing the whole
3rdparty
folder as a include dir. it was something like thistarget_include_directory(btcpp PUBLIC 3rdparty)
This line was problematic because all the vendored headers where visible to btcpp, regardless of the value of
USE_VENDORED_someLibrary
. So if you opted out of some vendored lib and replaced it with a package, it was still not clear which version of the header was going to end up being included. The one from the package or the one vendored? It was a recipe for disaster.Besides, CMake best practices is to always use targets.