roboticslab-uc3m / questions-and-answers

A place for general debate and question&answer
https://robots.uc3m.es/developer-manual/appendix/repository-index.html
2 stars 0 forks source link

Decouple external build configuration (Travis CI/YCM) from local settings #18

Closed PeterBowman closed 3 years ago

PeterBowman commented 7 years ago

I've set up CI builds for asibot-hmi with this configuration; note that this project depends on kinematics-dynamics, hence an additional git clone step. To avoid building KDL, which is a dependency for kinematics-dynamics itself, I had to configure it appropriately:

cmake .. -DENABLE_TrajectoryLib:BOOL=OFF \
         -DENABLE_KdlVectorConverterLib:BOOL=OFF \
         -DENABLE_BasicCartesianControl:BOOL=OFF \
         -DENABLE_CartesianControlServer:BOOL=OFF \
         -DENABLE_CartesianControlClient:BOOL=OFF \
         -DENABLE_KdlSolver:BOOL=OFF \
         -DENABLE_BasicTwoLimbCartesianControl:BOOL=OFF \
         -DENABLE_TwoLimbCartesianControlServer:BOOL=OFF \
         -DENABLE_tests:BOOL=OFF

First remark: any options irrelevant to my project could've been set to OFF/FALSE in kinematics-dynamics' root CMakeLists.txt file. Therefore, I'd just concentrate on the funcionalities I'm really interested in, not the other way around.

Then, I configured periodic cron jobs targeted at discovering issues linked with the development of YARP. However, my latest build errored because of recent changes to kinematics-dynamics (CI logs), which happened to introduce a new dependency on KDL. This forced me to take a look upstream and update my .travis.yml accordingly: roboticslab-uc3m/asibot-hmi@9b1959d.

As a second remark, one must always keep an eye on each upstream repo because of new dependencies or changes in CMake configuration. This and the previous remark could be easily extended to YCM-like repos (e.g. teo-main) for the same reasons.

Proposal:

Regarding the last point and YCM projects, the CONFIGURE_COMMAND parameter to ycm_ep_helper should come in handy (docs).

PeterBowman commented 7 years ago

In order to alleviate configuration efforts (particularly with installation guides in mind), we could avoid passing several -Dxxx CL parameters (one per CMake option, e.g. -DENABLE_fancy_program and so on) by introducing a single -Cxxx parameter pointing at the most convenient .cmake intended for initializing the cache during the first run of cmake. Examples at robotology/yarp: CL invocation, initial-cache.cmake.

PeterBowman commented 6 years ago

Another ridiculous example (scroll right): roboticslab-uc3m/kinematics-dynamics@d5a9599.

PeterBowman commented 6 years ago

I did some CMake refactorization work in a bunch of repos. Namely:

Setting all options to OFF may create a twofold problem:

PeterBowman commented 6 years ago

@jgvictores suggests to keep all/most options enabled and warn the user if it cannot get built for whatever reason (missing package, unsatisfied dependencies, etc.).

jgvictores commented 6 years ago

Yes, essentially all enabled, and automatically disabling (without breaking, just messages/warnings) if requirements are not met.

PeterBowman commented 6 years ago

Warnings may be annoying, but help to keep users aware of which components could not be successfully configured. Setting everything to OFF essentially hides the functionality of a repo.

Idea: try the FeatureSummary CMake module. It was adopted some time ago in amor-api, then in project-generator. Devs may use it to register all available CMake options and provide short descriptions. Then, CMake generates a report after the configuration step. Example output of amor-api:

-- YCM found in /usr/local/share/YCM.
-- AMOR API version: 0.2.1 (0.2.1)
-- Boost version: 1.58.0
-- Found the following Boost libraries:
--   thread
--   date_time
--   chrono
--   system
--   atomic
-- Could NOT find Doxygen (missing:  DOXYGEN_EXECUTABLE) 
-- 
-- The following features have been enabled:

 * AMOR_API_extended , Advanced AMOR API functions.

-- The following REQUIRED packages have been found:

 * Eigen3
 * Boost

-- The following features have been disabled:

 * AMOR_API_cartesian_rate , cartesian_rate program (AMOR+YARP).

-- The following OPTIONAL packages have not been found:

 * Doxygen

-- Configuring done
-- Generating done
-- Build files have been written to: /home/bartek/wingit/amor-api/build-wsl

Starting from The following features have been enabled, we see the result of invoking feature_summary(). Moreover, it lists satisfied and missing package dependencies (find_package() calls).

PeterBowman commented 6 years ago

YARP devices already provide some sort of summarization mechanism, check this cmake .. sample run:

--  --- plugin AmorCartesianControl: disabled
--  +++ plugin AsibotSolver: enabled
--  +++ plugin BasicCartesianControl: enabled
--  --- plugin BasicTwoLimbCartesianControl: disabled (dependencies unsatisfied)
--  +++ plugin CartesianControlClient: enabled
--  +++ plugin CartesianControlServer: enabled
--  +++ plugin KdlSolver: enabled
--  +++ plugin TwoLimbCartesianControlServer: enabled
PeterBowman commented 6 years ago

