Skip to content

Conversation

ericriff
Copy link

@ericriff ericriff commented Sep 26, 2025

On this PR I've reworked how the 3rd party libraries are consumed to improve the conan support.

The rationale is to give the consumer / packager of this library the chance to opt-out of the vendored dependencies and use pre-compiled packages instead.

The changes here are made in such a way that using the vendored version of the libraries is the default, to preserve the current behavior.

Instead of exposing the whole 3rdparty folder as a include dir, I've added the required CMakeLists files to make each vendored dependency build as a standalone library, which is cleaner and also provides clean bundries between the vendored and non-vendored dependencies.

There are some aspects I haven't verified, like the generated cmake config files. I'm not really sure how to test those.
There is also some more cleaning up required around the ament_build.cmake and conan_build.cmake modules, particularly the export_btcpp_package macro.

…nan so we can avoid exposing the whole 3rdparty folder on target_include_directories

Since this can create some confusion around which headers are actually being included -- the ones from that folder or the ones from conan?
Also fixes the include dirs by using ${CMAKE_CURRENT_SOURCE_DIR} instead of "."
For whatever reason including zmq.hpp before zmq_addon.hpp (which does include zmq.hpp internally) breaks builds
@facontidavide
Copy link
Collaborator

thanks, but this completely breaks compilation in ROS, apparently. check CI

@ericriff
Copy link
Author

thanks, but this completely breaks compilation in ROS, apparently. check CI

Yes, I've made it a draft to see how it goes with CI. I'll take a look.

@uilianries
Copy link
Contributor

once again, if you feel that cpp-sqlite is a blocker, I can just reimplement something similar with 30 minutes of vibe-coding and remove this altogether from the repo... just saying

It would be great! But in case you want to use some more maintained and active project for sqlite cpp wrapper, I would say SQLiteCpp. In its README, it pointed a bunch of other SQLite wrappers too, including some that are header-only (if you prefer)

@ericriff ericriff force-pushed the unvendor-dependencies branch 2 times, most recently from 4e43e36 to c000115 Compare September 30, 2025 20:01
@ericriff ericriff force-pushed the unvendor-dependencies branch from c000115 to 083bbf3 Compare September 30, 2025 20:04
@ericriff
Copy link
Author

ericriff commented Sep 30, 2025

So I was under the impression that Lexy was not supported on conan ATM. Mainly because of https://github.com/conan-io/conan-center-index/blob/master/recipes/behaviortree.cpp/all/conanfile.py#L106 and because I looked for it as lexy. But it is available, just with a different name (foonathan-lexy). So in the last commit I added support to consume that package from conan as well.

@ericriff ericriff force-pushed the unvendor-dependencies branch 2 times, most recently from 285c088 to 4232154 Compare September 30, 2025 21:40
@ericriff ericriff force-pushed the unvendor-dependencies branch from 4232154 to 314bdd7 Compare September 30, 2025 21:45
@facontidavide
Copy link
Collaborator

@ericriff FYI: 58ed0ec

@ericriff
Copy link
Author

@facontidavide it looks like using cmake presets on the conan builds opened a can of worms.
Windows builds (I think) were using C++14 and now are actually using 17. This made a bunch of compile time errors pop up on src/xml_parsing.cpp. Silly things like using or instead of ||. But I do see some warnings on the CI builds. Could you take a look? I don't have a windows box.

@facontidavide
Copy link
Collaborator

facontidavide commented Sep 30, 2025

624b11b

I am going to create a new tagged release, 4.7.3

please refer to that one

@ericriff
Copy link
Author

Could you tag after this PR goes in? So we can create a new package in conan with all these packaging changes.

@facontidavide
Copy link
Collaborator

sorry for pulling the rug under your PR with my changes, but I prefer to remove these dependencies now, before we handle with them in conan

https://github.com/BehaviorTree/BehaviorTree.CPP/releases/tag/4.7.3

@facontidavide
Copy link
Collaborator

Windows builds (I think) were using C++14 and now are actually using 17. This made a bunch of compile time errors pop up on src/xml_parsing.cpp. Silly things like using or instead of ||. But I do see some warnings on the CI builds. Could you take a look? I don't have a windows box.

I will look into this. thanks

@ericriff
Copy link
Author

This is looking good now!

@facontidavide
Copy link
Collaborator

Silly question but how do I reproduce the issue you are talking about in CI?

I added naively set(CMAKE_CXX_STANDARD 17) but it seems to compile just fine

https://github.com/BehaviorTree/BehaviorTree.CPP/actions/runs/18145002973/job/51644676948?pr=1013

@ericriff
Copy link
Author

ericriff commented Sep 30, 2025

Revert these changes and the errors will pop up 314bdd7

They started showing up when I added f88b727

I had to do that because foonathan-lexy errors out if conan is set up to use c++14, and that's what conan profile detect was setting on windows. You can see it in the logs

Even with my fixed from above, I see some warnings on the logs. IIRC, when the tests run. They're not errors so CI passes.

@facontidavide
Copy link
Collaborator

pushed d8c1609

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants