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

[Michael Hertling]

On 03/05/2012 02:04 AM, Stephen Kelly wrote:
> 
> Michael Hertling wrote:
> 
>>>> * Currently there is no Qt5Config.cmake.
>>>> Such a thing could probably exist and use the FIND_COMPONENTS to find
>>>> what was requested. [...]
> 
> Hi there,
> 
> Thank you for your insights on this issue. Do you have any other insights 
> into other issues I raised in the original post?

No, I just picked out the Qt5Config.cmake ones. Perhaps later...

>> Absolutely, I would greatly appreciate a well-designed and component-
>> aware Qt5Config.cmake. 
> 
> Yes. This thread confirms though that it is not a simple issue as I wrote 
> before :)

Indeed, component-aware find modules / config files are significantly
more complicated than component-unaware ones. Typical questions are:

- Accumulation of result variables
- Handling of unknown components
- Searching unrequested components
- Interpretation of *_FOUND variable
- Untouched *_*_FOUND variables
- Impact of REQUIRED/QUIET

>> In general, there might be reasons why a multi-
>> component package's components that are to be used together should not
>> be requested in separate FIND_PACKAGE() invocations, see [1] and look
>> for package X with components A and B. However, I don't know if Qt5
>> will be the first example of that kind.
> 
> Your exact example is not covered by the Qt5 situation as far as I can tell. 
> However, similar issues already crop up (with Qt4 based systems). Can you 
> confirm whether you are aware of the issues around code like this regarding 
> the use of -DQT_GUI_LIB with the foo target so I know if I need to explain 
> it and whether we are on the same page? :
> 
> find_package(Qt4 REQUIRED Gui Test)
> include(${QT_USE_FILE})
> add_executable(bar ${QT_QTCORE_LIBRARIES} ${QT_QTGUI_LIBRARIES})
> add_executable(foo ${QT_QTCORE_LIBRARIES} ${QT_QTTEST_LIBRARIES})

Do you mean the appearance of -DQT_GUI_LIB during foo's compilation
although the latter doesn't need it? If so, this is a, say, inverse
version of what I had in mind: An unnecessary -D, possibly enabling
undesired code in headers during the compilation. If not, could you
explain it once more in another way? ;-)

Actually, my general consideration in this regard is: There might be
quite subtle relations among a package's components, beyond the usual
B-needs-A one. Find modules and config files suit perfectly to handle
such relations, but in order to do this, they must be supplied with
sufficient information. So, all components going to be used together
should be requested together, and if one wants to use a different set
of components, one should request them with a separate FIND_PACKAGE()
call. In this way, a find module / config file is equipped to provide
optimal results, i.e. the exact settings to enable the requested set
of components - no more, no less. In fact, settings like QT_GUI_LIB
made me reason about this issue for the first time, though I still
do not know a real-life example for a -D which is related solely
to a combination of components, or anything else of that kind.

>> 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 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, e.g. like:

IF(Qt5_FOUND AND Qt5_Xml_FOUND)

This consideration is one of my major objections against the proposal
w.r.t. permitting config files to set *_FOUND by themselves. As it is
suggested, it would result in *_FOUND set to FALSE if just a component
is missing, so one can not use *_FOUND anymore to detect the package's
presence. Instead, one would need to apply other means, e.g. the *_DIR
variable set by FIND_PACKAGE() in config mode, but that doesn't work in
module mode, AFAIK. Anyway, I am afraid this will complicate the usage
of FIND_PACKAGE() and promote the inconsistencies among find modules
and config files.

>> I.e., the user would refer to this variable's
>> value before the FIND_PACKAGE() call; probably, that's not expected.
> 
> Why would the user refer to Qt5_Xml_FOUND before 
> 
> find_package(Qt5 REQUIRED Xml)
> 
> ?

(S)he wouldn't - bad wording of mine. A better one is: The user would
see the value the variable already had when FIND_PACKAGE() was called.

