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

[Michael Hertling]

On 02/25/2012 09:43 AM, Alexander Neundorf wrote:
> On Friday 24 February 2012, Michael Hertling wrote:
>> On 02/24/2012 03:34 PM, Stephen Kelly wrote:
> ...
>>>> [...] (that is, find_package(Qt5 REQUIRED
>>>> Gui Xml) might not find QtXml, but Qt5_FOUND would still be true if the
>>>> Qt5Config file is found, whether the component is or not), [...]
>>
>> No: FIND_PACKAGE(Qt5 REQUIRED ...) is expected to bail out if any of
>> the required components aren't found, so the value of the Qt5_FOUND
>> variable doesn't matter in this case. BTW, note that FIND_PACKAGE()
>> must not bail out if a component which the user hasn't requested is
>> not found, regardless of the REQUIRED flag, unless the component is
>> an immediate or mediate prerequisite of a required one.
>>
>> Regarding Qt5_FOUND, FIND_PACKAGE(Qt5 COMPONENTS ...), i.e. without
>> REQUIRED, is more interesting, see [4]. In short: Qt5_FOUND indicates
>> if Qt5 is basically present but says nothing about any component; the
>> user must refer to the component-specific FOUND variables, and those
>> must even be protected by the package-specific one:
> 
> Ah, yes, I remember this discussion.
> I'd sum up the results as follows:
> 
> * Config-mode should behave the same way as Module-mode with regard to 
> COMPONENTS (I think this still needs changes in the find_package() 
> implementation)

Which changes do you have in mind? AFAIK, config files and find modules
can perfectly behave the same - provided they're implemented correctly.

> * if REQUIRED is used, find_package(Foo COMPONENTS A B C) must only succeed if 
> all listed components have been found

Perhaps, we can also include components which are immediate or mediate
prerequisites of the listed ones. The REQUIRED flag should ensure that
anything necessary to use the listed components is actually available.

> * if REQUIRED is not used, we still have two opinions:
> 
> 1.) FOO_FOUND should be the major sign whether everything I wanted has been 
> found or not. I.e. it is only set to TRUE if all listed components have been 
> found. To check whether some of the components have been found, i.e. if 
> FOO_FOUND==FALSE, I can check the per-component _FOUND variables.
> The reasoning here is "I want to use some parts of a big package, if they all 
> are found, then I can use it, otherwise I can't use the package at all"

If FOO_FOUND==FALSE and FIND_PACKAGE() did load a config file, none of
the per-component _FOUND variables has received a definite value, i.e.
you'd refer to the values they already had before the invocation of
FIND_PACKAGE().

> 2.) FOO_FOUND should only indicate that at least something of Foo has been 
> found. To check which modules have been found, i.e. if FOO_FOUND==TRUE, I must 
> check the per-component _FOUND variables.
> The logic here is "I want to use some parts of a big package, and I can use 
> them independently from each other".
> 
> Both make sense.
> I'd prefer the first one.

Presumably, you're not surprised that I move for the second. ;-)
One of my favorite points for the latter is the following use case:

Suppose there's a multi-component package FOO which provides some
required components and some optional ones. The user has three
distinct possibilities how those components can be requested:

(A) Request all of them with the REQUIRED flag: Bad, a missing optional
    component would terminate the configuration although all is well.

(B) Request all of them without the REQUIRED flag: With your preference
    (1), a missing optional component would result in FOO_FOUND==FALSE
    although all is well. As for me, that's not what I'd expect if the
    findings are certainly acceptable.

(C) Request the required components with REQUIRED and the optional ones
    without this flag via a separate FIND_PACKAGE() invocation: If all
    required components are found but an optional component is missing,
    your preference (1) would result in FOO_FOUND==TRUE for the formers
    and FOO_FOUND==FALSE for the latters. That's even more inconsistent
    than (B), although the findings are likewise acceptable. Moreover,
    there might be reasons why one doesn't want to use more than one
    FIND_PACKAGE() call to request components which are going to be
    used together; recall the example of package X with components
    A and B and a possibly necessary -DX_WITH_B definition for A.

As a fourth possibility, one might drop the optional components from
the FIND_PACKAGE() call and rely on the assumption that unrequested
components are searched unaskedly by the config file / find module.
This should not be forced on the latter, and what should happen if
an optional component isn't known at all? Recall the example of Qt5
with a module XYZ added in a later release: An application willing
to use XYZ if available but configured against an earlier release
of Qt5 via FIND_PACKAGE(Qt5) would also refer to a Qt5_XYZ_FOUND
variable which has not - can't have - received a definite value.