keep all/most options enabled and warn the user if it cannot get built for whatever reason (missing package, unsatisfied dependencies, etc.)

Perhaps we should move most find_package() calls to an upper level (root CMakeLists.txt?) and then use the corresponding _FOUND variables in cmake_dependent_option()/yarp_prepare_plugin() (e.g. AmorCartesianControl/CMakeLists.txt@kinematics-dynamics).

jgvictores commented 6 years ago

Perhaps we should move most find_package() calls to an upper level (root CMakeLists.txt?)

Advantage: Faster and cleaner output of cmake. Disadvantage: Goes a bit against philosophy of everything near where it is used...

Seeing the disadvantage... Maybe only in the case a dep is detected to be used on more than one component, then move up one level? I have no idea....

PeterBowman commented 6 years ago

Idea: place find_package() calls before the opening if() clause in cases such as:

yarp_prepare_plugin(AmorCartesianControl
                    CATEGORY device
                    TYPE roboticslab::AmorCartesianControl
                    INCLUDE AmorCartesianControl.hpp
                    DEFAULT ON
                    DEPENDS ENABLE_KinematicRepresentationLib)

if(NOT SKIP_AmorCartesianControl)

find_package(AMOR_API REQUIRED)

# ...

Obviously, the REQUIRED parameter should be removed. Then, custom code (more ifs, message(), etc.) would take care of warning the user about unsatisfied deps and reflecting that in the GUI.

PeterBowman commented 6 years ago

Working example at roboticslab-uc3m/kinematics-dynamics@14cf9cb. Template:

find_package(3rdparty_pkg QUIET)

if(NOT 3rdparty_pkg_FOUND AND (NOT DEFINED ENABLE_MyComponent OR ENABLE_MyComponent))
    message(WARNING "3rdparty_pkg package not found, disabling MyComponent")
endif()

# same with yarp_prepare_plugin()
cmake_dependent_option(ENABLE_MyComponent "Enable/disable MyComponent" ON
                       3rdparty_pkg_FOUND OFF)

if(ENABLE_MyComponent) # 'NOT SKIP_MyComponent' for YARP devices
    # ...
else()
    set(ENABLE_MyComponent OFF CACHE BOOL "Enable/disable MyComponent" FORCE)
endif()

Travis YAML files may be updated so that local repository options are now omitted (see .travis.yml).

Regarding non-local CMake options (as described in the first comment in this issue, i.e. remote repos pulled via .travis.yml with lots of disabled components), we'll want to provide a means to disable all/most non-vital options. The reason is that downstreams should not be forced to compile unnecesary programs and libraries, especially whenever we just want to get access to a header file. To achieve that, the initial-cache.cmake way may be a valid solution (https://github.com/roboticslab-uc3m/questions-and-answers/issues/18#issuecomment-302174508).

PeterBowman commented 6 years ago

Expanding on my previous comment, such refactorization is only intended for devices/programs/libraries that depend on REQUIRED 3rd-party packages - except YARP. As spoken with @jgvictores, YARP itself should be considered a hard dependency in our repos, that is, we'll always instruct CMake to search for it with find_package(YARP REQUIRED) (note the second parameter). I'd go a bit further and pull this call to the root CMakeLists.txt, before traversing any subdirectories.

PeterBowman commented 6 years ago

