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

[Michael Hertling]

On 03/05/2012 01:35 AM, Stephen Kelly wrote:
> Michael Hertling wrote:
> 
>> Suppose the Qt folks decide that Qt5's core module doesn't need to
>> be explicitly requested because it is prerequisite for everything
>> else.
> 
> Just to disseminate information here, this is already the case.
> 
> You can currently do this:
> 
> find_package(Qt5Declarative)
> include_directories(${Qt5Declarative_INCLUDE_DIRS})
> add_definitions(${Qt5Declarative_DEFINITONS})
> target_link_libraries(foo ${Qt5Declarative_LIBRARIES})
> 
> Each find_package finds its dependencies and adds its dependencies values to 
> its own variables. So already, the core module (and in the above example, 
> the gui module) don't need to be explicitly mentioned.

That's not what I had in mind. AFAICS, the Qt5 modules' config files
are all single-component, and of course, they are expected to enable
all prerequisites necessary to enable their respective module.

What I thought of is: Suppose there's a comprehensive multi-component
Qt5Config.cmake which does not provide a Qt5Core component explicitly
because the latter is silently enabled anyway. So, FIND_PACKAGE(Qt5)
would yield solely Qt5Core, and FIND_PACKAGE(Qt5 COMPONENTS Qt5Gui)
would yield Qt5Gui and Qt5Core, e.g. Now, if Qt5_FOUND is allowed to
be FALSE just because Qt5Gui is missing, one can't use this variable
anymore to determine Qt5Core's availability; a totally unavailabe Qt5
would yield the same result. For a project that does need Qt5Core and
can use Qt5Gui - if available - this is crucial, but a reliable check
for the presence of Qt5Core would require further measures. However,
Alex recently came up with a proposal to distinguish mandatory from
optional components already in the FIND_PACKAGE() invocation, and
this seems capable to address that issue.

> This is one of the things I'd like feedback on, and on of the reasons I'm 
> asking people to try this out, or to read the generated Config files.
> 
> Can anyone say they've read the generated files? Has anyone confirm they 
> have run this or something like it? :
> 
> git clone git://gitorious.org/qt/qtbase.git
> cd qtbase
> ./configure
> ls lib/cmake

Yes. :)

> To see the input files for the generated config files see:
> 
> https://qt.gitorious.org/qt/qtbase/blobs/master/mkspecs/cmake/Qt5BasicConfig.cmake.in

Some remarks, though I haven't dissected each and every detail:

(1) Protecting imported targets: I suppose lines like

if (NOT _Qt5Gui_target)
    set(_Qt5Gui_target 1)
    add_library(Qt5::Gui SHARED IMPORTED)
endif()

serve to protect the Qt5::Gui imported target against redefinition
within the same scope? If so, look at the end of [1]. An inherited
directory property might be a cleaner and slightly more robust
alternative to a - rather fragile - variable:

DEFINE_PROPERTY(DIRECTORY PROPERTY Qt5Gui INHERITED ...)
GET_PROPERTY(p DIRECTORY PROPERTY Qt5Gui SET)
IF(NOT p)
    SET_PROPERTY(DIRECTORY PROPERTY Qt5Gui TRUE)
    ADD_LIBRARY(Qt5::Gui SHARED IMPORTED)
ENDIF()

Anyway, a minor issue and possibly not worth the effort.

(2) Enabling prerequisite Qt5 modules:

foreach(_module_dep ${_Qt5_MODULE_DEPENDENCIES})
    if (NOT Qt5${_module_dep}_FOUND)
        find_package(Qt5${_module_dep} REQUIRED)
    endif()
    ....
endforeach()

Basically, the information provided by FIND_PACKAGE() here is already
available at configuration time, so there's no actual need to query it
at, say, deploy time. Instead, it could be weaved immediately into the
config file. However, doing it in the above-noted manner might be very
well intended, e.g. to keep the config files compact and regular and
to straightly express the modules' dependencies. If one decides to
do so, one should consider to use:

FIND_PACKAGE(Qt5${_module_dep} REQUIRED PATHS ... NO_DEFAULT_PATH)

Otherwise, FIND_PACKAGE() might load a config file from another Qt5
installation due to its fine-grained parameterization by variables
like CMAKE_PREFIX_PATH, in the cache as well as the environment.

(3) Enabling prerequisite external packages:

!!IF contains(QT_CONFIG, system-png)
find_package(PNG REQUIRED)
list(APPEND Qt5Gui_LIB_DEPENDENCIES ${PNG_LIBRARIES})
!!ENDIF

A similar consideration as in (2), but note that PNG does not have a
config file, so the PATHS/NO_DEFAULT_PATH interception will not work.
Is it permissible that a binary using Qt5Gui might be built against
a different PNG library than Qt5Gui has been built against? If so,
you could perhaps think about REQUIRED: IMO, FIND_PACKAGE() called
without that flag must not bail out because of anything not being
found. Instead, the user must have the chance to handle this by him/
herself. Note that Qt5 might be optional for the user's project. So,
REQUIRED should be passed to the internal FIND_PACKAGE() invocations
only if Qt5*_FIND_REQUIRED is set, and the same consideration should
hold for the QUIET flag.

(4) Potential Qt5*_LIB_DEPENDENCIES accumulation:

AFAICS, these variables aren't initialized by SET(), and there is
also no LIST(REMOVE_DUPLICATES ...) invocation. Wouldn't they add
up if FIND_PACKAGE(Qt5*) is called multiple times? E.g., wouldn't

FIND_PACKAGE(Qt5Widgets)
FIND_PACKAGE(Qt5Gui)

provide for two FIND_PACKAGE(Qt5Gui) invocations ending up with

Qt5Gui_LIB_DEPENDENCIES==Qt5::Core;Qt5::Core

? Shouldn't these variables be compacted, too? Besides, enabling
Qt5Gui before Qt5Widgets wouldn't exhibit that behavior, right?

A final remark: If you retain FIND_PACKAGE() invocations in config
files as in (2,3), you should be prepared that they fail - w.r.t.
internal as well as external prerequisites. If they fail and you
pass a REQUIRED flag derived from Qt5*_FIND_REQUIRED - which I'd
recommend - the config file may need to return Qt5*_FOUND==FALSE
due to missing prerequisites. For this, you need the other patch
Alex has proposed, i.e. setting up an appropriate *_FOUND value
is the responsibility of the config files' authors. In short:
FIND_PACKAGE() in config files has certain implications. ;)

Regards,

Michael

[1] http://www.mail-archive.com/cmake@cmake.org/msg35873.html