D26751: ECMAddAppIcon: Add sc in regex to extract extension from valid names

2020-01-19 Thread Aleix Pol Gonzalez
apol accepted this revision. apol added a comment. Looks good to me, bonus points if you provide a better commit message explaining what it does and how you tested it. REPOSITORY R240 Extra CMake Modules BRANCH sc_appicon REVISION DETAIL https://phabricator.kde.org/D26751 To:

D26749: WIP: Support NDK r20 and Qt 5.14

2020-01-19 Thread Aleix Pol Gonzalez
apol added inline comments. INLINE COMMENTS > Android.cmake:173 > +set(ANDROID_STL ${CMAKE_ANDROID_STL_TYPE}) > +include(${CMAKE_ANDROID_NDK}/build/cmake/android.toolchain.cmake REQUIRED) > + Why's this better? Or how is it different? REPOSITORY R240 Extra CMake Modules REVISION DETAIL

D26752: ECMAddAppIcon: Do not warn about mac and window icons if isnt a OS specific build

2020-01-19 Thread patrick j pereira
patrickelectric added a comment. Hi @cgiboudeaux and @bcooksley, there is a reason of why this patch is valid. Have you read the commit message ? In #kirogi we provide a valid icon (svg) with a valid prefix (sc), as you probably know *sc* stands

D26752: ECMAddAppIcon: Do not warn about mac and window icons if isnt a OS specific build

2020-01-19 Thread Ben Cooksley
bcooksley requested changes to this revision. bcooksley added a comment. Christophe is correct here, it is worth warning developers about these issues regardless of the platform, so they can get the code ready for those platforms and test everything in their local environment as much as

D26752: ECMAddAppIcon: Do not warn about mac and window icons if isnt a OS specific build

2020-01-19 Thread Christophe Giboudeaux
cgiboudeaux added a comment. In D26752#596949 , @tcanabrava wrote: > > I don't see the gain on having a warning - in a windows system, about > missing mac icons if I'm not *deploying*. Then fix your code. ie only call

D26752: ECMAddAppIcon: Do not warn about mac and window icons if isnt a OS specific build

2020-01-19 Thread Christophe Giboudeaux
cgiboudeaux added a comment. In D26752#596809 , @tcanabrava wrote: > That’s not a developer issue, it’s a packaging issue. AUTHOR_WARNING *is* for developers. If you want to hide these warnings, use -Wno-dev [1] [1]

D26752: ECMAddAppIcon: Do not warn about mac and window icons if isnt a OS specific build

2020-01-19 Thread Tomaz Canabrava
tcanabrava added a comment. That’s not a developer issue, it’s a packaging issue. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D26752 To: patrickelectric, apol, tcanabrava, cgiboudeaux Cc: apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem,

D26752: ECMAddAppIcon: Do not warn about mac and window icons if isnt a OS specific build

2020-01-19 Thread Christophe Giboudeaux
cgiboudeaux added a comment. You may use Linux to develop software that's intended to be used also on Mac and Windows. You can't expect developers to have build environment for every platform REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D26752 To:

D26752: ECMAddAppIcon: Do not warn about mac and window icons if isnt a OS specific build

2020-01-19 Thread Tomaz Canabrava
tcanabrava added a subscriber: apol. tcanabrava added a comment. But why would I get the warning if I build on Linux? The warning should target the platform, not the entire build system. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D26752 To:

D26752: ECMAddAppIcon: Do not warn about mac and window icons if isnt a OS specific build

2020-01-19 Thread Christophe Giboudeaux
cgiboudeaux requested changes to this revision. cgiboudeaux added a comment. This revision now requires changes to proceed. I object. This warning is for developers. It tells them the icons are missing for some platforms. REPOSITORY R240 Extra CMake Modules REVISION DETAIL