Handle OPTIONAL_COMPONENTS in find_package(YARP) #1741

PeterBowman · 2018-06-10T17:48:04Z

There is no way to make this work with find_dependency() since it passes caller's REQUIRED option to the underlying find_package() call. That is, no error should be raised if an optional component was not
found while issuing a find_package(YARP REQUIRED ...) command. See also this related discussion in the CMake's community mailing list: ref.

Example invocation (tested with YARP static and shared builds):

find_package(YARP REQUIRED COMPONENTS OS dev
                           OPTIONAL_COMPONENTS math asdf)

CMake issues a diagnostic message for the missing YARP_asdf component:

-- Could NOT find YARP_asdf (missing: YARP_asdf_DIR)

This is a follow-up proposal to #1715. The CMake <3.9 fix was introduced in #1731.

drdanz

LGTM, thanks.
Unfortunately I made a lot of changes in order to fix static builds, and this will no longer apply... Also I consider "not supporting OPTIONAL_COMPONENTS" a bug, therefore I would like this in 3.0.1... do you mind moving your branch to the latest master?

PeterBowman · 2018-06-15T14:11:31Z

@drdanz rebased, updated and tested with BUILD_SHARED_LIBS=ON, but I'm not sure about the handling of static components:

yarp/src/libYARP_init/YARP_OSConfigStatic.cmake.in

Lines 64 to 89 in f23f86b

    
           _yarp_include_targets_file(YARP_OS) 
        
           # Properly find the dependencies. This will force to include all the 
        
           # dependencies also for packages in other exports 
        
           foreach(_component ${_yarp_included_components}) 
        
             # FIXME Remove this check when CMake 3.9 or later is required 
        
             if (CMAKE_VERSION VERSION_LESS 3.9) 
        
               set(_YARP_OS_FIND_PARTS_REQUIRED) 
        
               if (YARP_OS_FIND_REQUIRED) 
        
                 set(_YARP_OS_FIND_PARTS_REQUIRED REQUIRED) 
        
               endif() 
        
               set(_YARP_OS_FIND_PARTS_QUIET) 
        
               if (YARP_OS_FIND_QUIETLY) 
        
                 set(_YARP_OS_FIND_PARTS_QUIET QUIET) 
        
               endif() 
        
               find_package(${_component} 
        
                            ${_YARP_OS_FIND_PARTS_QUIET} 
        
                            ${_YARP_OS_FIND_PARTS_REQUIRED} 
        
                            HINTS "${YARP_CMAKECONFIG_DIR}" 
        
                            NO_DEFAULT_PATH) 
        
             else() 
        
               find_dependency(${_component} 
        
                               HINTS "${YARP_CMAKECONFIG_DIR}" 
        
                               NO_DEFAULT_PATH) 
        
             endif() 
        
           endforeach()

Unless I'm wrong, this block loops over YARP_OS's own targets and its dependencies', but it doesn't care about any COMPONENTS nor OPTIONAL_COMPONENTS option passed to find_package(YARP_OS) (I don't see any YARP_OS_FIND_COMPONENTS variable anywhere).

drdanz · 2018-06-20T13:17:33Z

@PeterBowman This is required because, in order to support static plugins, there is a circular dependency in static builds that does not exist in shared builds. i.e.
This means that, for static builds, none of the components involved in the loop (i.e. OS, sig, dev, and optionally math) can be missing. "optional components" are not optional in this case.

drdanz · 2018-06-20T13:24:19Z

cmake/template/YARPConfig.cmake.in

+               HINTS "${YARP_CMAKECONFIG_DIR}"
+               NO_DEFAULT_PATH)
+  if(YARP_${_yarp_module}_FOUND)
+    list(APPEND YARP_LIBRARIES YARP::YARP_${_yarp_module})


I'm not sure this is correct...
In my opinion, YARP_LIBRARIES should always contain all the COMPONENTS, even if not found, so that linking will fail if one component is missing.
For the OPTIONAL_COMPONENTS this check makes sense though.
So I'd say this should be something like

if(YARP_FIND_REQUIRED_${_yarp_module} OR YARP_${_yarp_module}_FOUND) list(APPEND YARP_LIBRARIES YARP::YARP_${_yarp_module}) endif()

@PeterBowman does this ^ make sense to you?

