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

[Michael Hertling]

On 02/26/2012 11:24 AM, Alexander Neundorf wrote:
> On Sunday 26 February 2012, Michael Hertling wrote:
>> 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.
> 
> I think currently cmFindPackage.cxx in Config mode doesn't evaluate the per 
> component variables at all to decide whether the package has been found or 
> not, which may be required for the next point:
>  
>>> * 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.
> 
> Yes.
> 
>>> * 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().
> 
> Yes. But other resulting variables are also not reset by find_package() in 
> case of failure. So I wouldn't see this a problem.

The fact that other variables remain untouched as well in this case
doesn't make things better; you'd still query variables which are to
be provided by the config file but have not been assigned a definite
value by the latter. IMO, that's bad style, makes the configuration
unneccessarily vulnerable and means asking for trouble in the long
term.

BTW, if FOO was a single-component package, one wouldn't use any of
its typical variables for any purpose after FIND_PACKAGE() returned
with FOO_FOUND==FALSE. Multi- and single-component packages should
behave the same in this regard, as well as find modules and config
files. FOO_FOUND's only consistent interpretation w.r.t. this is:
FOO_FOUND==FALSE --> Hands off, don't use the package in any way;
in particular, don't refer to any of the package's variables.

>>> 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.
> 
> I would expect that if I don't specify any (subset of) components, it will 
> search for all, FOO_FOUND is set to TRUE if a "sufficient" subset of 
> components has been found.
> "sufficient" would be package-specific.
> By explicitely adding components, they become part of the set of components 
> which is considered sufficient.

A casual question: With that reading, how would you figure out if
the package is actually not present, i.e. how can you distinguish
the cases "package entirely unavailable" and "sufficient subset of
components not entirely available"? FOO_FOUND doesn't suit anymore
for this purpose, so you would need to check each of the sufficient
subset's components, right? Moreover, if the unrequested components
are searched tacitly, you'd need to check them, too. Do you know all
components of a package in advance? Recall Qt5 with module XYZ added
later. So, you can't determine reliably if a multi-component package
is actually absent or if merely some of its components are missing.