IMO, the use case with mixed required and optional components points
out that your preference (1) - one way or the other - tends to yield
strange/surprising/inconsistent results, or to drive the user into
querying variables with non-definite values. That's not the case
with (2) which is quite bullet-proof, if I'm not mistaken.

BTW, what's so bad with (2)? The handling of the possibly numerous
FOUND variables could easily be simplified by some supplementary
functions, e.g. by suitably rewriting the FOUND variables:

FIND_PACKAGE(X COMPONENTS Y1 Y2 Y3 Y4 Y5)
# Y1,2 required, Y3,4,5 optional:
CHECK_PACKAGE_COMPONENTS(X REQUIRED Y1 Y2 COMPONENTS Y3 Y4 Y5)
# X_<i>_FOUND = X_FOUND AND X_<i>_FOUND, i=1,...,5
# X_FOUND = X_FOUND AND X_Y1_FOUND AND X_Y2_FOUND
IF(NOT X_FOUND)
    MESSAGE(FATAL_ERROR ...)
ENDIF()
# Enable Y1,2.
IF(X_Y3_FOUND)
    # Enable Y3.
ENDIF()
IF(X_Y4_FOUND)
    # Enable Y4.
ENDIF()
IF(X_Y5_FOUND)
    # Enable Y5.
ENDIF()

This is pretty straight forward, and if I understand correctly, it
does quite exactly what you intend w.r.t. X_FOUND while taking any
optional components into account. Finally, the implementation of
CHECK_PACKAGE_COMPONENTS() shouldn't be particularly difficult.

>> FIND_PACKAGE(Qt5 COMPONENTS XYZ)
>> IF(Qt5_FOUND AND Qt5_XYZ_FOUND)
>>     ...
>> ENDIF()
>>
>> Referring to Qt5_XYZ_FOUND alone is not reliable because this variable
>> wouldn't have received a definite value if Qt5Config.cmake hasn't been
>> found by FIND_PACKAGE(). I.e., the user would refer to this variable's
>> value before the FIND_PACKAGE() call; probably, that's not expected.
>>
>> Note that you should also think about handling components requested
>> by the user but unknown to Qt5Config.cmake. Consider the following
>> use case: One day, a component - aka module - XYZ is added to Qt5,
>> and there's an application that can make use of it but needn't to:
>>
>> FIND_PACKAGE(Qt5 COMPONENTS XYZ)
>> IF(Qt5_FOUND AND Qt5_XYZ_FOUND)
>>     ....
>> ENDIF()
>>
>> What should happen if this application is configured against a Qt5
>> installation which does not know XYZ yet? IMO, the user can expect
>> that Qt5_XYZ_FOUND receives a definite value during the processing
>> of Qt5Config.cmake in any case. I.e., the Qt5Config.cmake of a Qt5
>> installation unaware of XYZ must nevertheless set Qt5_XYZ_FOUND to
>> FALSE if XYZ has been requested by the user.
>>
>> For the latter, this means to refer only to components which have
>> been requested explicitly with FIND_PACKAGE(), and to not rely on
>> the assumption that any component is searched automatically, but
>> that's another story...
>>
>>>> [...] I'm not sure
>>>> it's a good idea, [...]
>>
>> Personally, I'm quite sure it is.
>>
>>>> [...] or at least it's more complicated. [...]
>>
>> Probably, but I think it's worth the effort, even for the reason alone
>> that a comprehensive Qt5Config.cmake could handle the possibly subtle
>> relations among Qt5 modules far better than distinct module-specific
>> configuration files ever can.
>>
>>>> [...] At least, I think
>>>> something like qt5_use_package is a better idea anyway.
>>
>> First of all, I definitely agree to your criticism of UseQt4.cmake in
>> [5], and I appreciate your attempt to get rid of such a use file for
>> Qt5. However, I'd choose a Qt5Config.cmake over qt5_use_package(),
>> and my current vision for the former's usage is roughly, e.g.,
>>
>> FIND_PACKAGE(Qt5 REQUIRED Xml)
>> SET_TARGET_PROPERTIES(t1 PROPERTIES
>>     COMPILE_DEFINITIONS ${Qt5_DEFINITIONS}
>>     INCLUDE_DIRECTORIES ${Qt5_INCLUDE_DIRS})  # If available one day.
> 
> I think something like this will be in 2.8.8.
> See the target_include_directories() thread on the cmake developers list (but 
> I didn't follow closely).

That'd be great - a long-awaited feature.

Regards,

Michael