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

[patrick j pereira]

patrickelectric added a comment.


  Hi @cgiboudeaux, thanks for the explanation, I'll take a look and get back 
here.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D26752

To: patrickelectric, apol, tcanabrava, cgiboudeaux, bcooksley
Cc: bcooksley, patrickelectric, apol, cgiboudeaux, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

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

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  In D26752#597149 , 
@patrickelectric wrote:
  
  > 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 for for 
scalable (SVG) files.
  
  
  As expected, the problem is in the kirogi code. Read the ECMAddAppIcon doc:
  
# The given icons, whose names must match the pattern::
#
#   -.png
  
  and now look at the kirogi code:
  
file(GLOB ICONS_SRCS 
"${CMAKE_CURRENT_SOURCE_DIR}/../data/icons/*apps-kirogi.svg")
ecm_add_app_icon(kirogi_SRCS ICONS ${ICONS_SRCS})
  
  The warning about Windows is expected.
  
  Now about Apple: if a .svg or .svgz is passed to ecm_add_app_icon, ksvg2icns 
(if found) will create the icons bundle.
  
  The solution is not hiding the warning when the macro is called on different 
platform but fixing the condition.
  
  Suggestion: After landing D26751 , add a 
`else()` block to `if (NOT (ext STREQUAL "svg" OR ext STREQUAL "svgz"))` where 
you `set(_ecm_add_app_icon_svg_icons TRUE)`
  and change:
  
if (NOT (mac_icons OR mac_sidebar_icons))
  
  to:
  
if (NOT (mac_icons OR mac_sidebar_icons) AND NOT 
_ecm_add_app_icon_svg_icons)
  
  Since ksvg2ico doesn't create the sidebar icons it can be further improved 
but at least you won't see a warning if you call ecm_add_app_icon with only svg 
files.
  
  You will only get the warning about the broken behaviour on Windows.
  
  Now another note: kirogi doesn't provide any png icon, this is also bad on 
Linux. Please create png icons and install them in the hicolor namespace.
  See 
https://specifications.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html#install_icons

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D26752

To: patrickelectric, apol, tcanabrava, cgiboudeaux, bcooksley
Cc: bcooksley, patrickelectric, apol, cgiboudeaux, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

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