>> (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.
> 
> Here's the difference, I would expect this.
> 
> The difference boils down to whether the components are AND- or OR combined:
> FOO_FOUND = COMP1 && COMP2 && COMP3
> vs.
> FOO_FOUND = COMP1 || COMP2 || COMP3
> 
> For REQUIRED we agree that we want the conjunction.
> For not REQUIRED, I would want the same behaviour, but without resulting in an 
> error if it was not found.

No, you've got me wrong: In my notion, FOO_FOUND - as set by FIND_
PACKAGE() - is *never* a boolean function of the components' FOUND
variables; it's even completely independent from the components in
all respects. Instead, it makes a statement about a package's basic
availability. The REQUIRED flag just introduces a kind of tri-state
logic with the states FOO_FOUND==TRUE, FOO_FOUND==FALSE and "bailed
out", and in the latter case, the value of FOO_FOUND doesn't matter
at all. In other words: If FIND_PACKAGE(FOO REQUIRED ...) bailed
out, it did not do so because FOO_FOUND would have intermediately
turned out to be FALSE. OTOH, if FIND_PACKAGE() returns regularly,
FOO_FOUND is an independent piece of information. Therefore, e.g.,
one can use it to detect the package's presence/absence reliably.
That's subtly different from FOO_FOUND=f(FOO_<CMPNT_i>_FOUND).

>> (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. 
> 
> Yes. If I can deal with having found only a subset of the listed components, I 
> can check the per-component FOUND variables.

This means that in CMake's configuration log, there'd be lines like

-- Found FOO...

and

-- Could NOT find FOO...

next to each other although everything is fine and the results are
perfectly usable. Following your preferred approach, a find module /
config file can not decide if FOO_FOUND==FALSE is bad or acceptable.
I.e., one would have to reconsider the messaging to the user, think
of FPHSA(). Personally, I'd dislike to see the latter message even
though FOO is available and functioning.

>>     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.
> 
> Yes, as written above, that's what I expect.
> Naming the components is then a shortcut, instead having to check all separate 
> FOUND variables, I can simply check the global one.

As far as I'm concerned, a find module can readily search for un-
requested components on its own behalf. However, there might be
reasons to not do so:

- Usually, searching a component is cheap but might be expensive
  in certain cases, so find modules shouldn't be generally - i.e.,
  by convention - obliged to search all of a package's components.

- The handling of REQUIRED and QUIET must be different for requested
  and unrequested components: For the latters, QUIET must apply and
  REQUIRED must not, regardless if these flags have been specified
  in the FIND_PACKAGE() invocation. This makes it a bit harder to
  design find modules and config files; think of FPHSA() again if
  it is also used to setup the components' FOUND variables.

For these reasons, I usually search components in find modules only
if they have been requested explicitly or are immediate or mediate
prerequisites of the requested ones. Accompanying, I urge the users
to explicitly request all components going to be used, and to refer
only to components having been requested.

>> 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.
> 
> In general it would be not set, i.e. FALSE.
>  
>> 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)? 
> 
> I did not say at all that (2) is bad. I said both a reasonable.
> 
>> 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
> 
> See, here's the difference again.
> How I understand it, to do this, you would have to specifiy only those 
> components which are required for you. All others are searched by default too:
> 
> find_package(X REQUIRED COMPONENTS Y1 Y2)
> 
> if(X_Y3_FOUND)
>     # Enable Y3.
> endif()
> if(X_Y4_FOUND)
>     # Enable Y4.
> endif()
> if(X_Y5_FOUND)
>     # Enable Y5.
> endif()
> 
> 
> If all are optional, do:
> 
> find_package(X)
> 
> if(X_Y1_FOUND)
>    ...
> if(X_Y2_FOUND)
>    ...
> if(X_Y3_FOUND)
>    ...

W.r.t. the set-up of the component-specific FOUND variables, it might
be possible to not specify optional components in FIND_PACKAGE(), but
it isn't in another regard, and your above-noted approach would fail:
If you want to use optional components but don't mention them in the
FIND_PACKAGE() call, the find module / config file cannot add these
components' settings to the package's _LIBRARIES and _INCLUDE_DIRS
variables. Example: Package X with library-type components A and B;
suppose you want to use both, but B is optional. Following your
favorite, you would essentially issue:

FIND_PACKAGE(X REQUIRED A)
IF(X_B_FOUND)
    # Enable B.
ENDIF()

How does X_B_LIBRARY get added to X_LIBRARIES, and X_B_INCLUDE_DIR to
X_INCLUDE_DIRS? The find module / config file has no chance to do so,
regardless if B is searched unaskedly. Adding X_B_{LIBRARY,INCLUDE_
DIR} to X_{LIBRARIES,INCLUDE_DIRS} later is neither the job of the
user nor is it reliable because B might have prerequisites in- or
outside the package. Instead, it is one of the major duties of the
find module / config file to add everything to the package-related
result variables which is necessary to make use of the desired set
of components. Evidently, unrequested ones can not be taken into
account, so you must explicitly request each one you're about to
use, be it optional or not, be it searched unaskedly or not, un-
less it is tacitly enabled by default.

> Maybe to sum (1) up again:
> 
> (a) all components are searched, and each X_Y_FOUND is set accordingly
> 
> (b) there is a package-specific default subset of these components, which have 
> to be found to make the package found, i.e. FOO_FOUND=TRUE.
> 
> (c) by adding COMPONENTS to the find_package() call, these components are 
> added to the set of components which have to be found to make FOO_FOUND=TRUE
> 
> (d) if REQUIRED is used and FOO_FOUND is false, it errors out
> 
> 
> How would that look like exactly for (2) ?

(2a) At least, the requested components along with their mediate and
immediate prerequisites are searched, and the respective *_*_FOUND
variables are set. The access to these variables must be protected
by the *_FOUND variable in case the package is missing as a whole.

(2bc) There may be a set of components which are searched by default,
and a find module may search all of a package's components, not just
the requested ones. However, this allows only for the set-up of the
components' *_*_FOUND variables; if a component is to be enabled,
it must still be explicitly requested in the FIND_PACKAGE() call.

(2d) If REQUIRED is specified, a find module / config file must bail
out if the package as a whole or a requested component or a mediate
or immediate prerequisite of a requested component is missing. The
REQUIRED flag must not be applied to unrequested components which
are searched on the find module's own behalf. If REQUIRED is not
specified, the *_FOUND variable indicates the basic presence of
the package but does not refer to any components; the latters'
presence is indicated by their respective *_*_FOUND variable.

A further important item w.r.t. multi-component packages is:

(2e) Find modules / config files suit perfectly to take possibly
subtle inter-component relations into account that may influence the
package's typical result variables like *_LIBRARIES or *_DEFINITIONS.
Thus, components that are to be used together must also be requested
together in order to permit the find module / config file to fully
consider their relations. In turn, find modules / config files must
provide exactly the settings which are neccessary to enable the set
of requested components.

Finally, correct me if I'm wrong, one of our discussion's main topics
is the following question: After FIND_PACKAGE() has returned, are you
really willing to query variables meant to indicate the package's and
its components' presence but that might - or actually - have not been
assigned a definite value by the find module / config file? As to me,
I'm rather unwilling to do so. Instead, I expect that each suchlike
variable queried after FIND_PACKAGE()'s return has received a value
during that command's execution. This very reason already makes me
to prefer approach (2), i.e. (2a-e). IMO, it is quite clean, safe
and consistent, somewhat easy and also equipped to handle common
as well as not-so-common set-ups.

Regards,

Michael