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

[Stephen Kelly]

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?

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

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

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

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

?

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

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

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

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

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

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

Thanks again for your insights here!

Steve.