Re: Review Request 111938: Allow installing both qca2 and qca3

On Sept. 1, 2013, 5:16 p.m., David Faure wrote: Looks fine to me. Ivan Romanov wrote: I don't agree with this patch. I didn't get any notification about this. So I very ask you before do any changes in cmake rules talk with me. It is important for me. David Faure wrote:

Re: Review Request 111938: Allow installing both qca2 and qca3

/local is the most rarely case. David Faure wrote: +1. This seems to be the best solution. Christophe Giboudeaux wrote: No no no, you completely ignored what I said yesterday. QCA_INSTALL_IN_QT_PREFIX must be OFF by default without condition, and that was fixed with this review

Review Request 112665: Drop Obsolete modules from ECM

/ Testing --- Thanks, Christophe Giboudeaux ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 114518: Rename the itemmodels frameworks to kitemmodels

a change tier1/itemmodels/autotests/CMakeLists.txt http://git.reviewboard.kde.org/r/114518/#comment32740 you didn't rename the macro tier1/itemmodels/CMakeLists.txt http://git.reviewboard.kde.org/r/114518/#comment32741 project(KItemModels) - Christophe Giboudeaux On Dec. 17, 2013

Re: Review Request 114521: Rename DNSSD to KDNSSD

thing line 89) If you choose the later, the camelcased files in kde4support need to be changed as well - Christophe Giboudeaux On Dec. 17, 2013, 4:43 p.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail

Re: Review Request 114522: Rename itemviews to kitemviews

--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114522/#review45886 --- Ship it! Ship It! - Christophe Giboudeaux On Dec. 17, 2013

Re: Review Request 114521: Rename DNSSD to KDNSSD

--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114521/#review45884 --- Ship it! Ship It! - Christophe Giboudeaux On Dec. 17, 2013

Review Request 114524: Load the kdoctools macro before trying to find the build deps

and installed all the framework modules in different prefixes without issue. The commit is already in the split kdoctools repo. Thanks, Christophe Giboudeaux ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman

Re: Framework metadata

On Wednesday 18 December 2013 19:50:58 Alex Merry wrote: On 18/12/13 17:54, Aurélien Gâteau wrote: We need to have at least a COPYING file in there, with the full content of the license. I believe this was already done before the split. I added COPYING files in each frameworks which have

Re: Review Request 114524: Load the kdoctools macro before trying to find the build deps

94ad675 Diff: https://git.reviewboard.kde.org/r/114524/diff/ Testing --- Built and installed all the framework modules in different prefixes without issue. The commit is already in the split kdoctools repo. Thanks, Christophe Giboudeaux ___ Kde

Re: Review Request 115086: Adapt to new attica name

--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115086/#review47607 --- Ship it! Ship It! - Christophe Giboudeaux On Jan. 17

Re: Review Request 115087: Adapt to new attica name

--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115087/#review47615 --- Ship it! Ship It! - Christophe Giboudeaux On Jan. 17

Re: Review Request 115140: Adapt to new attica name

could just remove these lines. - Christophe Giboudeaux On Jan. 20, 2014, 9:59 a.m., Stefano Avallone wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115140

Re: Review Request 115140: kde-workspace doesn't explicitly need attica anymore

--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115140/#review47802 --- Ship it! Ship It! - Christophe Giboudeaux On Jan. 20

Re: Review Request 115379: Check the C_COMPILER_ID when settings C_FLAGS, not CXX_COMPILER_ID

--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115379/#review48557 --- Ship it! Ship It! - Christophe Giboudeaux On Jan. 29

Review Request 117155: EGH - Also look for headers in the build dir

- modules/ECMGenerateHeaders.cmake 739c211 Diff: https://git.reviewboard.kde.org/r/117155/diff/ Testing --- Thanks, Christophe Giboudeaux ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo

Re: Review Request 117155: EGH - Also look for headers in the build dir

. This way, generated headers (e.g: kcfg files) don't have to be treated manually. Diffs - modules/ECMGenerateHeaders.cmake 739c211 Diff: https://git.reviewboard.kde.org/r/117155/diff/ Testing --- Thanks, Christophe Giboudeaux ___ Kde

