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

[Michael Hertling]

On 03/10/2012 02:25 PM, Alexander Neundorf wrote:
> On Friday 09 March 2012, Michael Hertling wrote:
>> On 03/05/2012 02:04 AM, Stephen Kelly wrote:
> ...
>>> I don't actually see the problem with checking Qt5_XYZ_FOUND. Unset
>>> variables are well defined as false in the if() command. Maybe I
>>> misunderstand you?
>>
>> Maybe. ;-) What ensures the variables had already been unset before
>> FIND_PACKAGE() was invoked? If they had evaluated as TRUE - for what-
>> ever reason - and FIND_PACKAGE() fails to load Qt5Config.cmake, they
>> will still be TRUE afterwards although Qt5 has not been found at all.
>>
>> IMO, if FIND_PACKAGE() fails to locate a package, one shouldn't rely
>> on the assumption that any variable except *_FOUND has a reasonable
>> value. Thus, in order to play safe, one should access the variables
>> only after checking the package's successful detection, 
> 
> Yes, exactly :-)

It's very good there's a consensus in this respect. ;-)

> (...which would be the case with the "set _FOUND only TRUE if all components 
> have been found).

Only in the successful case; in the failing case, *_FOUND would not
provide any information about the package's presence. Anyway, your
proposal stated below is capable to address this issue, AFAICS.

>> e.g. like:
>>
>> IF(Qt5_FOUND AND Qt5_Xml_FOUND)
> 
> See the other thread
> With a potential OPTIONAL_COMPONENTS parameters you could do:
> 
> find_package(Qt5 COMPONENTS QtXml)
> and checking Qt5_FOUND would be enough.
> 
> If you do 
> find_package(Qt5 COMPONENTS QtXml OPTIONAL_COMPONENTS QtFoo)
> you would have to check both Qt5_FOUND and Qt5_QtFoo_FOUND.

This sounds quite attractive; I'll answer to it on the other thread.

>> This consideration is one of my major objections against the proposal
>> w.r.t. permitting config files to set *_FOUND by themselves. 
> 
> Here I object.
> This is necessary.
> Let's say one Config file depends on another one and does
> find_package(TheOtherPackage NO_MODULE)
> 
> If TheOtherPackage is not found, it must be possible for the calling Config 
> file to indicate that, although the Config file has been found, the package is 
> not usable. because its dependencies have not been found.

This is a very good point. Until now, I presumed that a config file
contains the necessary information about its package's prerequisites
in a hard-coded manner, so it does not need to invoke FIND_PACKAGE().
Usually, that's possible since the information is available already
at configuration time. Allowing config files to call FIND_PACKAGE()
would surely add a certain flexibility, but it also bears a risk: A
successfully configured/built/installed package might fail at deploy
time because of an unavailable prerequisite that has been available
once before. If one is generally aware of this risk and willing to
accept it, I'm fine with it, too.

One or two thoughts and another cup of coffee later, I think that
allowing a config file to set *_FOUND by itself can actually be a
benefit, so my concerns about that are pretty much dispelled. :-)

Regards,

Michael