>>>> [...] 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}
> 
> Again, just to disseminate information, each Qt5<module>Config.cmake file 
> sets Qt5<module>_DEFINITIONS for use with the add_definitions() command, and 
> Qt5<module>_COMPILE_DEFINITIONS for use with the COMPILE_DEFINITIONS 
> property. The difference is that one already contains the '-D' and the other 
> doesn't (and breaks if it is present).
> 
> So, in your example, that would be ${Qt5_COMPILE_DEFINITIONS} if a 
> Qt5Config.cmake exists some day.

This *_COMPILE_DEFINITIONS flavor is great and should be added to
Modules/readme.txt as a recommendation next to the *_DEFINITIONS.

>>     INCLUDE_DIRECTORIES ${Qt5_INCLUDE_DIRS})  # If available one day.
> 
> The patches for this are in next but didn't get merged to master yet. It 
> should indeed be part of CMake 2.8.8 and function similar to what you write 
> here.

That's even greater. :) Limiting the effects of INCLUDE_DIRECTORIES()
is an issue with multiple targets defined in the same CMakeLists.txt.

>> TARGET_LINK_LIBRARIES(t1 ${Qt5_LIBRARIES})
>>
>> FIND_PACKAGE(Qt5 REQUIRED Sql)
>> SET_TARGET_PROPERTIES(t2 PROPERTIES
>>     COMPILE_DEFINITIONS ${Qt5_DEFINITIONS}
>>     INCLUDE_DIRECTORIES ${Qt5_INCLUDE_DIRS})  # If available one day.
>> TARGET_LINK_LIBRARIES(t2 ${Qt5_LIBRARIES})
>>
>> with t2 not being overlinked due to accumulated results.
> 
> Yes, accumulation versus replacement for the values of all the variables set 
> by Qt5Config.cmake is also a complication that it introduces.

IMO, one should pay strict attention that the typical uncached result
variables like *_LIBRARIES don't accumulate. Otherwise, linking more
than one target against different sets of components within the same
CMakeLists.txt becomes an annoyance.

>> Some spontaneous questions/remarks on {cmake,qt5}_use_package():
>>
>> - Do they need to be macros?
> 
> You mean as opposed to functions? I think they could be functions instead.
> 
>> - What if "_package" is a multi-component one?
> 
> The macro as it is currently written (in a unit test in the Qt5 repo) 
> guardss the check for ${_package} with ${${_package}_FOUND}. My assumption 
> was that the convention was that if ${${_package}_FOUND} is true it doesn't 
> need to be found again, so in special cases where components need to be 
> specified, you would simply invoke find_package before the cmake_use_package 
> call:
> 
> cmake_use_package(t1 PNG)
> find_package(Foo REQUIRED Xxx Yyy)
> # Foo variables contain the requirements for Xxx and Yyy too.
> cmake_use_package(t1 Foo) # Doesn't find Foo again, but uses the variables.
> 
> This thread indicates that that is not as conventional as I thought, but 
> this feature could be used to define what the convention should be as well 
> as create the incentive to create the convention.

OK, the components' stuff would simply pass thru via the package's
result variables. Is there a chance that components are optional?

# Use Xxx/Yyy if they're available:
find_package(Foo COMPONENTS Xxx Yyy)
# Suppose Foo_Xxx_FOUND==TRUE, Foo_Yyy_FOUND==FALSE.
# Foo variables contain settings for Xxx but not for Yyy.
cmake_use_package(t1 Foo) # Might search Foo again without Xxx/Yyy.

If Foo_FOUND is allowed to be FALSE because Yyy isn't found while Xxx
is, this would result in cmake_use_package() invoking find_package()
again, but without any components, AFAICS. With Foo_FOUND==TRUE, you
just use each component that has been found. Thus, if Foo is a multi-
component package, you must enter cmake_use_package() with Foo_FOUND
==TRUE. Otherwise, the internal find_package() call wouldn't add the
components' settings to Foo_LIBRARIES et al. Is it possibly cleaner
to require Foo_FOUND==TRUE instead of invoking find_package()? This
also addresses the issue w.r.t. REQUIRED: Should cmake_use_package()
be allowed to bail out without a possibility for the user to control
this behavior? Just spontaneous thoughts; I haven't inspected CUP()
in particular detail.

>> - What about COMPILE_DEFINITIONS_<CONFIG> properties?
> 
> to be decided :) along with include directories for different configs (which 
> don't even have a syntax for representation in CMake yet, but it's not going 
> to be INCLUDE_DIRECTORIES_<CONFIG>) etc too.
> 
>> - Target property INCLUDE_DIRECTORIES coming up in 2.8.8?
> 
> Yep.
> 
>> - Using SET_PROPERTY(... APPEND_STRING ...) on COMPILE_FLAGS?
> 
> That doesn't account for duplicates.
> 
>> - Do
>>
>>   list(APPEND _flags ...)
>>   ...
>>   set_target_properties(... PROPERTIES COMPILE_FLAGS ${_flags})
>>
>>   not provide for the semicolons that are undesired in COMPILE_FLAGS?
> 
> Not according to my experiments so far.

Look at the following CMakeLists.txt:

CMAKE_MINIMUM_REQUIRED(VERSION 2.8 FATAL_ERROR)
PROJECT(P C)
SET(CMAKE_VERBOSE_MAKEFILE ON)
FILE(WRITE ${CMAKE_BINARY_DIR}/main.c "int main(void){return 0;}\n")
ADD_EXECUTABLE(main main.c)
SET(flags "-O1")
LIST(APPEND flags "-O2")
LIST(APPEND flags "-O3")
MESSAGE("flags: ${flags}")
SET_TARGET_PROPERTIES(main PROPERTIES COMPILE_FLAGS ${flags})

Configuration yields

flags: -O1;-O2;-O3

as expected, but Make's output reveals:

[100%] Building C object CMakeFiles/main.dir/main.c.o
.../gcc -O1 -o CMakeFiles/main.dir/main.c.o -c .../main.c

I.e., only the first item in the list is used for compilation -
strange and wrong, but at least the semicolons don't riot. ;-)

>>>> We could have a small FindQt5.cmake in CMake which could do that however
>>>> maybe.
>>
>> Usually, a find module is more complicated than a config file because
>> the latter "knows" its package - along with information about static/
>> shared libraries, infixes, namespaces etc. ;-) - whereas the former
>> needs to figure out such things on its own during the search.
> 
> I think Qt5Config or FindQt5 would be special anyway because it is only an 
> aggregator of some kind or a 'virtual package with dependencies'. Even if 
> Qt5_FOUND and Qt5_Core_FOUND and Qt5_Gui_FOUND etc are defined, 
> Qt5Core_FOUND and Qt5Gui_FOUND would also be defined because of the calls to 
> find_package(Qt5Core) etc. Already that kind of variable duplication which 
> the Qt5Config for the purpose of components introduces isn't too nice I 
> think.

Do you mean {FindQt5,Qt5Config}.cmake as a wrapper around multiple
FIND_PACKAGE(Qt5xyz ...) invocations that concatenates the several
Qt5xyz_LIBRARIES to yield the final Qt5_LIBRARIES, e.g.? If so, a
possible objection that comes to my mind is that

FIND_PACKAGE(Qt5 COMPONENTS Gui Xml Svg)

ends up with Qt5Gui twice and Qt5Core four times in Qt5_LIBRARIES.
Something similar holds for -D/-I unless {FindQt5,Qt5Config}.cmake
sorts it out. This prolongs link times - although I don't know if
significantly or negligibly - and might be an issue w.r.t. to the
maximal command line length. Once more, just spontaneous thoughts,
nothing definitive. In general, however, it's easier for a, say,
integral {FindQt5,Qt5Config}.cmake to provide optimal results.

What I actually meant with regard to a FindQt5.cmake shipped with
CMake: This would need to cope with several Qt5 installations, and
these might be quite different, think of cross compiling issues. In
contrast, a Qt5Config.cmake is part of the respective installation
and must cope only with the latter. Thus, it is potentially less
complicated than a FindQt5.cmake would need to be.

Personally, I'd be in favor of an integral Qt5Config.cmake.

Regards,

Michael