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

[Alexander Neundorf]

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.
 
> > 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.

> (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.


> (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.

>     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.

> 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)
   ...



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) ?

Alex