Re: Review Request 119043: pollkit-qt-1 buildsystem adjustements

://git.reviewboard.kde.org/r/119043/#comment42702 LINK_PRIVATE - Christophe Giboudeaux On June 30, 2014, 8:28 p.m., Hrvoje Senjan wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119043

Re: Review Request 119043: pollkit-qt-1 buildsystem adjustements

: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119043/ --- (Updated June 30, 2014, 9:53 p.m.) Review request for KDE Frameworks, Polkit Qt, Aleix Pol Gonzalez, and Christophe Giboudeaux

Re: Review Request: Fix attica dependency

://git.reviewboard.kde.org/r/107911/#comment18267 else() knewstuff/knewstuff3/CMakeLists.txt http://git.reviewboard.kde.org/r/107911/#comment18268 endif() - Christophe Giboudeaux On Dec. 25, 2012, 8:26 p.m., Valentin Rusu wrote

Re: Review Request: Fix attica dependency

On Dec. 26, 2012, 9:12 a.m., Christophe Giboudeaux wrote: knewstuff/knewstuff3/CMakeLists.txt, line 21 http://git.reviewboard.kde.org/r/107911/diff/1/?file=101183#file101183line21 endif() Please read http://techbase.kde.org/Policies/CMake_Coding_Style#End_commands

Re: Review Request 111080: Macro*.cmake changes

/111080/#comment26924 Please add INCLUDE_QUIET_PACKAGES to your feature_summary calls. one could believe that WHAT ALL prints all but that doesn't include packages searched using the QUIET keyword - Christophe Giboudeaux On July 23, 2013, 8:02 a.m., Andrea Scarpino wrote

Re: Review Request 112038: Look for LibAttica where needed

://git.reviewboard.kde.org/r/111937 without the 'find_package(LibAttica NO_MODULE) line'. Thanks, Christophe Giboudeaux ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112038: Look for LibAttica where needed

--- Konsole build with https://git.reviewboard.kde.org/r/111937 without the 'find_package(LibAttica NO_MODULE) line'. Thanks, Christophe Giboudeaux ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman

Re: Review Request 112038: Look for LibAttica where needed

://git.reviewboard.kde.org/r/112038/diff/ Testing --- Konsole build with https://git.reviewboard.kde.org/r/111937 without the 'find_package(LibAttica NO_MODULE) line'. Thanks, Christophe Giboudeaux ___ Kde-frameworks-devel mailing list Kde-frameworks

Re: Review Request 112038: Look for LibAttica where needed

with https://git.reviewboard.kde.org/r/111937 without the 'find_package(LibAttica NO_MODULE) line'. Thanks, Christophe Giboudeaux ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks

Re: Review Request 120959: [KPimUtils] Don't install headers into $include_dir/KPIMUtils/KPIMUtils

On nov. 3, 2014, 7:59 après-midi, Christophe Giboudeaux wrote: KPIMUtils/KPIMUtils/SomeHeader is correct. This patch isn't (and the facebook resource builds fine without this patch). KF5PimUtilsTargets.cmake sets this: set_target_properties(KF5::PimUtils PROPERTIES

Re: Review Request 120959: [KPimUtils] Don't install headers into $include_dir/KPIMUtils/KPIMUtils

On nov. 3, 2014, 7:59 après-midi, Christophe Giboudeaux wrote: KPIMUtils/KPIMUtils/SomeHeader is correct. This patch isn't (and the facebook resource builds fine without this patch). KF5PimUtilsTargets.cmake sets this: set_target_properties(KF5::PimUtils PROPERTIES

Re: Review Request 120959: [KPimUtils] Don't install headers into $include_dir/KPIMUtils/KPIMUtils

On nov. 3, 2014, 7:59 après-midi, Christophe Giboudeaux wrote: KPIMUtils/KPIMUtils/SomeHeader is correct. This patch isn't (and the facebook resource builds fine without this patch). KF5PimUtilsTargets.cmake sets this: set_target_properties(KF5::PimUtils PROPERTIES

Re: Review Request 120959: [KPimUtils] Don't install headers into $include_dir/KPIMUtils/KPIMUtils

On nov. 3, 2014, 7:59 après-midi, Christophe Giboudeaux wrote: KPIMUtils/KPIMUtils/SomeHeader is correct. This patch isn't (and the facebook resource builds fine without this patch). KF5PimUtilsTargets.cmake sets this: set_target_properties(KF5::PimUtils PROPERTIES

Re: Review Request 123031: Let Kross be useable w/o searching for private deps

On March 18, 2015, 7:40 p.m., Christophe Giboudeaux wrote: KF5KrossConfig.cmake.in, line 9 https://git.reviewboard.kde.org/r/123031/diff/1/?file=355503#file355503line9 Are you sure ? ./ui/actioncollectionview.h:27:#include QWidget Hrvoje Senjan wrote

Re: Review Request 123082: Add missing license

--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123082/#review77839 --- Ship it! Ship It! - Christophe Giboudeaux On March 20

Re: Review Request 123031: Let Kross be useable w/o searching for private deps

/123031/#comment53326 Are you sure ? ./ui/actioncollectionview.h:27:#include QWidget - Christophe Giboudeaux On March 18, 2015, 5:34 p.m., Hrvoje Senjan wrote: --- This is an automatically generated e-mail. To reply, visit

Re: Review Request 122987: Allow user to specify path to myspell dictionary files

DICTIONARY: /usr/share/hunspell/fr_FR.aff /usr/share/hunspell/fr_FR.dic src/plugins/hunspell/CMakeLists.txt https://git.reviewboard.kde.org/r/122987/#comment53312 endif() - Christophe Giboudeaux On March 17, 2015, 1:09 p.m., Eugene Shalygin wrote

Re: kded's cmake package

Hi, On mercredi 29 juin 2016 11:22:30 CEST Harald Sitter wrote: > Hola! > > So, I just noticed that KDED's cmake package is inconsistently named. > > cmake/KF5DocTools/ > versus > cmake/KDED/ > > Is there a reason for this? Are we going to change this for KF6? > No and that sounds curious.

D7253: Add build-flatpak target if there is a flatpak recipe around

cgiboudeaux added inline comments. INLINE COMMENTS > KDECMakeSettings.cmake:361 > +function(_find_json_recipe json_recipe) > +execute_process(COMMAND git ls-files *org.kde.*.json > +OUTPUT_VARIABLE json find_package(Git) if(Git_FOUND) execute_process(COMMAND GIT_EXECUTABLE...

Heads-up : automoc changes in CMake 3.8

Hi, CMake 3.8 comes with a change that could break compilation under certain circumstances.(*) in CMake < 3.8, automoc'ed files are stored in . Starting with CMake 3.8, they're now created in : /_autogen/include What does it change ? In most cases, nothing. moc files will be re-created. The

D7198: Set CMAKE_*_OUTPUT_DIRECTORY to run tests without installing.

cgiboudeaux accepted this revision. This revision is now accepted and ready to land. REPOSITORY R240 Extra CMake Modules BRANCH master REVISION DETAIL https://phabricator.kde.org/D7198 To: dfaure, cgiboudeaux Cc: #frameworks, #build_system

D7198: Set CMAKE_*_OUTPUT_DIRECTORY to run tests without installing.

cgiboudeaux added a comment. In https://phabricator.kde.org/D7198#134234, @cgiboudeaux wrote: > Fixed with https://phabricator.kde.org/R240:7e7b6da8c66b7ecf1c21f330c31ffe7975259498 (+ https://phabricator.kde.org/R240:ef01c93637e1840ee701673970a371c011e36a40) REPOSITORY R240

D7198: Set CMAKE_*_OUTPUT_DIRECTORY to run tests without installing.

cgiboudeaux added a comment. Fixed with https://phabricator.kde.org/R240:7e7b6da8c66b7ecf1c21f330c31ffe7975259498 REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D7198 To: dfaure, cgiboudeaux, kfunk Cc: aacid, kfunk, #frameworks, #build_system

D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM

cgiboudeaux added a comment. -1. They don't match the ECM coding style and code quality (doc, license, endif(), pkgconfig...) And : > kdelibs4support/cmake/modules/FindGLIB2.cmake > ecm/attic/modules/FindGLIB2.cmake These two are there for legacy purpose. >

D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM

cgiboudeaux added inline comments. INLINE COMMENTS > FindGLIB2.cmake:10 > +# True if the GLib2 library is available > +# ``GLIB2_INCLUDE_DIR`` > +# The GLib2 include directory Call it GLIB2_INCLUDE_DIRS. > FindGLIB2.cmake:43 > +# > +# For details see the accompanying

D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM

cgiboudeaux added a comment. Looks good. What's your migration plan ? if you don't want to bump the ECM requirements for applications looking for pulseaudio, you have to set some compatibility/deprecated vars (eg: kmix uses uppercase variables) INLINE COMMENTS > FindPulseAudio.cmake:109

D8780: Try `llvm-config` to find `libclang`

cgiboudeaux added inline comments. INLINE COMMENTS > FindPythonModuleGeneration.cmake:212 > +libclang_LIBRARY > +clang > +PATH ${LLVM_LIBDIR} NAMES clang clang-3.8 clang-3.9 and remove the lines 234-239 > FindPythonModuleGeneration.cmake:213 > +

D9018: Don't cause circular linking on Windows

cgiboudeaux added inline comments. INLINE COMMENTS > CMakeLists.txt:13 > > -set_target_properties(kspell_enchant PROPERTIES OUTPUT_NAME "enchant") > install(TARGETS kspell_enchant DESTINATION > ${KDE_INSTALL_PLUGINDIR}/kf5/sonnet/) 'kspell' sounds like an old KDE3 thing. Shouldn't that

D9018: Don't cause circular linking on Windows

cgiboudeaux added inline comments. INLINE COMMENTS > cgiboudeaux wrote in CMakeLists.txt:13 > 'kspell' sounds like an old KDE3 thing. Shouldn't that plugin be renamed > sonnet_enchant ? Ah, won't be needed, the plugin isn't built. REPOSITORY R246 Sonnet REVISION DETAIL

D8998: Add FindSeccomp to find-modules

cgiboudeaux added a comment. Mostly good. Last question : is the version important ? If yes, please add an additional way to get the version if Seccomp_VERSION is empty. (you can parse seccomp.h to find it, look at the other Find*.cmake modules for examples) INLINE COMMENTS >

D8858: Fix testWaylandFullscreenShell.

This revision was automatically updated to reflect the committed changes. Closed by commit R127:7cd465a3bad3: Fix testWaylandFullscreenShell. (authored by cgiboudeaux). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D8858?vs=22546=23037#toc REPOSITORY R127 KWayland CHANGES SINCE LAST

D8998: Add FindSeccomp to find-modules

cgiboudeaux added inline comments. INLINE COMMENTS > FindSeccomp.cmake:12 > +# The seccomp include directories > +# ``Seccomp_LIBRARIES`` > +# The seccomp libraries for linking so, what about naming your variables Seccomp_LIBRARIES and Seccomp_INCLUDE_DIRS in the file ? >

Frameworks and pkgconfig files

Hello, in https://bugs.kde.org/386933, the bug reporter mentions the pkgconfig file name shall match the library one. Do we have a policy about the pkgconfig stuff ? Only a couple frameworks install such files with different scheme (Baloo (uppercase B), libKF5Attica (library name),

D9056: Add the description tag to the generated pkgconfig files

cgiboudeaux created this revision. cgiboudeaux added reviewers: dfaure, apol. Restricted Application added projects: Frameworks, Build System. Restricted Application added subscribers: Build System, Frameworks. REVISION SUMMARY pkgconfig complains when the .pc file doesn't have a description.

D8858: Fix testWaylandFullscreenShell.

cgiboudeaux added a comment. ping ! REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D8858 To: cgiboudeaux, graesslin Cc: plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart,

D9056: Add the description tag to the generated pkgconfig files

This revision was automatically updated to reflect the committed changes. Closed by commit R240:fd60f2b893d0: Add the description tag to the generated pkgconfig files (authored by cgiboudeaux). REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE

D9299: Introduce INSTALL_PREFIX_SCRIPT to easily set up prefixes

cgiboudeaux added a comment. In https://phabricator.kde.org/D9299#179032, @kfunk wrote: > If we'd name this file somewhat less generic then it could be even installed by default, no? > > I had the scheme of the QNX setup script in my mind:

D9299: Introduce INSTALL_PREFIX_SCRIPT to easily set up prefixes

cgiboudeaux added a comment. In https://phabricator.kde.org/D9299#179035, @cgiboudeaux wrote: > In https://phabricator.kde.org/D9299#179032, @kfunk wrote: > > > If we'd name this file somewhat less generic then it could be even installed by default, no? > > > > I had the scheme

D9299: Introduce INSTALL_PREFIX_SCRIPT to easily set up prefixes

cgiboudeaux added a comment. +1. Please add the missing documentation about this new option. INLINE COMMENTS > apol wrote in KDEInstallDirs.cmake:700-704 > Usually isn't necessary as you get the libraries through the rpath. I can add > it though. It won't harm and solve potential issues if

D5175: Fix 'Installed name of kio_http_cache_cleaner conflicts with related KDE4 installation'.

cgiboudeaux added a comment. This will have effects on the non-kio users : https://lxr.kde.org/search?_string=kio_http_cache_cleaner REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D5175 To: habacker, dfaure, ltoscano, bcooksley Cc: cgiboudeaux, #frameworks

D8858: Fix testWaylandFullscreenShell.

cgiboudeaux updated this revision to Diff 22546. cgiboudeaux added a comment. Add message if the executable wasn't found REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8858?vs=22492=22546 BRANCH master REVISION DETAIL

D8780: Try `llvm-config` to find `libclang`

cgiboudeaux added inline comments. INLINE COMMENTS > turbov wrote in FindPythonModuleGeneration.cmake:212 > What distribution is this? Why only 3.8 and 3.9?? Why not 4.0 or 5.0??? > > I've just checked Ubuntu 14.04 (Clang 3.3 to 3.9 available) and 17.10 (up to > 5.0 available)... installing

D8780: Try `llvm-config` to find `libclang`

cgiboudeaux added inline comments. INLINE COMMENTS > turbov wrote in FindPythonModuleGeneration.cmake:213 > Why `HINTS` is "better"?? As for me this is not a "hint" -- at this point we > know exactly the correct libdir location... it is why I use `PATH`...

D8777: restore old behaviour of FindPulseAudio

This revision was automatically updated to reflect the committed changes. Closed by commit R240:c02178fa380c: restore old behaviour of FindPulseAudio (authored by jhirte, committed by cgiboudeaux). REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE

D8790: Add FindSasl2.cmake to ECM

cgiboudeaux added a project: KDE PIM. cgiboudeaux added a reviewer: KDE PIM. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D8790 To: cgiboudeaux, #kde_pim Cc: #frameworks, #build_system, dvasin, winterz, vkrause, mlaurent, knauss, dvratil

D8790: Add FindSasl2.cmake to ECM

cgiboudeaux created this revision. Restricted Application added projects: Frameworks, Build System. Restricted Application added subscribers: Build System, Frameworks. REVISION SUMMARY We have copies of this module in several PIM repositories (kdepim-runtime, kldap, kimap, libksieve...).

D8790: Add FindSasl2.cmake to ECM

This revision was automatically updated to reflect the committed changes. Closed by commit R240:9e2ed7fb8950: Add FindSasl2.cmake to ECM (authored by cgiboudeaux). REPOSITORY R240 Extra CMake Modules CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8790?vs=22267=22276 REVISION DETAIL

D8858: Fix testWaylandFullscreenShell.

cgiboudeaux created this revision. cgiboudeaux added a reviewer: graesslin. Restricted Application added subscribers: Frameworks, plasma-devel. Restricted Application added projects: Plasma on Wayland, Frameworks. REVISION SUMMARY This test needs the weston executable. Skip the test if the

D8837: Fix testtrash with qtbase 5.10 beta 4

cgiboudeaux added a comment. In https://phabricator.kde.org/D8837#168327, @bcooksley wrote: > Elvis, how many other issues like this do we expect to crop up due to Qt 5.10? https://bugs.kde.org/386420 ? REPOSITORY R241 KIO BRANCH master REVISION DETAIL

D8777: restore old behaviour of FindPulseAudio

cgiboudeaux accepted this revision. cgiboudeaux added a comment. This revision is now accepted and ready to land. Please commit after fixing the last issue. INLINE COMMENTS > FindPulseAudio.cmake:123 > > -mark_as_advanced(PulseAudio_INCLUDE_DIRS PulseAudio_INCLUDE_DIR > -

D8777: restore old behaviour of FindPulseAudio

cgiboudeaux added a comment. you also need set(PULSEAUDIO_FOUND "${PulseAudio_FOUND}"). You then have to move the compat variables below "find_package_handle_standard_args". Please also fix the mark_as_advanced() line to match the renamed vars. REPOSITORY R240 Extra CMake Modules

D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM

cgiboudeaux added a comment. In https://phabricator.kde.org/D7823#164474, @cgiboudeaux wrote: > Don't forget to create .rst files in doc/find-module and change 'Since 5.39.0' to 'Since 5.40.0' '5.41.0', sorry REPOSITORY R240 Extra CMake Modules REVISION DETAIL

D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM

cgiboudeaux accepted this revision. cgiboudeaux added a comment. This revision is now accepted and ready to land. No news from the ECM maintainer, please push. Don't forget to create .rst files in doc/find-module and change 'Since 5.39.0' to 'Since 5.40.0' REPOSITORY R240 Extra CMake

D8660: ECMAddTests: set QT_PLUGIN_PATH so locally built plugins can be found

cgiboudeaux accepted this revision. cgiboudeaux added a comment. This revision is now accepted and ready to land. +1. Tested with a few repo without noticing any regression. REPOSITORY R240 Extra CMake Modules BRANCH master REVISION DETAIL https://phabricator.kde.org/D8660 To:

D8281: Allow to use IcoTool for Windows icons

cgiboudeaux added inline comments. INLINE COMMENTS > FindIcoTool.cmake:1 > +# Copyright 2017 Vincent Pinon > +include(${CMAKE_CURRENT_LIST_DIR}/ECMFindModuleHelpersStub.cmake) This module doesn't have any license and doesn't provide any doc. > FindIcoTool.cmake:3 >

D7823: Add FindGLIB2.cmake and FindPulseAudio.cmake to ECM

cgiboudeaux added a comment. In https://phabricator.kde.org/D7823#164880, @marten wrote: > In https://phabricator.kde.org/D7823#164879, @cfeck wrote: > > > Are the copies in the attic directory still needed for compatibility reasons, or can they get removed? > > > No needed by

D9116: Make sure to search for Qt5-based qmlplugindump

cgiboudeaux added a comment. -1 this will not work for anyone. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D9116 To: asturmlechner, apol Cc: cgiboudeaux, #frameworks, #build_system

D9116: Make sure to search for Qt5-based qmlplugindump

cgiboudeaux added a comment. In https://phabricator.kde.org/D9116#174856, @dfaure wrote: > Well, it's just a hint. If it's not found there, no harm done. But yeah, at least the paths for most distros could be there, to be fair ;) > > For OpenSuSE no change needed, it's in /usr/bin.

D9299: Introduce INSTALL_PREFIX_SCRIPT to easily set up prefixes

cgiboudeaux added inline comments. INLINE COMMENTS > KDEInstallDirs.cmake:700-704 > +export XDG_DATA_DIRS=${KDE_INSTALL_FULL_DATADIR}:$XDG_DATA_DIRS > +export XDG_CONFIG_DIRS=${KDE_INSTALL_FULL_CONFDIR}:$XDG_CONFIG_DIRS > +export PATH=${KDE_INSTALL_FULL_BINDIR}:$PATH > +export

D12867: Fix minor documentation typos

cgiboudeaux accepted this revision. cgiboudeaux added a comment. This revision is now accepted and ready to land. Thanks. Please use the GIT_SILENT keyword for typo fixes. REPOSITORY R240 Extra CMake Modules BRANCH doc_typos (branched from master) REVISION DETAIL

D13102: Don't include a 64 when building 64bit architectures on flatpak

cgiboudeaux added a comment. Please update the doc above. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D13102 To: apol, #frameworks Cc: cgiboudeaux, eszlari, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D13406: In cmake macro file use CMAKE_CURRENT_LIST_DIR consequently instead of mixed use with KF5I18n_DIR

cgiboudeaux added a comment. -1, this change would break the autotests. REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D13406 To: habacker, ilic Cc: cgiboudeaux, kde-frameworks-devel, michaelh, ngraham, bruns

D13406: In cmake macro file use CMAKE_CURRENT_LIST_DIR consequently instead of mixed use with KF5I18n_DIR

cgiboudeaux added a comment. In D13406#275765 , @cgiboudeaux wrote: > In D13406#275755 , @habacker wrote: > > > Fixed autotests > > > Still -1, the code is now more complicated just for a

D13406: In cmake macro file use CMAKE_CURRENT_LIST_DIR consequently instead of mixed use with KF5I18n_DIR

cgiboudeaux added a comment. In D13406#275755 , @habacker wrote: > Fixed autotests Still -1, the code is not more complicated just for a cosmetic change in a build system file. Are you trying to fix anything ? REPOSITORY R249 KI18n

D13328: A backend is required for kdnssd

cgiboudeaux added inline comments. INLINE COMMENTS > adridg wrote in CMakeLists.txt:65-66 > Hi. If you're posting a review, please provide constructive input. If there > are policies or guidelines about places, please link to them. If there > aren't, but you would prefer a change, please

D13328: A backend is required for kdnssd

cgiboudeaux added inline comments. INLINE COMMENTS > CMakeLists.txt:65-66 > ecm_install_po_files_as_qm(po) > +else() > +message(FATAL_ERROR "Either Avahi or DNSSD is required for KDE > applications to make use of multicast DNS/DNS-SD service discovery") > endif() wrong place

[build.kde.org] Verbose output for the tests failures

Hello, A couple days ago, Ben pushed a tiny change [1] to the build.kde.org config files. Now, when a test fails on the CI, il will also paste the test output instead of just mentioning 'test xx failed'. This should hopefully help finding why some tests succeed locally but fail on

D13102: Don't include a 64 when building 64bit architectures on flatpak

cgiboudeaux accepted this revision. This revision is now accepted and ready to land. REPOSITORY R240 Extra CMake Modules BRANCH master REVISION DETAIL https://phabricator.kde.org/D13102 To: apol, #frameworks, cgiboudeaux Cc: cgiboudeaux, eszlari, kde-frameworks-devel, kde-buildsystem,

D13102: Don't include a 64 when building 64bit architectures on flatpak

cgiboudeaux added a comment. Thanks. +1 REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D13102 To: apol, #frameworks Cc: cgiboudeaux, eszlari, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D10166: Add -Wlogical-op -Wzero-as-null-pointer-constant to KF5 warnings

cgiboudeaux accepted this revision. This revision is now accepted and ready to land. REPOSITORY R240 Extra CMake Modules BRANCH arcpatch-D10166 (branched from master) REVISION DETAIL https://phabricator.kde.org/D10166 To: aacid, cgiboudeaux Cc: kde-frameworks-devel, kde-buildsystem,

D10166: Add -Wlogical-op -Wzero-as-null-pointer-constant to KF5 warnings

cgiboudeaux added a comment. Thanks, just remove the trailing space before the 2 closing parenthesis. REPOSITORY R240 Extra CMake Modules BRANCH arcpatch-D10166 (branched from master) REVISION DETAIL https://phabricator.kde.org/D10166 To: aacid, cgiboudeaux Cc: kde-frameworks-devel,

D13406: In cmake macro file use CMAKE_CURRENT_LIST_DIR consequently instead of mixed use with KF5I18n_DIR

cgiboudeaux accepted this revision. cgiboudeaux added a comment. This revision is now accepted and ready to land. In D13406#277236 , @habacker wrote: > > BTW: I was faced to this on trying to understand the ki1i8n build system,

D13698: Improve ECMAddAppIconMacro.

cgiboudeaux added inline comments. INLINE COMMENTS > vpinon wrote in FindIcoTool.cmake:1 > Hello, > FindIcoTool.cmake is largely copied from FindPng2Ico.cmake, I'm even not sure > I can really claim the copyright. > In any case, I would choose the same license as all other ECM files (among >

D13698: Improve ECMAddAppIconMacro.

cgiboudeaux added inline comments. INLINE COMMENTS > FindIcoTool.cmake:1 > +# Copyright 2017 Vincent Pinon > + - Missing doc - Missing license > ECMAddAppIcon.cmake:70 > > + > > #= extra line that shouldn't be

D12674: Mark `Phonon4Qt5` dependency as optional in CMakeLists file

cgiboudeaux added inline comments. INLINE COMMENTS > CMakeLists.txt:68 > > -find_package(Phonon4Qt5 4.6.60 REQUIRED NO_MODULE) > +find_package(Phonon4Qt5 4.6.60 OPTIONAL_COMPONENTS NO_MODULE) > set_package_properties(Phonon4Qt5 PROPERTIES there are no component, why do you use this keyword ?

D9116: Make sure to search for Qt5-based qmlplugindump

cgiboudeaux added a comment. In https://phabricator.kde.org/D9116#186638, @asturmlechner wrote: > Can we move forward with this? ECM provides ECMQueryQmake.cmake. you can try eg: include("${ECM_MODULE_DIR}/ECMQueryQmake.cmake") then calling query_qmake(qt_binaries_dir

D9116: Make sure to search for Qt5-based qmlplugindump

cgiboudeaux added a comment. In https://phabricator.kde.org/D9116#198423, @asturmlechner wrote: > Want me to fix FindQtWaylandScanner.cmake in the same way? Sure. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D9116 To: asturmlechner, apol,

D10201: FindQtWaylandScanner.cmake: Use qmake-query for HINT

cgiboudeaux accepted this revision. cgiboudeaux added a comment. This revision is now accepted and ready to land. Thanks REPOSITORY R240 Extra CMake Modules BRANCH master REVISION DETAIL https://phabricator.kde.org/D10201 To: asturmlechner, #frameworks, #build_system, cgiboudeaux Cc:

D9116: Make sure to search for Qt5-based qmlplugindump

cgiboudeaux accepted this revision. This revision is now accepted and ready to land. REPOSITORY R240 Extra CMake Modules BRANCH arcpatch-D9116 REVISION DETAIL https://phabricator.kde.org/D9116 To: asturmlechner, apol, cgiboudeaux Cc: aacid, dfaure, cgiboudeaux, #frameworks,

D10166: Add -Wlogical-op -Wzero-as-null-pointer-constant to KF5 warnings

cgiboudeaux added a comment. These flags are not in the Clang warnings (list for each version here: https://github.com/Barro/compiler-warnings) REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D10166 To: aacid Cc: cgiboudeaux, dhaumann, #frameworks,

D8998: Add FindSeccomp to find-modules

cgiboudeaux accepted this revision. cgiboudeaux added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > FindSeccomp.cmake:21 > +# > +# Since 5.42.0. > + 5.44.0 REPOSITORY R240 Extra CMake Modules BRANCH master REVISION DETAIL

D9480: Add cmake function 'kdbusaddons_generate_dbus_service_file'

cgiboudeaux added inline comments. INLINE COMMENTS > KF5DBusAddonsMacros.cmake:4 > +# > +# kdbusaddons_generate_dbus_service_file(executable name path) > +# kdbusaddons_generate_dbus_service_file(EXECUTABLE NAME PATH) to improve the readability. Please also document each parameter. >

D14569: Make it possible for ECM to detect po files at configure time

cgiboudeaux accepted this revision. cgiboudeaux added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > KDECMakeSettings.cmake:95 > +# > +# ``KDE_L10N_SYNC_TRANSLATIONS`` (OFF by default) will downloadthe > translations at configuration > +# time instead of

  1   2   3   4   >