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

Looking for c++17 for python bindings #635

Merged
merged 2 commits into from
Mar 28, 2023
Merged

Conversation

@GiulioRomualdi
Copy link
Member Author

Didn't solve the CI failure 😭

2023-03-27T08:37:04.7308490Z [ 75%] Building CXX object bindings/python/CMakeFiles/pybind11_blf.dir/bipedal_locomotion_framework.cpp.o
2023-03-27T08:37:07.2958342Z [ 76%] Building CXX object bindings/python/CMakeFiles/pybind11_blf.dir/System/src/Advanceable.cpp.o
2023-03-27T08:37:08.8024829Z In file included from /home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/src/ParametersHandler/include/BipedalLocomotion/ParametersHandler/IParametersHandler.h:16,
2023-03-27T08:37:08.8025924Z                  from /home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/src/System/include/BipedalLocomotion/System/Advanceable.h:13,
2023-03-27T08:37:08.8026941Z                  from /home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/bindings/python/System/include/BipedalLocomotion/bindings/System/Advanceable.h:17,
2023-03-27T08:37:08.8027878Z                  from /home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/bindings/python/System/src/Advanceable.cpp:13:
2023-03-27T08:37:08.8029547Z /home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/src/GenericContainer/include/BipedalLocomotion/GenericContainer/TemplateHelpers.h:212:43: error: expected template-name before ‘<’ token
2023-03-27T08:37:08.8046238Z   212 | struct is_string : public std::disjunction<std::is_same<char*, typename std::decay<T>::type>,
2023-03-27T08:37:08.8047296Z       |                                           ^
2023-03-27T08:37:08.8048334Z /home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/src/GenericContainer/include/BipedalLocomotion/GenericContainer/TemplateHelpers.h:212:43: error: expected ‘{’ before ‘<’ token
2023-03-27T08:37:08.8049958Z /home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/src/GenericContainer/include/BipedalLocomotion/GenericContainer/TemplateHelpers.h:224:16: error: ‘string_view’ in namespace ‘std’ does not name a type
2023-03-27T08:37:08.8053034Z   224 | constexpr std::string_view
2023-03-27T08:37:08.8054150Z       |                ^~~~~~~~~~~

@GiulioRomualdi
Copy link
Member Author

According to the error, there may be something wrong with these lines. However, no one touched the code for a while 🤔

/**
* is_string is a utility metafunction to detect the if a given type is a std::string or it can be
* trivially converted in a std::string.
*/
template <typename T>
struct is_string : public std::disjunction<std::is_same<char*, typename std::decay<T>::type>,
std::is_same<const char*, typename std::decay<T>::type>,
std::is_same<std::string, typename std::decay<T>::type>>
{
};

@traversaro
Copy link
Collaborator

traversaro commented Mar 27, 2023

Last good job: https://github.com/ami-iit/bipedal-locomotion-framework/actions/runs/4496370514 , GitHub Image Version 20230313.1
First failing (for this problem) job: https://github.com/ami-iit/bipedal-locomotion-framework/actions/runs/4507105321, GitHub Image Version: 20230317.1

So I guess it could be an image regression, see actions/runner-images#7310 . I do not see anything compiler related, so it could be a CMake 3.26.0 regression?

@traversaro
Copy link
Collaborator

To avoid this kind of regression due to GitHub images, for Linux jobs we could use directly vanilla docker images, so to avoid this problems. See https://github.com/robotology/robotology-superbuild/blob/2e991a2573bea720609ead2361c9eca83e68b7c5/.github/workflows/ci.yml#L160 for such an example.

@traversaro
Copy link
Collaborator

traversaro commented Mar 27, 2023

Just to check, if we could try to install the older CMake 3.25.3 that was working fine with the workaround suggested in actions/runner-images#7336 (comment), i.e.:

curl -sL https://github.com/Kitware/CMake/releases/download/v3.25.3/cmake-3.25.3-linux-x86_64.sh -o cmakeinstall.sh \
&& chmod +x cmakeinstall.sh \
&& ./cmakeinstall.sh --prefix=/usr/local --exclude-subdir \
&& rm cmakeinstall.sh

@traversaro
Copy link
Collaborator

I would try 3.25.3, as perhaps the regression is present in both 3.26.0 and 3.26.1 .

@GiulioRomualdi
Copy link
Member Author

Asking for Cmake 3.25.3 fixes the issue. Now macOS fails with a problem related to osqp-eigen (it seems it is not able to find a feasible solution).

The same problem is also solved in linux and windows. There are no issues there.

Here is the failure: https://github.com/ami-iit/bipedal-locomotion-framework/actions/runs/4532346253/jobs/7983612235?pr=635#step:24:138

The fact that it fails only in macOS-release scared me. However, we know that osqp is not deterministic. Indeed from https://github.com/google/osqp-cpp#faq

  • Is OSQP deterministic?
    • No, not in its default configuration. Section 5.2 of the OSQP
      paper describes that the update rule
      for the step size rho depends on the ratio between the runtime of the
      iterations and the runtime of the numerical factorization. Setting
      adaptive_rho to false disables this update rule and makes OSQP
      deterministic, but this could significantly slow down OSQP's
      convergence.

@traversaro
Copy link
Collaborator

traversaro commented Mar 27, 2023

You know what I think about macOS Homebrew: especially on a project like blf, probably we can just provide support on macOS via conda and drop homebrew CI. : )

@GiulioRomualdi GiulioRomualdi force-pushed the GiulioRomualdi-patch-1 branch from 66973ea to d8f24b4 Compare March 28, 2023 08:23
@GiulioRomualdi
Copy link
Member Author

Since it fixes ubuntu I will merge the PR. Regarding macOS probably @traversaro is right 😭

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

Successfully merging this pull request may close these issues.

3 participants