-
Notifications
You must be signed in to change notification settings - Fork 48
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 bipedal-locomotion-framework #526
Conversation
Thanks! At a first glance it seem nice, let's see what is missing. For adding a new components the documentation as outline by the steps in https://github.com/robotology/robotology-superbuild/blob/master/doc/developers-faqs.md#how-to-add-a-new-profile, but let's first see if CI passes, and then we can go forward. |
TAG master | ||
COMPONENT dynamics | ||
FOLDER robotology | ||
CMAKE_ARGS -DBUILD_TESTING:BOOL=OFF |
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.
Could it make sense to pass the options on enabling/disabling the dependencies explicitly?
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.
do you mean passing FRAMEWORK_USE_manif
etc? https://github.com/dic-iit/bipedal-locomotion-framework/blob/master/cmake/BipedalLocomotionFrameworkFindDependencies.cmake#L179
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.
|
||
include(YCMEPHelper) | ||
|
||
set(bipedal-locomotion-framework_DEPENDS "") |
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.
Probably in the CMake world we can stick to the CMake package name for consistency, so:
set(bipedal-locomotion-framework_DEPENDS "") | |
set(BipedalLocomotionFramework_DEPENDS "") |
@@ -161,9 +162,17 @@ if(ROBOTOLOGY_ENABLE_DYNAMICS) | |||
find_or_build_package(walking-controllers) | |||
find_or_build_package(whole-body-estimators) | |||
find_or_build_package(whole-body-controllers) | |||
find_or_build_package(bipedal-locomotion-framework) |
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.
The name here should match the one of the Build<package>.cmake
and the one passed to ycm_ep_helper
. Even if not strictly necessary, it is a good idea to just use the cmake package name (so in this case BipedalLocomotionFramework
), because find_or_build_package
mirrors find_package
, so if we have:
find_package(BipedalLocomotionFramework)
we should also have:
find_or_build_package(BipedalLocomotionFramework)
The casadi error on Debian Buster are one of those that make you feel alive:
However, Debian Buster is one of those platforms for which we keep compatibility mostly for the Core software to run on the pc104 of old robots, probably we do not need for now to support |
Anyhow, I just check and this is sweet: Debian Buster backports has indeed Ninja 1.10 (see https://packages.debian.org/buster-backports/ninja-build), so we just need to install it from the backports as we already to for CMake in https://packages.debian.org/buster-backports/ninja-build . However, if more problem emerge we can just avoid to test it on Debian Buster, even because now that we support conda we can easily compile our code on arbitrary old Linux distributions through it. |
I think we need to change the idyntree tag |
I will release iDynTree 2.0 soon in the next days, this should handle that. Instead we need to have a way to exclude some components from the release. |
I didn't get it |
The release of the robotology-superbuild is coherent group of tags for all the subprojects, that are continuously tested together. As BFL still do not have a release for the 2020.11 release of the superbuild, I would like to exclude it from the release 2020.11, while including in the "rolling release" version of the superbuild that we typically used in the lab. However it was not a request for a PR, more a note for me. |
Once this PR is approved: robotology/idyntree#778 , I will release iDynTree 2.0.0 and then we will have a bfl-compatible iDynTree on both 2020.11 release, Stable and Unstable branches. |
…ndencies - manif - qhull - cppad - casadi
9a798ea
to
9503fa9
Compare
As discussed in #517 (comment) some dependencies can be installed with brew/vcpkg. We now should decide what to do for them. For instance, |
|
I would disable for now the |
At the moment I would for the simplest solution, i.e. compile everywhere, we can complexify it once the first PR is landed. |
|
As the situation is quickly growing complex for the relative compatibility of options and platform support, I am preparing a matrix to clarify which profiles are a supported on which platform, I am adding it after #535 lands. |
As in any case we are not going to add tags for bfl in 2020.11, I think we can skip that profile for now in release CIs, for example adding after all the configure steps:
|
ops I did it in 082ec05 |
Ah cool, that's fine then, you can leave them, I did not think we wanted to do that for the moment, but if you did the effort we can leave them. |
24fb79b
to
d0056b0
Compare
I also added |
I think we need to add matio to the apt and brew dependencies in docs and CI, in vcpkg binary archive it is already available. |
Casadi failures on Linux:
|
It should be fixed in 91377a0 |
On windows is failing but I'm not able to find it. @traversaro could you help me? |
Everything is compiling!!! |
Thanks @GiulioRomualdi |
This is the first test for having bipedal-locomotion-framework in the superbuild.
I'm pretty sure I missed something
cc @traversaro @S-Dafarra