ami-iit / bipedal-locomotion-framework

Suite of libraries for achieving bipedal locomotion on humanoid robots
https://ami-iit.github.io/bipedal-locomotion-framework/
BSD 3-Clause "New" or "Revised" License
132 stars 36 forks source link

InstallBasicPackageFiles: Fix bug of OVERRIDE_MODULE_PATH that corrupt CMAKE_MODULE_PATH values set by blf transitive dependencies #827

Closed traversaro closed 3 months ago

traversaro commented 3 months ago

@davidegorbani in https://github.com/ami-iit/biomechanical-analysis-framework/pull/26 found a subtle bug that caused:

find_package(BipedalLocomotionFramework REQUIRED)
find_package(BipedalLocomotionFramework REQUIRED)

to fail, only when called two times.

The problem was related to the OVERRIDE_MODULE_PATH was not interacting nicely with with find_package that also added paths to CMAKE_MODULE_PATH . Why?

Basically the problem is that in its current implementation, OVERRIDE_MODULE_PATH saves the value of CMAKE_MODULE_PATH before all the dependencies are found via find_package, and then restores its value at the end, adding in the CMake config files that is called by find_package(BipedalLocomotionFramework) something like:

include(CMakeFindDependencyMacro)
set(CMAKE_MODULE_PATH_BK_BipedalLocomotionFramework ${CMAKE_MODULE_PATH})
set(CMAKE_MODULE_PATH ${PACKAGE_PREFIX_DIR}/share/BipedalLocomotionFramework/cmake)
find_package(iDynTree 3.0.0)
find_package(Eigen3 3.2.92)
find_package(spdlog 1.5.0)
find_package(YARP 3.7.0 COMPONENTS companion profiler dev os idl_tools)
find_package(casadi)
find_package(cppad)
find_package(manif 0.0.4)
find_package(matioCpp)
find_package(LieGroupControllers 0.2.0)
find_package(OpenCV)
find_package(PCL)
find_package(realsense2)
find_package(tomlplusplus 3.0.1)
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH_BK_BipedalLocomotionFramework})

The problem here is that find_package(YARP) transitively calls find_package(YCM), that appends some variables in the CMAKE_MODULE_PATH. However, this values are erased by the set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH_BK_BipedalLocomotionFramework}) call.

The second time find_package(BipedalLocomotionFramework) is called, YCM_FOUND is set to TRUE, so find_package(YCM) is not executing all its logic, but the CMAKE_MODULE_PATH does not contain the CMAKE_MODULE_PATH values required by YARP, and hence the failure experienced in https://github.com/ami-iit/biomechanical-analysis-framework/pull/26 .

This PR fixes this problem by just prepending the value specified in OVERRIDE_MODULE_PATH, and then removing the added items from the CMAKE_MODULE_PATH at the end of the find_package calls, in this way preserving any value that was added in the meanwhile.

