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.

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

add option to conditionally build manual selector node #397

Merged
merged 2 commits into from
Jun 21, 2022

Conversation

alsora
Copy link
Contributor

@alsora alsora commented Jun 20, 2022

Currently, the code looks whether ncurses is found and if that's the case, it tries to build the manual selector node.
This PR adds an explicit option to enable/disable building this node.

Besides giving more control on the dependencies, this simplifies cross-compilation procedures for example when building this package in conan (see here https://github.com/conan-io/conan-center-index/blob/master/recipes/behaviortree.cpp/all/conanfile.py)

Conan requires that all dependencies that will be "searched" (via find_package) are listed as requirements in the conanfile and it does not play well with optional dependencies that are not controlled via CMake options.
In particular, without my change, the code implicitly required the cross-compilation of ncurses.

  • in order to specify ncurses as a dependency in the conanfile it is required to cross-compile it.
  • if a user didn't specify ncurses as a dependency, find_package(Curses QUIET) could still find an x86_64 version if this was present in the OS (e.g. ubuntu desktop) thus linking it with the library and obviously failing the compilation as the library format was incompatible.

Signed-off-by: Alberto Soragna alberto.soragna@gmail.com

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@facontidavide
Copy link
Collaborator

I like the idea, but this PR breaks compilation on those platforms that don't have ncurse installed.
As you can see, the Window CI is failing

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
@facontidavide facontidavide merged commit aa375b7 into BehaviorTree:master Jun 21, 2022
@facontidavide
Copy link
Collaborator

thanks

@alsora alsora deleted the asoragna/option-ncurses branch June 21, 2022 17:20
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.

None yet

2 participants