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

Forward compatibility with YARP 3.0 #65

Closed PeterBowman closed 6 years ago

PeterBowman commented 6 years ago

YARP 3.0 is finally out! And back compat is broken with YARP 2.x code. There are two sides to this coin:

The former is causing Travis build failures since the past few days (said line was added shortly before the final release). I'm not prone to blindly remove the version specifier to find_package() (see https://github.com/roboticslab-uc3m/questions-and-answers/issues/54 for background), but this could be easily solved by arranging some convenient checks in the following manner:

find_package(YARP 3.0 QUIET COMPONENTS ...)
# in the future, remove this check and use REQUIRED above
if(NOT YARP_FOUND)
    find_package(YARP 2.3.70 REQUIRED)
endif()

Alternatively:

find_package(YARP QUIET)
if(YARP_VERSION_SHORT VERSION_LESS 2.3.70)
    message(FATAL_ERROR "You need YARP 2.3.70+, but only ${YARP_VERSION_SHORT} was found!")
endif()

See also https://github.com/roboticslab-uc3m/questions-and-answers/issues/54#issuecomment-397289683 regarding YARP components and https://github.com/roboticslab-uc3m/questions-and-answers/issues/18 regarding optional project components.

Migration is a gradual step. Once fixed, CI builds will test our code against YARP master and devel (to keep up with YARP's development, see https://github.com/roboticslab-uc3m/questions-and-answers/issues/7), both currently at 3.0+, but we'll surely keep using YARP 2.3.x in our PCs. My second proposal covers the improvement of .travis.yml files across the organization in order to add an additional job for said legacy YARP version. Thus, we'll make sure both the previous and current release cycles of YARP are compatible with our code. In that vein, I found this reference page useful since conditional jobs shall be run: two per pr/push action (YARP 2.3.72.1 and YARP master), only one for cron-enabled builds (YARP devel, like usual). Some YAML code improvements laid out at https://github.com/roboticslab-uc3m/questions-and-answers/issues/48 might come in handy here.

PeterBowman commented 6 years ago

Marking as blocked, I'd close https://github.com/roboticslab-uc3m/questions-and-answers/issues/54 first.

PeterBowman commented 6 years ago

Forgot to add: YARP 3.0 requires CMake 3.5. Bump it in non-superbuild repos, teo-main and the like have been already updated in order to support stricter requirements in orchestrated subprojects.

PeterBowman commented 6 years ago

As spoken with @jgvictores, in the future, we'd like to create one tag per repo prior to requiring YARP 3.0 everywhere. The goal is to keep track of the last point in the commit history that supported YARP 2.x.

PeterBowman commented 6 years ago

My second proposal covers the improvement of .travis.yml files across the organization in order to add an additional job for said legacy YARP version.

See https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/d52434cbe35dc17e61c46153bfd9d783ba0a5bdc. Testing our new policy: run a job for every supported minor YARP release (currently: v2.3.70, v2.3.72), which in total accounts for six jobs considering gcc/clang and YARP's master branch, on every push action (example). In addition, cron builds would add another couple of jobs against YARP's devel branch, thus totalling eight jobs (example). Also, we pull YCM's master branch in the former build set, and YCM's devel in the latter.

PeterBowman commented 6 years ago

I've come up with a possibly elegant solution at https://github.com/roboticslab-uc3m/questions-and-answers/issues/48#issuecomment-399684024. I mark this therefore as blocked because of build times having considerably increased as a consequence of adding new jobs. Let's solve the dependency caching issue first.

PeterBowman commented 6 years ago

Support for optional components has been approved and merged into current YARP master with https://github.com/robotology/yarp/pull/1741. YARP v3.0.1 is expected to come out soon, hence I'd like to review all find_package(YARP 3.0 ...) calls again.

PeterBowman commented 6 years ago

Keep https://github.com/robotology/yarp/pull/1778 in mind while updating the speech project.

PeterBowman commented 6 years ago

Since https://github.com/robotology/yarp/pull/1778, looking for YCM before YARP leads to CI builds errors in superbuild-like repos that orchestrate subprojects in which a feature_summary(FATAL_ON_MISSING_REQUIRED_PACKAGES WHAT ...) call is present (example, FeatureSummary docs (CMake 3.9)). Rationale (sample app):

# Registers and sets a _CMAKE_YARP_TYPE=REQUIRED global property.
find_package(YARP REQUIRED)
message(STATUS "YARP_FOUND: ${YARP_FOUND}") # 1 (found)

# Let's assume we didn't get there yet. Note the QUIET OPTION.
find_package(YARP 99 QUIET)
message(STATUS "YARP_FOUND: ${YARP_FOUND}") # 0 (not found)

include(FeatureSummary)
feature_summary(FATAL_ON_MISSING_REQUIRED_PACKAGES WHAT ALL)

This snippet would throw an error. YARP_FOUND is set to FALSE in the subsequent find_package() call. However, this package is still regarded as a REQUIRED one, hence the fatal outcome of the feature_summary() command.

There is plenty of room for improvements, but I'll start with removing the FATAL_ON_MISSING_REQUIRED_PACKAGES option. Any prior find_package() command would throw anyway.

Edit: forgot to add that the alternative (and counterintuitive) solution is fairly simple, just issue find_package(YCM REQUIRED) after find_package(YARP REQUIRED).

Edit2: this error showed up because superbuild repos fetch the master branch of YCM, whereas latest YARP master requests YCM devel (see linked PR).

PeterBowman commented 6 years ago

For the time being, I'm not going to use YARP 3.x components. This feature will render no benefits in our projects and would require extra care:

Again: for now, I'm happy with current component defaults and no repo of ours require anything more convolute than that. Let's adopt the following sequence (already used throughout most repos) and close this issue:

# Hard dependencies.
find_package(YCM 0.8 REQUIRED)

# https://github.com/roboticslab-uc3m/questions-and-answers/issues/65
find_package(YARP 3.0 QUIET)
if(NOT YARP_FOUND)
    find_package(YARP 2.3.70 REQUIRED)
endif()

find_package(COLOR_DEBUG REQUIRED)

Edit:

Keep https://github.com/robotology/yarp/pull/1778 in mind while updating the speech project.

I forgot about this. YARP 3.x will happily treat idl_tools in the default components, but show deprecations warnings if the CMake macros it sets are used. Looks like explicit components are preferred, at least in this case. I'd defer this matter until a) deprecations become build failures, or b) we enforce YARP 3.0 or later. Then, we'll start resolving deprecations one by one.

PeterBowman commented 5 years ago

https://github.com/roboticslab-uc3m/questions-and-answers/issues/65#issuecomment-403255082:

Edit: forgot to add that the alternative (and counterintuitive) solution is fairly simple, just issue find_package(YCM REQUIRED) after find_package(YARP REQUIRED).

Perhaps linked with https://github.com/robotology/yarp/commit/9bfd7ca3fb48ecb126c86cd2d839bd32e7a78033.

PeterBowman commented 5 years ago

This code:

find_package(YARP 3.0 QUIET COMPONENTS ...)
# in the future, remove this check and use REQUIRED above
if(NOT YARP_FOUND)
    find_package(YARP 2.3.70 REQUIRED)
endif()

...is causing trouble on Windows and YARP 2.3.70. The YARP_DIR variable seems to be ignored by the second find_package(). My hunch is that this might happen on Linux, too, as long as YARP 3.x is not found and YARP_DIR points at a build directory for YARP 2.x.