To catch possible failures like this in the future, I modified cmake-package-check to always look for a package two times (https://github.com/ami-iit/cmake-package-check).

traversaro commented 3 months ago

To catch possible failures like this in the future, I modified cmake-package-check to always look for a package two times (https://github.com/ami-iit/cmake-package-check).

This was added in CI in https://github.com/ami-iit/bipedal-locomotion-framework/pull/827/commits/49622d20ce2e3cc94e2340ac3cb42a1f7253c99c .

traversaro commented 3 months ago

The logic is not working correctly, I see:

CMake Warning at /usr/local/lib/cmake/BipedalLocomotionFramework/BipedalLocomotionFrameworkConfig.cmake:31 (find_package):
  By not providing "Findcppad.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "cppad", but
  CMake did not find one.

  Could not find a package configuration file provided by "cppad" with any of
  the following names:

    cppadConfig.cmake
    cppad-config.cmake

  Add the installation prefix of "cppad" to CMAKE_PREFIX_PATH or set
  "cppad_DIR" to a directory containing one of the above files.  If "cppad"
  provides a separate development package or SDK, be sure it has been
  installed.
Call Stack (most recent call first):
S-Dafarra commented 3 months ago

Does it make sense to set a flag and simply avoid that logic from the second run?

traversaro commented 3 months ago

Does it make sense to set a flag and simply avoid that logic from the second run?

I think you would still find corner cases in which the system would not behave as expected. For example, currently:

find_package(BipedalLocomotionFramework REQUIRED)
find_package(YCM REQUIRED)

results in an emtpy CMAKE_MODULE_PATH (and hence the repo will not be able to find YCM modules), while:

find_package(YCM REQUIRED)
find_package(BipedalLocomotionFramework REQUIRED)

will work as expected.

traversaro commented 3 months ago

The logic is not working correctly, I see:

CMake Warning at /usr/local/lib/cmake/BipedalLocomotionFramework/BipedalLocomotionFrameworkConfig.cmake:31 (find_package):
  By not providing "Findcppad.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "cppad", but
  CMake did not find one.

  Could not find a package configuration file provided by "cppad" with any of
  the following names:

    cppadConfig.cmake
    cppad-config.cmake

  Add the installation prefix of "cppad" to CMAKE_PREFIX_PATH or set
  "cppad_DIR" to a directory containing one of the above files.  If "cppad"
  provides a separate development package or SDK, be sure it has been
  installed.
Call Stack (most recent call first):

Problem solved, just a typo and the fact tthat foreach() does not support IN LISTS if you are passing a literal list (as opposed to a variable list).

GiulioRomualdi commented 3 months ago

We currently have this failure

cd build
  cmake --install . --config Release
  cmake-package-check BipedalLocomotionFramework
  shell: /usr/bin/bash -l {0}
  env:
    INPUT_RUN_POST: true
    CONDA: /usr/share/miniconda[3](https://github.com/ami-iit/bipedal-locomotion-framework/actions/runs/8480631605/job/23236690685?pr=827#step:15:3)
    CONDA_PKGS_DIR: /home/runner/conda_pkgs_dir
CMake Error at src/ContinuousDynamicalSystem/cmake_install.cmake:52 (file):
  file INSTALL cannot copy file
  "/home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/build/lib/libBipedalLocomotionFrameworkContinuousDynamicalSystem.so.0.18.100"
-- Installing: /usr/local/lib/libBipedalLocomotionFrameworkContinuousDynamicalSystem.so.0.18.100
  to
  "/usr/local/lib/libBipedalLocomotionFrameworkContinuousDynamicalSystem.so.0.18.100":
  Permission denied.
Call Stack (most recent call first):
  src/cmake_install.cmake:[4](https://github.com/ami-iit/bipedal-locomotion-framework/actions/runs/8480631605/job/23236690685?pr=827#step:15:4)7 (include)
  cmake_install.cmake:52 (include)
traversaro commented 3 months ago

We currently have this failure

cd build
  cmake --install . --config Release
  cmake-package-check BipedalLocomotionFramework
  shell: /usr/bin/bash -l {0}
  env:
    INPUT_RUN_POST: true
    CONDA: /usr/share/miniconda[3](https://github.com/ami-iit/bipedal-locomotion-framework/actions/runs/8480631605/job/23236690685?pr=827#step:15:3)
    CONDA_PKGS_DIR: /home/runner/conda_pkgs_dir
CMake Error at src/ContinuousDynamicalSystem/cmake_install.cmake:52 (file):
  file INSTALL cannot copy file
  "/home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/build/lib/libBipedalLocomotionFrameworkContinuousDynamicalSystem.so.0.18.100"
-- Installing: /usr/local/lib/libBipedalLocomotionFrameworkContinuousDynamicalSystem.so.0.18.100
  to
  "/usr/local/lib/libBipedalLocomotionFrameworkContinuousDynamicalSystem.so.0.18.100":
  Permission denied.
Call Stack (most recent call first):
  src/cmake_install.cmake:[4](https://github.com/ami-iit/bipedal-locomotion-framework/actions/runs/8480631605/job/23236690685?pr=827#step:15:4)7 (include)
  cmake_install.cmake:52 (include)

Fixed by https://github.com/ami-iit/bipedal-locomotion-framework/pull/827/commits/0abd88c040dd4e511c9f9ee89dcc20dcf427a855 (if it works I will rebase). This will not be necessary if https://github.com/conda-forge/cmake-feedstock/pull/208 is merged.

traversaro commented 3 months ago

Homebrew failures are unrelated and addressed in https://github.com/ami-iit/bipedal-locomotion-framework/pull/822 , so I guess they can be ignored. There is a failure in conda-forge CI, but it seems related to some kind of network issue, that anyone persists even by restarting the CI.

GiulioRomualdi commented 3 months ago

Accordingly to https://github.com/ami-iit/bipedal-locomotion-framework/pull/827#issuecomment-2027364808, the PR can be merged, @traversaro do you agree?

GiulioRomualdi commented 3 months ago

Rebased on master, let's see

traversaro commented 3 months ago

There is a failure in conda-forge CI, but it seems related to some kind of network issue, that anyone persists even by restarting the CI.

xref:

Note that the problems seems mostly on the robostack-staging, not on conda-forge .

GiulioRomualdi commented 3 months ago

Rebased on master let's seen now

traversaro commented 3 months ago

Cool, now there is a legit error. On Windows on conda the cmake-package-check BipedalLocomotionFramework command fails with:

2024-04-05T10:37:36.7993177Z -- Found Python3: C:/hostedtoolcache/windows/Python/3.12.2/x64/python3.exe (found version "3.12.2") found components: Interpreter
2024-04-05T10:37:36.8585636Z Traceback (most recent call last):
2024-04-05T10:37:36.8586720Z   File "C:\Miniconda3\envs\test\Library\share\ament_cmake_core\cmake\package_templates\templates_2_cmake.py", line 21, in <module>
2024-04-05T10:37:36.8588457Z     from ament_package.templates import get_environment_hook_template_path
2024-04-05T10:37:36.8589122Z ModuleNotFoundError: No module named 'ament_package'
2024-04-05T10:37:36.8627884Z CMake Error at C:/Miniconda3/envs/test/Library/share/ament_cmake_core/cmake/ament_cmake_package_templates-extras.cmake:41 (message):
2024-04-05T10:37:36.8629421Z   execute_process(C:/hostedtoolcache/windows/Python/3.12.2/x64/python3.exe
2024-04-05T10:37:36.8630907Z   C:/Miniconda3/envs/test/Library/share/ament_cmake_core/cmake/package_templates/templates_2_cmake.py
2024-04-05T10:37:36.8632414Z   C:/Users/runneradmin/AppData/Local/Temp/tmpm7jsskty/ament_cmake_package_templates/templates.cmake)
2024-04-05T10:37:36.8633336Z   returned error code 1
2024-04-05T10:37:36.8634000Z Call Stack (most recent call first):
2024-04-05T10:37:36.8634707Z   C:/Miniconda3/envs/test/Library/share/ament_cmake_core/cmake/ament_cmake_coreConfig.cmake:41 (include)
2024-04-05T10:37:36.8635873Z   C:/Miniconda3/envs/test/Library/share/rclcpp/cmake/ament_cmake_export_include_directories-extras.cmake:8 (find_package)
2024-04-05T10:37:36.8636942Z   C:/Miniconda3/envs/test/Library/share/rclcpp/cmake/rclcppConfig.cmake:41 (include)
2024-04-05T10:37:36.8637916Z   C:/Miniconda3/envs/test/Library/cmake/BipedalLocomotionFrameworkConfig.cmake:28 (find_package)
2024-04-05T10:37:36.8638628Z   CMakeLists.txt:5 (find_package)

Because we are probably call find_package(rclcpp) in the CMake's BLF config file, and those somehow call Python (cps can't come soon enough to deal with this). Anyhow, that is some kind of ros package bug, let me look into that.

traversaro commented 3 months ago

Ok, the problem is the usual one of CMake that prefers to find a Python in the system (i.e. C:/hostedtoolcache/windows/Python/3.12.2/x64/python3.exe) instead of the one in the conda environment. I think it should be sufficient to use a recent enough policy in CMake to fix that, let's do this in cmake-package-check and see if it fixes the problem.

GiulioRomualdi commented 3 months ago

CI passing! Merging! Thank you @traversaro

traversaro commented 3 months ago

xref: https://github.com/robotology/robometry/issues/153