CMake should raise an error during the generate phase (non-fatal, though), check the output I copy-pasted at #1740 to see what I mean. Anyway, I'd consider the YARP_LIBRARIES variable unreachable in such case: if I issue a find_package(YARP 3.0 COMPONENTS asdf) call and the YARP_asdf component is not found, YARP_FOUND will be set to FALSE (by check_required_components()), so I think it's safe to assume that nobody will reference YARP_LIBRARIES nor any other variable or target (since YARP will not be regarded by CMake as found on system).

PeterBowman · 2018-06-20T17:28:04Z

This means that, for static builds, none of the components involved in the loop (i.e. OS, sig, dev, and optionally math) can be missing. "optional components" are not optional in this case.

Still not sure, I ran a cmake . --trace-expand and I think it didn't show any higher level components, just YARP_OS itself and its inner dependencies (such as *_ros, *_rtf and the like). Need to test again, but I'm a bit busy, let me get back to this by the end of the week :). Marking as WIP again, for now.

drdanz · 2018-06-21T10:16:18Z

There is another annoying problem with using find_package instead of find_dependency, the FeatureSummary module will print a line for each component:

For example:

cmake_minimum_required(VERSION 3.0)
project(test_fs)
include(FeatureSummary)
find_package(YARP COMPONENTS conf OS gsl math)
feature_summary(WHAT ALL INCLUDE_QUIET_PACKAGES)

will print:

-- The following OPTIONAL packages have been found:

 * YARP_conf
 * YARP_OS
 * YARP_gsl
 * YARP_math
 * YARP, A library and toolkit for communication and device interfaces, <https://www.yarp.it/>

-- The following OPTIONAL packages have not been found:

 * YCM (required version >= 0.8.1)

Using find_dependency only prints YCM (that is searched with find_package as well)

I think this can be fixed in a hackish way by setting

  set_property(GLOBAL PROPERTY _CMAKE_YARP_${_yarp_module}_TRANSITIVE_DEPENDENCY TRUE)

(note: this should be done for YCM as well, but we should ensure that it is not set to true yet)

PeterBowman · 2018-06-21T10:28:43Z

Is it feasible to include a /cmake/ycm-0.x.x/cmake-3.9/CMakeFindDependencyMacro.cmake file in YARP?

drdanz · 2018-06-21T12:06:52Z

Will cmake 3.9 version allow to replace REQUIRED? Otherwise is there a specific reason why you want to use that?

drdanz · 2018-06-21T13:01:45Z

(note: this should be done for YCM as well, but we should ensure that it is not set to true yet)

Done: 8f582d3

PeterBowman · 2018-06-21T14:48:02Z

cmake/template/YARPConfig.cmake.in

-                    NO_DEFAULT_PATH)
+  set(_YARP_FIND_PART_${_yarp_module}_REQUIRED)
+  # Only propagate REQUIRED if module was not passed to OPTIONAL_COMPONENTS
+  if(YARP_FIND_PARTS_REQUIRED AND YARP_FIND_REQUIRED_${_yarp_module})


Note to self: YARP_FIND_REQUIRED instead of YARP_FIND_PARTS_REQUIRED.

PeterBowman · 2018-06-21T14:56:03Z

Will cmake 3.9 version allow to replace REQUIRED? Otherwise is there a specific reason why you want to use that?

You are right, sorry. Similarly, find_dependencies() would require to hack the YARP_FIND_REQUIRED variable on each iteration, which renders no advantage over find_package(). Perhaps it could have made sense before this PR.

drdanz · 2018-06-21T16:54:20Z

You are right, sorry. Similarly, find_dependencies() would require to hack the YARP_FIND_REQUIRED variable on each iteration, which renders no advantage over find_package().

I'd say let's stick with find_package, but with a workaround like 8f582d3 for each dependency...

PeterBowman · 2018-06-24T22:50:30Z

Still not sure, I ran a cmake . --trace-expand (...)

yarp/src/libYARP_init/YARP_OSConfigStatic.cmake.in

Line 68 in d3fb843

foreach(_component ${_yarp_included_components})

Result of expanding that CMake variable on a simple downstream app (plain find_package(YARP)):

