----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119901/#review65382 -----------------------------------------------------------
Nice! See my comments below, though. kde-modules/KDEInstallDirs.cmake <https://git.reviewboard.kde.org/r/119901/#comment45693> Remove this TODO, since you've done it. kde-modules/KDEInstallDirs.cmake <https://git.reviewboard.kde.org/r/119901/#comment45690> Move the include to inside the if(KDE_ECM_INSTALL_TO_QT_SYS_DIR), so that it doesn't try to find qmake if it's not set. kde-modules/KDEInstallDirs.cmake <https://git.reviewboard.kde.org/r/119901/#comment45692> I don't think you need both KDE and ECM there. KDE alone is sufficient - that is the prefix of the module name, after all. It ends up a bit odd in the ECMGeneratePriFile module, but I don't like the fact we set an install variable in there anyway. Actually, I would call it KDE_INSTALL_USE_QT_SYS_PATHS. modules/ECMGeneratePriFile.cmake <https://git.reviewboard.kde.org/r/119901/#comment45691> Similarly, this should be inside the conditional. modules/ECMQueryQmake.cmake <https://git.reviewboard.kde.org/r/119901/#comment45689> I think we need to be a bit more clever here. I'd go for a cache variable, and if (TARGET Qt5::qmake), set the default to that, otherwise set the default to just "qmake". modules/ECMQueryQmake.cmake <https://git.reviewboard.kde.org/r/119901/#comment45688> CMake macros/functions that take a variable name usually take it first. Swapping the argument order would make it more consistent with other things. - Alex Merry On Aug. 27, 2014, 1:11 p.m., Rohan Garg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119901/ > ----------------------------------------------------------- > > (Updated Aug. 27, 2014, 1:11 p.m.) > > > Review request for Build System and KDE Frameworks. > > > Repository: extra-cmake-modules > > > Description > ------- > > Use qmake to query dirs for plugins and imports instead of hardcoding them in > ECM. > > > Diffs > ----- > > modules/ECMQueryQmake.cmake PRE-CREATION > kde-modules/KDEInstallDirs.cmake 880539b > modules/ECMGeneratePriFile.cmake 34001d6 > > Diff: https://git.reviewboard.kde.org/r/119901/diff/ > > > Testing > ------- > > Seems to work on my system. > > > Thanks, > > Rohan Garg > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel