D7094: Include a module for finding qml imports as runtime dependencies
This revision was automatically updated to reflect the committed changes. Closed by commit R240:e5301edf1daf: Include a module for finding qml imports as runtime dependencies (authored by apol). REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7094?vs=17922&id=17924 REVISION DETAIL https://phabricator.kde.org/D7094 AFFECTED FILES modules/ECMFindQMLModule.cmake.in modules/ECMQMLModules.cmake tests/CMakeLists.txt To: apol, #build_system, #frameworks, sitter Cc: dfaure, aacid
D7094: Include a module for finding qml imports as runtime dependencies
sitter accepted this revision. sitter added a comment. This revision is now accepted and ready to land. Oh it's gorgeous! REPOSITORY R240 Extra CMake Modules BRANCH master REVISION DETAIL https://phabricator.kde.org/D7094 To: apol, #build_system, #frameworks, sitter Cc: dfaure, aacid
D7094: Include a module for finding qml imports as runtime dependencies
apol updated this revision to Diff 17922. apol added a comment. if no qmlplugindump REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7094?vs=17911&id=17922 BRANCH master REVISION DETAIL https://phabricator.kde.org/D7094 AFFECTED FILES modules/ECMFindQMLModule.cmake.in modules/ECMQMLModules.cmake tests/CMakeLists.txt To: apol, #build_system, #frameworks, sitter Cc: dfaure, aacid
D7094: Include a module for finding qml imports as runtime dependencies
sitter added inline comments.
INLINE COMMENTS
> apol wrote in ECMFindQMLModule.cmake.in:30
> Doing find_program for now. The right fix would be to change Qt to export the
> `qmlplugindump` target.
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)
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
https://phabricator.kde.org/D7094
To: apol, #build_system, #frameworks, sitter
Cc: dfaure, aacid
D7094: Include a module for finding qml imports as runtime dependencies
apol updated this revision to Diff 17911. apol marked 3 inline comments as done. apol added a comment. Address sitter's comments REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D7094?vs=17628&id=17911 BRANCH master REVISION DETAIL https://phabricator.kde.org/D7094 AFFECTED FILES modules/ECMFindQMLModule.cmake.in modules/ECMQMLModules.cmake tests/CMakeLists.txt To: apol, #build_system, #frameworks, sitter Cc: dfaure, aacid
D7094: Include a module for finding qml imports as runtime dependencies
apol added inline comments. INLINE COMMENTS > sitter wrote in ECMFindQMLModule.cmake.in:30 > 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. Doing find_program for now. The right fix would be to change Qt to export the `qmlplugindump` target. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D7094 To: apol, #build_system, #frameworks, sitter Cc: dfaure, aacid
D7094: Include a module for finding qml imports as runtime dependencies
sitter added a comment.
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 ;))
INLINE COMMENTS
> ECMFindQMLModule.cmake.in:30
> +
> +execute_process(COMMAND qmlplugindump "@MODULE_NAME@" "@VERSION@"
> ERROR_VARIABLE ERRORS_OUTPUT OUTPUT_VARIABLE DISREGARD_VARIABLE
> RESULT_VARIABLE ExitCode)
> +
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.
> ECMQMLModules.cmake:14
> +# ::
> +# ecm_find_qmlmodule( )
> +#
If I read the code correctly it takes vargs equal to `find_package`, may be
worth mentioning.
> ECMQMLModules.cmake:60
> +set_package_properties(${GENMODULE} PROPERTIES
> +DESCRIPTION "${MODULE_NAME} is a runtime dependency"
> +TYPE RUNTIME)
Description should probably mention that this is a qml module.
"QML module ${MODULE_NAME} is a runtime dependency" or something like that.
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
https://phabricator.kde.org/D7094
To: apol, #build_system, #frameworks, sitter
Cc: dfaure, aacid
D7094: Include a module for finding qml imports as runtime dependencies
apol added a comment. In https://phabricator.kde.org/D7094#133579, @dfaure wrote: > 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. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D7094 To: apol, #build_system, #frameworks, sitter Cc: dfaure, aacid
D7094: Include a module for finding qml imports as runtime dependencies
dfaure added a comment. Would that help with doing things right for the issue in https://phabricator.kde.org/D6466 as well? REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D7094 To: apol, #build_system, #frameworks, sitter Cc: dfaure, aacid
D7094: Include a module for finding qml imports as runtime dependencies
aacid added a comment. Guess it would be useful, i'm 68% sure https://bugs.kde.org/show_bug.cgi?id=383065 is because packaging... REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D7094 To: apol, #build_system, #frameworks, sitter Cc: aacid
D7094: Include a module for finding qml imports as runtime dependencies
apol created this revision. Restricted Application added projects: Frameworks, Build System. REVISION 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. REPOSITORY R240 Extra CMake Modules BRANCH master REVISION DETAIL https://phabricator.kde.org/D7094 AFFECTED FILES modules/ECMFindQMLModule.cmake.in modules/ECMQMLModules.cmake To: apol, #build_system, #frameworks, sitter