foreach(_component YARP_priv_hmac;YARP_yarpcar;YARP_priv_xmlrpcpp;YARP_sig;YARP_wire_rep_utils;YARP_math;YARP_eigen;YARP_yarpmod;YARP_dev;YARP_rosmsg;YARP_rosmsg_native;YARP_rosmsg_std_msgs;YARP_rosmsg_actionlib_msgs;YARP_rosmsg_diagnostic_msgs;YARP_rosmsg_geometry_msgs;YARP_rosmsg_nav_msgs;YARP_rosmsg_sensor_msgs;YARP_rosmsg_shape_msgs;YARP_rosmsg_stereo_msgs;YARP_rosmsg_trajectory_msgs;YARP_rosmsg_visualization_msgs;YARP_rosmsg_tf;YARP_rosmsg_tf2_msgs )

BTW it's called 420 times; in 419 of them, ${_yarp_included_components} resolves to an empty string.

drdanz · 2018-06-25T13:58:47Z

Result of expanding that CMake variable on a simple downstream app (plain find_package(YARP)):

foreach(_component YARP_priv_hmac;YARP_yarpcar;YARP_priv_xmlrpcpp;YARP_sig;YARP_wire_rep_utils;YARP_math;YARP_eigen;YARP_yarpmod;YARP_dev;YARP_rosmsg;YARP_rosmsg_native;YARP_rosmsg_std_msgs;YARP_rosmsg_actionlib_msgs;YARP_rosmsg_diagnostic_msgs;YARP_rosmsg_geometry_msgs;YARP_rosmsg_nav_msgs;YARP_rosmsg_sensor_msgs;YARP_rosmsg_shape_msgs;YARP_rosmsg_stereo_msgs;YARP_rosmsg_trajectory_msgs;YARP_rosmsg_visualization_msgs;YARP_rosmsg_tf;YARP_rosmsg_tf2_msgs )

That's correct...

BTW it's called 420 times; in 419 of them, ${_yarp_included_components} resolves to an empty string.

That's a little weird, does this mean that the YARP_OSConfigStatic.cmake file is included 420 times?

drdanz · 2018-06-25T14:01:05Z

Anyway, this is unrelated to this patch, we can fix it (if it requires fixing) on a separate PR.
Please update this PR, I'd like to merge this and release YARP 3.0.1 in a few days

There is no way to make this work with `find_dependency()` since it passes caller's REQUIRED to the underlying `find_package()` call. That is, no error should be raised if an optional component was not found while issuing a `find_package(YARP REQUIRED ...)` command.

PeterBowman · 2018-06-25T18:05:11Z

That's a little weird, does this mean that the YARP_OSConfigStatic.cmake file is included 420 times?

Yes.

I'd say let's stick with find_package, but with a workaround like 8f582d3 for each dependency...

We'd have to add this tweak on the YCM side, too (InstallBasicPackageFiles.cmake). That is, I can apply that hack around each find_package() call in YARPConfig.cmake, but not inside YARP_<component>Config.cmake where the inner dependencies (e.g. YARP_conf) are searched for. Those will still show up when calling feature_summary().

Please update this PR, I'd like to merge this and release YARP 3.0.1 in a few days

Removing [WIP] label, please review. I'm not touching YARP_OSConfigStatic.cmake.in since it has nothing to do with (OPTIONAL_)COMPONENTS passed to find_package(YARP), and find_package(YARP_OS COMPONENTS ...) is never handled anyway. See #1741 (comment) regarding the YARP_LIBRARIES thing.

drdanz

LGTM now

drdanz · 2018-06-26T13:06:13Z

Merged, thanks.

We'd have to add this tweak on the YCM side, too (InstallBasicPackageFiles.cmake).

This tweak would useful be for CMake < 3.9 only, since the new ones use find_dependency, and do not handle components. I don't think it is worth doing such a change.

PeterBowman requested a review from drdanz as a code owner June 10, 2018 17:48

PeterBowman force-pushed the cmake-optional-components branch 5 times, most recently from 871f1d5 to 19a87ba Compare June 13, 2018 11:09

PeterBowman mentioned this pull request Jun 14, 2018

Pull up common YARP-related lines to root CMakeLists.txt roboticslab-uc3m/questions-and-answers#54

Closed

drdanz suggested changes Jun 15, 2018

View reviewed changes

drdanz added Component: CMake Target: YARP v3.0.1 labels Jun 15, 2018

PeterBowman force-pushed the cmake-optional-components branch from 19a87ba to d266987 Compare June 15, 2018 12:03

PeterBowman changed the base branch from devel to master June 15, 2018 12:03

PeterBowman changed the title ~~Handle OPTIONAL_COMPONENTS in find_package(YARP)~~ [WIP] Handle OPTIONAL_COMPONENTS in find_package(YARP) Jun 15, 2018

PeterBowman force-pushed the cmake-optional-components branch from d266987 to f61845f Compare June 15, 2018 12:17

PeterBowman changed the title ~~[WIP] Handle OPTIONAL_COMPONENTS in find_package(YARP)~~ Handle OPTIONAL_COMPONENTS in find_package(YARP) Jun 15, 2018

PeterBowman force-pushed the cmake-optional-components branch 2 times, most recently from b1b13da to a18b531 Compare June 19, 2018 10:57

drdanz reviewed Jun 20, 2018

View reviewed changes

PeterBowman changed the title ~~Handle OPTIONAL_COMPONENTS in find_package(YARP)~~ [WIP] Handle OPTIONAL_COMPONENTS in find_package(YARP) Jun 20, 2018

PeterBowman commented Jun 21, 2018

View reviewed changes

PeterBowman force-pushed the cmake-optional-components branch from a18b531 to 771e62b Compare June 25, 2018 17:33

PeterBowman force-pushed the cmake-optional-components branch from 771e62b to 7858479 Compare June 25, 2018 17:35

PeterBowman changed the title ~~[WIP] Handle OPTIONAL_COMPONENTS in find_package(YARP)~~ Handle OPTIONAL_COMPONENTS in find_package(YARP) Jun 25, 2018

drdanz approved these changes Jun 26, 2018

View reviewed changes

drdanz merged commit b7983c9 into robotology:master Jun 26, 2018

PeterBowman deleted the cmake-optional-components branch June 26, 2018 13:09

PeterBowman mentioned this pull request Jun 26, 2018

Forward compatibility with YARP 3.0 roboticslab-uc3m/questions-and-answers#65

Closed

traversaro mentioned this pull request Jun 26, 2018

Superbuild finds its own subproject components on Ubuntu if <package>_DIR env variables are defined robotology/robotology-superbuild#92

Closed

drdanz added the Resolution: Merged label Jul 2, 2018

PeterBowman mentioned this pull request Jan 1, 2020

Remove obsolete code for CMake older than 3.9 robotology/ycm-cmake-modules#309

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle OPTIONAL_COMPONENTS in find_package(YARP) #1741

Handle OPTIONAL_COMPONENTS in find_package(YARP) #1741

PeterBowman commented Jun 10, 2018

drdanz left a comment

PeterBowman commented Jun 15, 2018 •

edited

Loading

drdanz commented Jun 20, 2018

drdanz Jun 20, 2018

drdanz Jun 20, 2018

PeterBowman Jun 20, 2018

PeterBowman commented Jun 20, 2018

drdanz commented Jun 21, 2018

PeterBowman commented Jun 21, 2018

drdanz commented Jun 21, 2018

drdanz commented Jun 21, 2018

PeterBowman Jun 21, 2018

PeterBowman commented Jun 21, 2018

drdanz commented Jun 21, 2018

PeterBowman commented Jun 24, 2018

drdanz commented Jun 25, 2018

drdanz commented Jun 25, 2018

PeterBowman commented Jun 25, 2018

drdanz left a comment

drdanz commented Jun 26, 2018

Handle OPTIONAL_COMPONENTS in find_package(YARP) #1741

Handle OPTIONAL_COMPONENTS in find_package(YARP) #1741

Conversation

PeterBowman commented Jun 10, 2018

drdanz left a comment

Choose a reason for hiding this comment

PeterBowman commented Jun 15, 2018 • edited Loading

drdanz commented Jun 20, 2018

drdanz Jun 20, 2018

Choose a reason for hiding this comment

drdanz Jun 20, 2018

Choose a reason for hiding this comment

PeterBowman Jun 20, 2018

Choose a reason for hiding this comment

PeterBowman commented Jun 20, 2018

drdanz commented Jun 21, 2018

PeterBowman commented Jun 21, 2018

drdanz commented Jun 21, 2018

drdanz commented Jun 21, 2018

PeterBowman Jun 21, 2018

Choose a reason for hiding this comment

PeterBowman commented Jun 21, 2018

drdanz commented Jun 21, 2018

PeterBowman commented Jun 24, 2018

drdanz commented Jun 25, 2018

drdanz commented Jun 25, 2018

PeterBowman commented Jun 25, 2018

drdanz left a comment

Choose a reason for hiding this comment

drdanz commented Jun 26, 2018

PeterBowman commented Jun 15, 2018 •

edited

Loading