Proposed roadmap (I'll replicate and expand this in the issue description if nobody opposes):

TODO: I need to check this ideas on a superbuild-like repo, especially regarding CMake options and BuildPkg.cmake files.

PeterBowman commented 6 years ago

Another TODO: if I place find_package(YARP REQUIRED) at the top, and then proceed as described earlier (3rd-party dep) with a YARP version that is not available on my system (e.g. find_package(YARP 2.3.70 QUIET)), what will happen with directories processed after that which depend on previous (supported) YARP releases?

(I was thinking about certain YARP devices in openrave-yarp-plugins)

jgvictores commented 6 years ago

I think I've found an issue with the current proposal.

Looking at these lines, cmake always goes through find_package(ARAVIS QUIET). Now, if we look at FindARAVIS.cmake, it has a find_package(GLib REQUIRED) which makes everything break, always, even if we are not using AravisGigE... :-(

PeterBowman commented 6 years ago

That find_package(GLib REQUIRED) should be moved to the caller list file (e.g. AravisGigE/CMakeLists.txt). Similarly, you have to explicitly call both find_package(OpenRAVE REQUIRED) and find_package(Boost REQUIRED) in the openrave-yarp-plugins repo.

However, I'd personally give a try to the find_dependency() command (ref) within FindARAVIS.cmake. I used to have one at ROBOTICSLAB_KINEMATICS_DYNAMICSConfig.cmake.in.

jgvictores commented 6 years ago

Okay, we can open a new issue for this, for now hacked via sudo apt install libglib2.0-dev.

PeterBowman commented 6 years ago

Another example: disable component if YARP's math lib cannot be found (roboticslab-uc3m/kinematics-dynamics@76eb0d7).

Edit: follow-up at roboticslab-uc3m/kinematics-dynamics@b93ff7d (avoid continuous generation of warning messages).

PeterBowman commented 6 years ago

Sometimes, a component depends on another. If the latter is not enabled, the former (usually) does not show up in the GUI at all. However, my proposal prints a warning anyway and I noticed that dependers do still show up (vision repo, check TravisLib and colorDetection(2D)).

PeterBowman commented 6 years ago

More examples:

PeterBowman commented 6 years ago

This mechanism (https://github.com/roboticslab-uc3m/questions-and-answers/issues/18#issuecomment-362724497) is not aware of 3rd-party deps being available upon successive configure runs. Example:

PeterBowman commented 6 years ago

YARP itself should be considered a hard dependency in our repos, that is, we'll always instruct CMake to search for it with find_package(YARP REQUIRED) (note the second parameter)

Example: https://github.com/roboticslab-uc3m/yarp-devices/commit/2fdc802cb7eecd061af11829a3ade5e93c56b224.

PeterBowman commented 6 years ago

https://github.com/roboticslab-uc3m/questions-and-answers/issues/18#issuecomment-359198905:

Idea: try the FeatureSummary CMake module.

I think it's already supported in custom YARP devices (ref, available since 2.3.68.1), that is, add a feature_summary(...) call at the end of root CMakeLists.txt and output should be automatically generated along with a list of available components.

PeterBowman commented 6 years ago

Another example (test two conditions): https://github.com/roboticslab-uc3m/vision/commit/f5c89fb414f885ec7f1804a4948517bd9d54ba97.

PeterBowman commented 6 years ago

Note: new YARP components may require additional attention when working with RL projects depending upon each other, see https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/273ac975247f12e5fbf640b1c08769c15e5fcaf1 and https://github.com/roboticslab-uc3m/questions-and-answers/issues/65.

PeterBowman commented 6 years ago

TODO: reflect this new dependency+warning mechanism in project-generator and repositories derived from this one (mainly tools and asibot-hmi).

PeterBowman commented 6 years ago

More ideas: https://github.com/robotology/yarp/commit/758d23c4ae8c1affdffdfc0e04830944774a38f6 (Plugins not enabled due to missing dependencies are now shown in ccmake andcmake-gui together with a list of dependencies that are not satisfied.).

PeterBowman commented 6 years ago

Somewhat a follow-up to https://github.com/roboticslab-uc3m/questions-and-answers/issues/48 motivated by https://github.com/roboticslab-uc3m/openrave-yarp-plugins/issues/91: if we just need a header file (usually, a YARP device interface) or a CMake module (rare) from a dependency, then clone/download, run cmake .., skip the compile part altogether and adjust the <project>_DIR variable.

PeterBowman commented 6 years ago

Perhaps we should move most find_package() calls to an upper level (root CMakeLists.txt?)

YARP itself should be considered a hard dependency in our repos

For future reference, this was done at https://github.com/roboticslab-uc3m/questions-and-answers/issues/54.

PeterBowman commented 6 years ago

(https://github.com/roboticslab-uc3m/questions-and-answers/issues/18#issuecomment-374586531)

This mechanism (...) is not aware of 3rd-party deps being available upon successive configure runs.

YARP v3.0.x suffers from this same problem, too, after https://github.com/robotology/yarp/commit/758d23c4ae8c1affdffdfc0e04830944774a38f6 was introduced. Looks like the final part of my proposed solution (https://github.com/roboticslab-uc3m/questions-and-answers/issues/18#issuecomment-362724497), which also resembles what YARP aims to achieve, is colliding with cmake_dependent_option's internals called either explicitly or from within yarp_prepare_plugin():

set(ENABLE_MyComponent OFF CACHE BOOL "Enable/disable MyComponent" FORCE)
PeterBowman commented 6 years ago

Note for travelers:

if(${var} MATCHES "^${var}$")

is a legacy way of saying:

if(NOT DEFINED ${var})

Found in CMakeDependentOption.cmake.

PeterBowman commented 3 years ago

CMake 3.13 introduces policy CMP0077. Not sure if we want this new behavior, though.

PeterBowman commented 3 years ago

This problem has been approached in some valid ways that are close enough to the intended result. I'm going to close it at last. A few remarks regarding what was discussed here:

Therefore, the original issue was already addressed quite a while ago. We ended up spreading boilerplate as detailed in previous comments (e.g. https://github.com/roboticslab-uc3m/vision/commit/f5c89fb414f885ec7f1804a4948517bd9d54ba97). Some people choose to see the ugliness in this code. The disarray. I choose to see the practicality. To believe there is an order to our CMake list files, a purpose.

PeterBowman commented 2 years ago

CMake 3.19 introduced presets which mostly behave like a JSON-formatted initial cache.