[CMake] Code and API review request for Qt5 CMake files

[Alexander Neundorf]

On Thursday 01 March 2012, Michael Hertling wrote:
> On 02/28/2012 10:03 PM, Alexander Neundorf wrote:
> > ...will reply later in detail.
> > 
> > Could you please go through the existing find-modules shipped with cmake
> > which support COMPONENTS and make a summary of how they handle them ?
> > 
> > At least FindQt4.cmake be default searches all components.
> > 
> > Thanks
> > Alex
> 
> The following CMakeLists.txt can be used to systematically investigate
> the results of the component-aware find modules currently shipped with
> CMake, these are Find{Boost,GTK2,HDF5,ImageMagick,Java,OpenSceneGraph,
> Qt4,wxWidgets,XMLRPC}:
> 
> CMAKE_MINIMUM_REQUIRED(VERSION 2.8 FATAL_ERROR)
> PROJECT(FINDRESULTVARIABLES C CXX)
> SET(CMAKE_VERBOSE_MAKEFILE ON)
> 
> FUNCTION(FindResultVariables pkg)
>     SET(prefix ${pkg})
>     IF(DEFINED ARGV1)
>         SET(prefix ${ARGV1})
>     ENDIF()
>     UNSET(REQUIRED)
>     IF(${pkg}_REQUIRED)
>         SET(REQUIRED REQUIRED)
>     ENDIF()
>     FIND_PACKAGE(${pkg} COMPONENTS ${${pkg}_COMPONENTS} ${REQUIRED})
>     MESSAGE("Begin: ${pkg} result variables")
>     GET_DIRECTORY_PROPERTY(v VARIABLES)
>     FOREACH(i IN LISTS v)
>         IF(i MATCHES "^${prefix}.*_FOUND$")
>             MESSAGE("${i}=${${i}}")
>         ENDIF()
>     ENDFOREACH()
>     MESSAGE("${prefix}_LIBRARIES: ${${prefix}_LIBRARIES}")
>     MESSAGE("End: ${pkg} result variables")
> ENDFUNCTION()
> 
> FindResultVariables(Boost)
> FindResultVariables(GTK2)
> FindResultVariables(HDF5)
> FindResultVariables(ImageMagick)
> FindResultVariables(Java)
> FindResultVariables(OpenSceneGraph)
> FindResultVariables(Qt4 QT)
> FindResultVariables(wxWidgets)
> FindResultVariables(XMLRPC)

Thanks :-)
 
> HDF5, OpenSceneGraph and XMLRPC aren't installed on my system; my
> preliminary findings for the remaining packages are as follows:

Maybe you can have a look at the code of those ?

> (1) Searching unrequested components:
> 
> Qt4: Yes.
> GTK2,ImageMagick: Partially.
> wxWidgets: All components if none are requested.
> 
> The remaining modules don't search unrequested components.
> 
> (2) Assigning *_*_FOUND variables:
> 
> Qt4: Only for modules which are known and found.
> ImageMagick: Also for requested but unknown components.
> wxWidgets: No *_*_FOUND variables at all but forwards unknown
>            components to *_LIBRARIES variable without an error.
> 
> The remaining modules assign only to *_*_FOUND variables for
> components which they know and which have been requested.
> 
> (3) Respecting the REQUIRED flag:
> 
> wxWidgets: REQUIRED ignored completely.

This is clearly a bug.

> Boost: SEND_ERROR instead of FATAL_ERROR.

This is somewhere between acceptable and bug.

> GTK2,Java: Bail out on unknown components even if not REQUIRED.

Depending on how unknown components should be handled, this is either ok or 
not.
I'm leaning towards supporting only known components, so the find-module or 
config file knows what it is doing.
In this case this would be correct behaviour, the programmer would see this 
error, not the user.

> The remaining modules bail out on unavailable requested components.
> 
> (4) Assinging the package-related *_FOUND variable:
> 
> Java: No *_FOUND variable at all although it's documented.

Bug.

