Include a module for finding qml imports as runtime dependencies
ClosedPublic

Authored by apol on Aug 3 2017, 2:09 PM.

Details

Summary

Allows to check if a module is available on the system and sets it as a
runtime dependency.
This is useful for projects so that they can specify their qml dependencies
easily and packagers and developers get to see what's missing by looking
at the cmake output.

Diff Detail

Repository
R240 Extra CMake Modules
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apol created this revision.Aug 3 2017, 2:09 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptAug 3 2017, 2:09 PM
aacid added a subscriber: aacid.Aug 3 2017, 6:32 PM

Guess it would be useful, i'm 68% sure https://bugs.kde.org/show_bug.cgi?id=383065 is because packaging...

dfaure added a subscriber: dfaure.Aug 7 2017, 10:36 PM

Would that help with doing things right for the issue in https://phabricator.kde.org/D6466 as well?

apol added a comment.Aug 8 2017, 2:04 AM

Would that help with doing things right for the issue in https://phabricator.kde.org/D6466 as well?

No, I don't think so. This is mostly for packagers who need to document dependencies.

sitter edited edge metadata.Aug 8 2017, 2:12 PM

qmlplugindump not being found needs to be handled somehow. Other than that only minor nitpicks.

(as always I'd also be more confident if it had a test case ;))

modules/ECMFindQMLModule.cmake.in
31

Not sure if we have a common practice for this, but I am thinking this needs to have a find_program() and give suitable output if qmlplugindump itself cannot be found. Pointing the user towards qtdeclarative being needed to check qml dependencies.

modules/ECMQMLModules.cmake
15

If I read the code correctly it takes vargs equal to find_package, may be worth mentioning.

61

Description should probably mention that this is a qml module.

"QML module ${MODULE_NAME} is a runtime dependency" or something like that.

apol updated this revision to Diff 17911.Aug 9 2017, 2:06 AM
apol marked 3 inline comments as done.

Address sitter's comments

modules/ECMFindQMLModule.cmake.in
31

Doing find_program for now. The right fix would be to change Qt to export the qmlplugindump target.

sitter added inline comments.Aug 9 2017, 8:50 AM
modules/ECMFindQMLModule.cmake.in
31

You still need to do "something" if qmlplugindump isn't found. Print a message(WARNING and/or add_feature_info or something similar.
Right now the code would mark the plugin not found but not tell the user that the reason it was not found is that the helper is missing.

e.g. here kirigami is actually installed but qmlplugindump is not:

 10:45:09  /tmp/t/b 
$ cmake ..
-- The C compiler identification is GNU 5.4.0
-- The CXX compiler identification is GNU 5.4.0
-- Check for working C compiler: /usr/lib/icecc/bin/gcc
-- Check for working C compiler: /usr/lib/icecc/bin/gcc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/lib/icecc/bin/g++
-- Check for working CXX compiler: /usr/lib/icecc/bin/g++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- qmlplugindump failed for org.kde.kirigami.
-- Could NOT find org.kde.kirigami-QMLModule (missing:  org.kde.kirigami-QMLModule_FOUND) 
-- Configuring done
-- Generating done
-- Build files have been written to: /tmp/t/b

 10:45:12  /tmp/t/b 
$ cat ../CMakeLists.txt 
project(test)
cmake_minimum_required(VERSION 3.5)

find_package(ECM 1.3.0 REQUIRED NO_MODULE)
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${ECM_MODULE_PATH})

include(ECMQMLModules)
ecm_find_qmlmodule(org.kde.kirigami 2.1)
apol updated this revision to Diff 17922.Aug 9 2017, 10:31 AM

if no qmlplugindump

sitter accepted this revision.Aug 9 2017, 11:04 AM

Oh it's gorgeous!

This revision is now accepted and ready to land.Aug 9 2017, 11:04 AM
This revision was automatically updated to reflect the committed changes.