[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 for for 
scalable (SVG) files.
  
  As I said in the commit body, "KSVG2ICNS will not exist if the program is not 
being compiled to APPLE"
  You'll probably say: "Well, so the developer should install this program."
  Nops, if you take a look in **kiconthemes**: 
https://github.com/KDE/kiconthemes/blob/3e668c7fba9fda7469a75247e0926530cdd1eb29/src/CMakeLists.txt#L3
  You'll see that KDE only provides such binary if APPLE is true.
  Why such binary is important ? As you can see in **ECMAddAppIcon**: 
https://github.com/KDE/extra-cmake-modules/blob/master/modules/ECMAddAppIcon.cmake#L117
  There is a **APPLE** check to convert the SVG binaries to mac valid icons.
  Well, if **KSVG2ICNS** is only build to APPLE.
  **ECMAddAppIcon** only looks for **KSVG2ICNS** with **APPLE**.
  Why should **ECMAddAppIcon** provide such warning if KDE **ECM** 
and**kiconthemes** only do such thing for APPLE ?
  There is no logic reason for this warning if everything else only works with 
**APPLE**.
  If the developer wants to cross build for **APPLE** he should set **APPLE** 
and build everything to **APPLE**, this warning is still not valid with the 
logic present in this comment. 
  I did point using the code and the workflow of the ECM files why this patch 
is valid, please provide the same thing if not.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D26752

To: patrickelectric, apol, tcanabrava, cgiboudeaux, bcooksley
Cc: bcooksley, patrickelectric, apol, cgiboudeaux, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

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

[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 possible.
  I know for certain that there are developers who rely on our CI system and 
the Binary Factory to test and validate their applications (because they 
themselves do not have access to a development environment on those platforms).
  
  If this warning were to occur only on the platform(s) which it impacts then 
it would become much harder for people to fix and debug.
  
  Of course, if you aren't targeting those platforms, you can just do the 
necessary if() to not hit this path.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D26752

To: patrickelectric, apol, tcanabrava, cgiboudeaux, bcooksley
Cc: bcooksley, patrickelectric, apol, cgiboudeaux, kde-frameworks-devel, 
kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

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

[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 ecm_add_app_icon on platforms you support 
(and leave the others broken, that's bad but you won't see the warning)
  
  > nor I do see a warning on a linux system about windows or mac run time
  >  issues (and missing icons is a run time issue).
  
  This is not a runtime issue. On Windows at least, the application icon is 
embedded in the executable.
  
  These warnings are real issues.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D26752

To: patrickelectric, apol, tcanabrava, cgiboudeaux
Cc: patrickelectric, apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, 
LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns

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

[Tomaz Canabrava]

I think we are misunderstanding eachother here.
If I'm developing the software and running cmake all the time, having a
warning that I can't fix (because it depends in another platform) is noise,
but still being a developer I don't want to run with -Wno-dev.
I do work in applications that targets more than one system (and they are
mac / windows / ios / android) I tend to use buildhosts, and those
buildhosts will tell me the warning - if any. But only if I'm targeting
them.

I don't see the gain on having a warning - in a windows system, about
missing mac icons if I'm not *deploying*.
nor I do see a warning on a linux system about windows or mac run time
issues (and missing icons is a run time issue).


On Sun, 19 Jan 2020 at 12:03 Tomaz Canabrava  wrote:

> That’s not a developer issue, it’s a packaging issue.
>
> On Sun, 19 Jan 2020 at 12:02 Christophe Giboudeaux <
> nore...@phabricator.kde.org> wrote:
>
>> cgiboudeaux added a comment. View Revision
>> 
>>
>> 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: *patrickelectric, apol, tcanabrava, cgiboudeaux
>> *Cc: *apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem,
>> LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns
>>
>

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

[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] https://cmake.org/cmake/help/latest/manual/cmake.1.html

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, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

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

[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, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

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

[Tomaz Canabrava]

That’s not a developer issue, it’s a packaging issue.

On Sun, 19 Jan 2020 at 12:02 Christophe Giboudeaux <
nore...@phabricator.kde.org> wrote:

> cgiboudeaux added a comment. View Revision
> 
>
> 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: *patrickelectric, apol, tcanabrava, cgiboudeaux
> *Cc: *apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem,
> LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns
>

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

[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: patrickelectric, apol, tcanabrava, cgiboudeaux
Cc: apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

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

[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: patrickelectric, apol, tcanabrava, cgiboudeaux
Cc: apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

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

[Tomaz Canabrava]

But why would I get the warning if I build on Linux? The warning should
target the platform, not the entire build system.


On Sun, 19 Jan 2020 at 10:00 Christophe Giboudeaux <
nore...@phabricator.kde.org> wrote:

> cgiboudeaux requested changes to this revision.
> cgiboudeaux added a comment.
> This revision now requires changes to proceed. View Revision
> 
>
> I object. This warning is for developers. It tells them the icons are
> missing for some platforms.
>
> *REPOSITORY*
> R240 Extra CMake Modules
>
> *REVISION DETAIL*
> https://phabricator.kde.org/D26752
>
> *To: *patrickelectric, apol, tcanabrava, cgiboudeaux
> *Cc: *cgiboudeaux, kde-frameworks-devel, kde-buildsystem, LeGast00n,
> GB_2, bencreasy, michaelh, ngraham, bruns
>

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

[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
  https://phabricator.kde.org/D26752

To: patrickelectric, apol, tcanabrava, cgiboudeaux
Cc: cgiboudeaux, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

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

[patrick j pereira]

patrickelectric retitled this revision from "ECMAddAppIcon: Do not warn about 
mac icons if isnt a mac build" to "ECMAddAppIcon: Do not warn about mac and 
window icons if isnt a OS specific build".

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  warning_icons

REVISION DETAIL
  https://phabricator.kde.org/D26752

To: patrickelectric, apol, tcanabrava
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns