Improve handling of dependencies #1012
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| find_package(SQLite3 REQUIRED) | ||
|
|
||
| add_library(cpp-sqlite INTERFACE) | ||
|
|
||
| add_library(cpp-sqlite::cpp-sqlite ALIAS cpp-sqlite) | ||
|
|
||
| target_include_directories(cpp-sqlite | ||
| INTERFACE | ||
| ${CMAKE_CURRENT_SOURCE_DIR} | ||
| ) | ||
|
|
||
| target_link_libraries(cpp-sqlite | ||
| INTERFACE | ||
| SQLite::SQLite3 | ||
| ) |
| 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. 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 |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| add_library(wildcards INTERFACE) | ||
|
|
||
| add_library(wildcards::wildcards ALIAS wildcards) | ||
|
|
||
| target_include_directories(wildcards | ||
| INTERFACE | ||
| ${CMAKE_CURRENT_SOURCE_DIR} | ||
| ) |
|
There was a problem hiding this comment. I think this file can be deleted, it is not used. There was a problem hiding this comment. I also recommend removing this file, as conan.cmake is Conan 1.x only, but I would suggest doing it in a separate PR and not editing this file at all, as it's an external artifact. Please, read https://github.com/conan-io/cmake-conan for further reference. There was a problem hiding this comment. I edited it by mistake when I blanket-replaced all There was a problem hiding this comment. For the time being I've reverted these changes c869c77 |
|
There was a problem hiding this comment. I've replaced the basic conanfile.txt with the more powerful python variant, mainly to be able to set some CMake variables on the toolchain to opt-out of the vendored libraries when conan provides them. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,37 @@ | ||||||
| from conan import ConanFile | ||||||
| from conan.tools.cmake import CMakeToolchain, CMakeDeps | ||||||
ericriff marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
|
|
||||||
| class BehaviortreeCppConan(ConanFile): | ||||||
| name = "behaviortree.cpp" | ||||||
| settings = "os", "arch", "compiler", "build_type" | ||||||
|
|
||||||
| default_options = { | ||||||
| "flatbuffers/*:header_only": True, | ||||||
| } | ||||||
|
|
||||||
ericriff marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| def build_requirements(self): | ||||||
ericriff marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| self.test_requires("gtest/1.14.0") | ||||||
|
|
||||||
| def requirements(self): | ||||||
| self.requires("flatbuffers/24.12.23") | ||||||
| self.requires("minicoro/0.1.3") | ||||||
| self.requires("minitrace/cci.20230905") | ||||||
| self.requires("sqlite3/3.40.1") # This should be a transitive dependency of cpp-sqlite | ||||||
| self.requires("tinyxml2/10.0.0") | ||||||
| self.requires("zeromq/4.3.4") # This should be a transitive dependency of cppzmq | ||||||
|
|
||||||
| def generate(self): | ||||||
| tc = CMakeToolchain(self) | ||||||
|
|
||||||
| #tc.variables["USE_VENDORED_CPPSQLITE"] = False | ||||||
| #tc.variables["USE_VENDORED_CPPZMQ"] = False | ||||||
| tc.variables["USE_VENDORED_FLATBUFFERS"] = False | ||||||
|
||||||
| tc.variables["USE_VENDORED_FLATBUFFERS"] = False | |
| tc.cache_variable["USE_VENDORED_FLATBUFFERS"] = False |
Always prefer cache variables. Variables are listed in the toolchain file and are related to cmake configuration. In some cases, those variables are ignored due cmake evaluation order.
There was a problem hiding this comment.
So if I use cache variables they don't end up on the toolchain file and then they never reach the build. Where are they set if not on the toolchain? on the presets? if that's the case then I should rework the docs and the CICD to use presets instead, and that requires a fairly modern CMake. Right? Not sure if that's worth it on this PR.
Compiling with [conan](https://conan.io/):
Assuming that you are in the **root** directory of `BehaviorTree.CPP`:
mkdir build_release
conan install . -of build_release -s build_type=Release --build=missing
cmake -S . -B build_release -DCMAKE_TOOLCHAIN_FILE="build_release/build/Release/generators/conan_toolchain.cmake"
cmake --build build_release --parallelThere was a problem hiding this comment.
They will be exposed in the CMakePreset, which is the correct place to project definitions: https://docs.conan.io/2/examples/tools/cmake/cmake_toolchain/build_project_cmake_presets.html#examples-tools-cmake-toolchain-build-project-presets
There was a problem hiding this comment.
Cool. But that requires CMake 3.23+ (acording to the conan docs).
Are we good with bumping the minimun CMake version just for this @facontidavide ?
The project currently asks for 3.16
cmake_minimum_required(VERSION 3.16.3) # version on Ubuntu FocalThere was a problem hiding this comment.
EDIT:
Don't get mixed, Conan is just an alternative to manage dependencies. Conan using CMake presets may require 3.23, but the project does not.
People still can list cmake as a tool requirement, but I don't think worth pushing the project to use 3.23 only because of Conan.
Still, using CMake presets is the current recommendation in the book CMake Professional (Craig Scott). So you minimize possible problems when using cmake toolchain.
There was a problem hiding this comment.
I don't understand what you meant 🤔
There was a problem hiding this comment.
@ericriff Sorry, I was in a hurry when I tried to write. I just edited my message.
There was a problem hiding this comment.
OK so you're suggesting using CMake 3.23+ when we're building with conan? Like leaving a note on the readme?
Because including it as a tool_requires won't cut it, I believe.
Because I'd have to do something like
conan install . -s build_type=Release --build=missing
cmake --preset conan-release <----- This is the system CMake, conan's is not in PATH
Unless we also enable the builenv?
There was a problem hiding this comment.
Unless we also enable the builenv?
Yes, will need to use build env. But adding a note about using system cmake 2.23 with conan is also not bad.
In the future, Conan will have conan run command, which should make it more implicit. The feature is still under development, but you can take a look: conan-io/conan#18972
There was a problem hiding this comment.
OK CICD has a new enough CMake so this is not a problem. I've implemented presets on the last batch of commits. Took me a couple of tries to dial in the windows side since I'm only used to linux.
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_propertyto 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
someLibraryis 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
3rdpartyfolder 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.