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

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

Closed
PeterBowman opened this issue Mar 28, 2018 · 10 comments
Closed
Assignees

Comments

@PeterBowman
Copy link
Member

PeterBowman commented Mar 28, 2018

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 PeterBowman changed the title YCM bootstrap script does not honor YCM_TAG YCM bootstrap script does not honor YCM_TAG for on-system lookup Mar 28, 2018
@PeterBowman
Copy link
Member Author

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 #54.

@PeterBowman
Copy link
Member Author

@PeterBowman
Copy link
Member Author

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

Alternatively, improve this behavior upstream.

@PeterBowman
Copy link
Member Author

Reminder: set(YCM_TAG <version>) is effectively a backport in case latest upstream master is corrupted, see #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
Copy link
Member Author

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 roboticslab-uc3m/color-debug#11). Please poke me if I forget to make the announcement!

@PeterBowman
Copy link
Member Author

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: #17, roboticslab-uc3m/kinematics-dynamics@8dc8928.

PeterBowman added a commit to roboticslab-uc3m/yarp-devices that referenced this issue Jun 16, 2018
PeterBowman added a commit to roboticslab-uc3m/openrave-yarp-plugins that referenced this issue Jun 16, 2018
@PeterBowman
Copy link
Member Author

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 added a commit to roboticslab-uc3m/speech that referenced this issue Jun 17, 2018
PeterBowman added a commit to roboticslab-uc3m/vision that referenced this issue Jun 17, 2018
PeterBowman added a commit to roboticslab-uc3m/tools that referenced this issue Jun 18, 2018
PeterBowman added a commit to roboticslab-uc3m/asibot-hmi that referenced this issue Jun 18, 2018
@PeterBowman
Copy link
Member Author

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

  • teo-bimanipulation: demo-like repo, I'm not comfortable with meddling in this for now
  • teo-grasp: same as above
  • tiago-main (private repo): not sure about the final outcome of this one
  • amor-api (private repo): I'll decide on bootstrapping YCM or not by the next minor release

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

  • color-debug: this repository might be used outside our organization, hence keeping things simple (the less dependencies, the better) seemed the best option
  • gait: not touching, this repository is out of the radar for now

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

@PeterBowman
Copy link
Member Author

PeterBowman commented Jul 8, 2018

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: robotology/robotology-superbuild#21, robotology/ycm-cmake-modules#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 added a commit to asrob-uc3m/robotDevastation-robots that referenced this issue Nov 8, 2018
rsantos88 pushed a commit to roboticslab-uc3m/yarp-devices that referenced this issue Jan 29, 2019
@PeterBowman
Copy link
Member Author

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 (#55 (comment)), ongoing work on #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.

PeterBowman added a commit to roboticslab-uc3m/asibot-main that referenced this issue Apr 18, 2019
PeterBowman added a commit to roboticslab-uc3m/teo-main that referenced this issue Apr 18, 2019
PeterBowman added a commit to roboticslab-uc3m/amor-main that referenced this issue Apr 18, 2019
PeterBowman added a commit to asrob-uc3m/robotDevastation-simulator that referenced this issue Apr 18, 2019
@PeterBowman PeterBowman mentioned this issue Apr 18, 2019
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant