search

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

YCM bootstrap script does not honor YCM_TAG for on-system lookup #55

Closed PeterBowman closed 6 years ago

PeterBowman commented 6 years ago

We usually have something like this at root CMakeLists.txt:

# Bootstrap YCM. Keep it compatible with cmake_minimum_required().
set(YCM_TAG v0.6.0)
include(YCMBootstrap)

YCM is not bootstrapped (and, consequently, cloned from its remote GH repo) when found on system. However, it isn't either when the version found on system is prior to the one requested by the project's CMake configuration, e.g. we installed YCM 0.2.2 (and forgot to upgrade!), YCMBootstrap finds it and skips the clone step.

Ideally, CMake should be smart enough to dismiss on-system YCM if the YCM_TAG requirement is not satisfied, and tell YCMBootstrap to proceed as if no YCM was installed at all. Proposed workaround:

set(_ycm_release 0.6.0)
find_package(YCM ${_ycm_release} QUIET)

if(NOT YCM_FOUND)
    set(YCM_TAG v${_ycm_release})
    set(USE_SYSTEM_YCM FALSE)
    include(YCMBootstrap)
endif()
PeterBowman commented 6 years ago

Alternatively, consume YCM as a hard dependency in non-superbuild repos, i.e. put find_package(YCM 0.6.0 REQUIRED) instead of the previous lines of code. These are actively developed projects, I can't imagine anyone forcing CMake to pull changes from YCM's remote repository on each configure step. In fact, I advised @jgvictores to install YCM on TEO's onboard CPUs. On the other hand, superbuild repos (-main) are meant to be self-contained, thus bootstrapping makes sense in that case.

Regarding other hard dependencies, see https://github.com/roboticslab-uc3m/questions-and-answers/issues/54.

PeterBowman commented 6 years ago

Reference: http://robotology.github.io/ycm/gh-pages/git-master/manual/ycm-using.7.html.

PeterBowman commented 6 years ago

Just debating available options, I'm not fully convinced about this:

Alternatively, improve this behavior upstream.

PeterBowman commented 6 years ago

Reminder: set(YCM_TAG <version>) is effectively a backport in case latest upstream master is corrupted, see https://github.com/roboticslab-uc3m/questions-and-answers/issues/24. This is easier to handle when working with bootstrapped YCM as all it does is a tag checkout after git fetch (or git clone). On-system installations shall use the EXACT option to find_package() at the cost of losing flexibility (no exact version found -> fatal error).

PeterBowman commented 6 years ago

As spoken with @jgvictores, green lights for YCM as hard dependency. Main reason is maintainability and future enhancements of our CMake code (for the record, current hacks are blocking my progress on https://github.com/roboticslab-uc3m/color-debug/issues/11). Please poke me if I forget to make the announcement!

PeterBowman commented 6 years ago

I'm going to apply cron jobs to latest YCM devel branch and usual push jobs to the master one, as similarly done with YARP: https://github.com/roboticslab-uc3m/questions-and-answers/issues/17, https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/8dc892865a14665a3f2b9683ae564cf9bcfcadc1.

PeterBowman commented 6 years ago

In superbuild repos (teo-main, asibot-main, amor-main), apply the fix described at #55 (comment) (or fix this properly upstream) and keep using YCM as a soft dependency.

Since orchestrated projects will now consume YCM as a hard dependency, and therefore the relevant YCM version will be requested from their root CMakeLists.txt, there is no need to care for a sensible minimum YCM version at the root CMakeLists.txt of superbuild projects. That is, the include(YCMBootstrap) call should suffice.

PeterBowman commented 6 years ago

I think this issue has been now completed. Only leftovers (occurrences of YCM_TAG across the organization):

In addition, several repositories that previously hosted a AddUninstallTarget.cmake file now require YCM as a hard dependency, too. Leftovers:

BTW perhaps it's time to think about a monologue label!

PeterBowman commented 6 years ago

It turns out that YCM 0.8 (unreleased at the time of writing) takes care of the initial problem, that is, requesting a minimum required on-system YCM version that, if not found, proceeds to the bootstrap process: https://github.com/robotology/robotology-superbuild/issues/21, https://github.com/robotology/ycm/pull/140 (the variable is YCM_MINIMUM_VERSION). Anyway, consuming YCM as a hard dependency is the preferred way now in most repos, even the superbuild ones, and it should have been so back then. I might still introduce it in amor-api, though.

PeterBowman commented 5 years ago

Since orchestrated projects will now consume YCM as a hard dependency, and therefore the relevant YCM version will be requested from their root CMakeLists.txt, there is no need to care for a sensible minimum YCM version at the root CMakeLists.txt of superbuild projects. That is, the include(YCMBootstrap) call should suffice.

I'm prone to apply the YCM_MINIMUM_VERSION thing (https://github.com/roboticslab-uc3m/questions-and-answers/issues/55#issuecomment-403298296), ongoing work on https://github.com/roboticslab-uc3m/questions-and-answers/issues/78 has convinced me that usage of certain YCM goodies may be narrowed to superbuild projects only. So, we might end up requesting a higher YCM version in these repos than in their orchestrated subprojects.