> wxWidgets: TRUE even if requested components aren't found, see above.
> 
> The remaining modules return FALSE if a requested component isn't found.
> 
> My comments on these, say, multifarious findings are:

;-)

> Ad.(1): In general, automatically searching unrequested components
> does not mean a harm, but it is also not beneficial at all events:
> 
> - No guarantee to catch all components - consider a component added
>   later to the package - so no guarantee that all later *_*_FOUND
>   variables have been assigned a definite value by FIND_PACKAGE().
> - Complicates find modules / config files due to REQUIRED/QUIET.
> - Potentially higher costs due to unneeded search operations.
> 
> For the latter loint, there is a not-so-uncommon use case: Suppose
> a find module wants to check whether a library matches its header.
> Putting away cross-compiling issues for the moment, this requires
> building and running a test program. If it is to be performed for
> each Qt4 module, e.g., a FIND_PACKAGE(Qt4) invocation would most
> certainly be quite expensive. For these reasons, I usually advice
> to search only requested components, request all components going
> to be used and refer only to components having been requested.

In general this is not done in cmake, and also in general not recommened. 
Find-modules should simply use find_path(), find_library() etc.

> Ad.(2): Due to my guiding principle - refer only to FOUND variables
> which have been assigned a definite value by FIND_PACKAGE() - I do
> consider as important that a *_*_FOUND variable is assigned to for
> each requested component. FindImageMagick.cmake does it right, but
> the other modules - except for FindwxWidgets.cmake - have me check
> *_*_FOUND variables without a definite value for requested but un-
> known components. Again: The latters might become known one day.

I'd prefer not supporting unknown components. The find-module doesn't know 
anything about them (since they are unknown), whether they have additional 
dependencies, or whatnot.

> Ad.(3): After FIND_PACKAGE(... REQUIRED), I definitely do not want
> to check if all requested components including their prerequisites
> are actually present. OTOH, the user must have the oppurtunity to
> handle a package's or a component's absence by h(is|er)self. Thus,
> FIND_PACKAGE() without REQUIRED should never bail out because of
> something being not found, but with REQUIRED, it must bail out.

Yes.

> Ad.(4): None of the modules returning *_FOUND==FALSE just because
> a component is missing could be turned into a config file at the
> moment without changing its behavior. In order to address this
> issue in the future, one has to decide if either
> 
> - a find module should behave like a config file in this regard and
>   return *_FOUND==FALSE only if the package is totally unavailable;
>   my favorite, or
> 
> - FIND_PACKAGE() should should allow config files to assign FALSE to
>   *_FOUND if a requested component is unavailable; your favorite, I
>   guess.

This is on my TODO for 2.8.8.

> In the latter case, the components' *_*_FOUND variables would need
> to be assigned to also, and if no config file has been found, this
> must be done by FIND_PACKAGE() autonomously. Though, the *_*_FOUND
> variables are just a convention of Modules/readme.txt whereas the
> package's *_FOUND variable is documented for FIND_PACKAGE(); this
> is a different quality. Finally, consider the following use case:
> 
> Suppose a package X with a non-(de)selectable core component and an
> additionally selectable component Y; Qt's core module is a perfect
> candidate for the former. If FIND_PACKAGE(X COMPONENTS Y) returns
> with X_FOUND==FALSE and X_Y_FOUND!=TRUE, how do you determine the
> core component's availability, following your favorite approach?
> AFAICS, you can't: The results include "X totally unavailable" as
> well as "X available without Y". Following my favorite approach,
> by contrast, X_FOUND directly indicates the availability of the
> core component. That's an example for the neccessity to reliably
> detect a package's total presence/absence, and a multi-component
> package needs not to be a pure additive entity made of its parts.
> 
> My main conclusion from the above-noted mess among CMake's current
> component-aware find modules is that we urgently need a convention
> how such modules and config files are intended to work. Hopefully,
> we can take a step forward; Qt5's advent is a good opportunity.

I agree.

Alex