Repository: pyinstaller/pyinstaller
Pull request: Icon translation using PIL
Fixes #6510

This puts a little extra code in the Mac path and the Windows path to try and convert icons into the correct platform format using PIL/Pillow.

The code for the paths is similar, so it could potentially be centralized in one location, but that seemed more disruptive, especially since I don't know the PyInstaller internals.

One thing I noticed is that the old Mac path didn't fail on incorrect icons, it would just transfer them into the bundle and MacOS wouldn't recognize the images properly. It now will fail explicitly like Windows does.

I tried to do the complete code and docs, but I haven't yet done the tests. I was unable to run the tests on Mac, because of #6430. I did get stuff building so I could see the icon, by making a couple codesigning-adjacent functions in osx.py just return instantly, but I don't think I got a functional executable out of it.

On Windows, I get: 
`FAILED tests/functional/test_basic.py::test_invalid_icon - KeyError: 'cachedir'`
As well as
`FAILED tests/unit/test_modulegraph/test_zipio.py::TestModuleGraph::test_islink - OSError: [WinError 1314] A required ...FAILED tests/unit/test_modulegraph/test_zipio.py::TestModuleGraph::test_readlink - OSError: [WinError 1314] A require...`
Which I assume is flakiness / windows defender, since I don't see how that could come from this PR.

I'm unsure how to properly test this, given PIL / no PIL.


Problem statement:
Opportunistically translate icon images to platform format (ico, icns) at build time
**Is your feature request related to a problem? Please describe.**
When packaging applications on different operating systems, the icons need to provided in a different format. I always end up uploading my PNG images to some sketchy "PNG to ICO" and "PNG to ICNS" converter websites.

**Describe the solution you'd like**
I saw the other day that PIL fully supports both of these formats. So it would be relatively simple to make a program that translates images in many formats into the required ICO or ICNS. It would be very convenient if PyInstaller could use PIL to translate icons to the system formats when they're built. I figure PyInstaller wouldn't want to add PIL as a full dependency just for this small optional bit of functionality, so I propose that PyInstaller tries to import PIL and convert to the correct format if incorrect format images are passed to `--icon`. If PIL isn't installed, that's fine, it will just do what it does now.

**Describe alternatives you've considered**
Of course this is very much just a convenience. There are conversion web sites online to handle this, or I could use PIL to do it myself.

**Additional context**
I could work on this.
Maybe maintainers think this is more maintenance hassle than it's worth in convenience. But I thought it was a cool idea to put out there. 😄

Hints from issue discussion:
I like this idea. If you could work on it that would be great.
Ooh, I hate optional dependencies. It's one extra thing to forget to also install when you install PyInstaller. It creates two code paths to test but no sane way to cover them both in one test suite run. It's one more unknown when someone reports they're having issues. Hard no from me, but I imagine I'm outvoted on that.
@bwoodsend Thank you for the feedback.

We could do PIL as a mandatory dependency on Mac and Windows. I figured that there would be more hesitation to do that though, which is why I proposed it optionally. But it would be cool to get this convenience to everyone, if another dependency isn't too big of an ask.

If I did a PR on this, I would make an effort to make the error messages and docs as clear as possible to mitigate any misunderstandings.

**Pass it unsupported format, PIL found:**
Log output “PIL translated icon file {given} to {format}”
**Pass it unsupported format, PIL not found:**
Error "unsupported image format. Use {format} or install PIL to attempt automatic format conversion"
**Pass it supported format:**
Same as now

PIL is already an optional dependency anyway, for converting/resizing the splash screen image when necessary (or returning the error if PIL is unavailable). So I see no problem if it is used in the same way for icon conversion.
@Starbuck5 Just do it. I'll cover my eyes and swallow my squeamishness.
@Starbuck5 Are you still wanting to do this? I've rather changed my mind about not wanting this after remembering what a pain PNG to icon/icns is myself so if you'd rather not do it then I will.
@bwoodsend I've been very slow on this, but yes, I'd like to do it.

I lost steam after doing the Windows version and the Documentation.
Remaining to do is the Mac version and the tests.

This is what I've currently got: https://github.com/starbuck5/pyinstaller/tree/icon-translation
Cool, no worries. What you've got so far looks good.

Commit message: Rename to normalize_icon_type

Also changes how the generated file is named to hopefully better avoid conflict

Patch to review:
diff --git a/PyInstaller/building/icon.py b/PyInstaller/building/icon.py
new file mode 100644
index 00000000000..aceb9854e0a
--- /dev/null
+++ b/PyInstaller/building/icon.py
@@ -0,0 +1,68 @@
+#-----------------------------------------------------------------------------
+# Copyright (c) 2013-2022, PyInstaller Development Team.
+#
+# Distributed under the terms of the GNU General Public License (version 2
+# or later) with exception for distributing the bootloader.
+#
+# The full license is in the file COPYING.txt, distributed with this software.
+#
+# SPDX-License-Identifier: (GPL-2.0-or-later WITH Bootloader-exception)
+#-----------------------------------------------------------------------------
+
+from typing import Tuple
+
+import os
+
+
+def normalize_icon_type(icon_path: str, allowed_types: Tuple[str], convert_type: str, workpath: str) -> str:
+    """
+    Outputs a valid icon path or raises an Exception trying
+    Checks to be sure the icon exists, and attempts to use Pillow to convert
+    it to the right format if it's in an unsupported format
+
+    Takes:
+    icon_path - the icon entered by the user
+    allowed_types - a tuple of icon formats that should be allowed through
+        EX: ("ico", "exe")
+    convert_type - the type to attempt conversion too if necessary
+        EX: "icns"
+    workpath - the temp directory to save any newly generated image files
+    """
+
+    # explicitly error if file not found
+    if not os.path.exists(icon_path):
+        raise FileNotFoundError(f"Icon input file {icon_path} not found")
+
+    _, extension = os.path.splitext(icon_path)
+    extension = extension[1:]  # get rid of the "." in ".whatever"
+
+    if extension not in allowed_types:
+        try:
+            from PIL import Image as PILImage
+            import PIL
+        except ImportError:
+            PILImage = None
+
+        if PILImage:
+            try:
+                generated_icon = os.path.join(workpath, f"generated{hash(icon_path)}.{convert_type}")
+                with PILImage.open(icon_path) as im:
+                    im.save(generated_icon)
+                icon_path = generated_icon
+            except PIL.UnidentifiedImageError:
+                raise ValueError(
+                    f"Something went wrong converting icon image '{icon_path}' to '.{convert_type}' with Pillow, "
+                    f"perhaps the image format is unsupported. Try again with a different file or use a file that can "
+                    f"be used without conversion on this platform: {allowed_types}"
+                )
+        # if Pillow isn't found, the user is notified that they can either try and install Pillow or translate to the
+        # correct format however they see fit
+        else:
+            raise ValueError(
+                f"Received icon image '{icon_path}' which exists but is not in the correct format. On this platform, "
+                f"only {allowed_types} images may be used as icons. If Pillow is installed, automatic conversion will "
+                f"be attempted. Please install Pillow or convert your '{extension}' file to one of {allowed_types} "
+                f"and try again."
+            )
+
+    return icon_path
diff --git a/PyInstaller/building/makespec.py b/PyInstaller/building/makespec.py
index a7693f7aaf6..82c1572c7ea 100644
--- a/PyInstaller/building/makespec.py
+++ b/PyInstaller/building/makespec.py
@@ -453,10 +453,12 @@ def __add_options(parser):
         "-i",
         "--icon",
         dest="icon_file",
-        metavar='<FILE.ico or FILE.exe,ID or FILE.icns or "NONE">',
+        metavar='<FILE.ico or FILE.exe,ID or FILE.icns or Image or "NONE">',
         help="FILE.ico: apply the icon to a Windows executable. FILE.exe,ID: extract the icon with ID from an exe. "
-        "FILE.icns: apply the icon to the .app bundle on Mac OS. Use \"NONE\" to not apply any icon, thereby making "
-        "the OS to show some default (default: apply PyInstaller's icon)",
+        "FILE.icns: apply the icon to the .app bundle on Mac OS. If an image file is entered that isn't in the "
+        "platform format (ico on Windows, icns on Mac), PyInstaller tries to use Pillow to translate the icon into "
+        "the correct format (if Pillow is installed). Use \"NONE\" to not apply any icon, thereby making the OS show "
+        "some default (default: apply PyInstaller's icon)",
     )
     g.add_argument(
         "--disable-windowed-traceback",
diff --git a/PyInstaller/building/osx.py b/PyInstaller/building/osx.py
index b0415f03b9f..24147c4ecc1 100644
--- a/PyInstaller/building/osx.py
+++ b/PyInstaller/building/osx.py
@@ -17,6 +17,7 @@
 from PyInstaller.building.datastruct import TOC, Target, logger
 from PyInstaller.building.utils import (_check_path_overlap, _rmtree, add_suffix_to_extension, checkCache)
 from PyInstaller.compat import is_darwin
+from PyInstaller.building.icon import normalize_icon_type
 
 if is_darwin:
     import PyInstaller.utils.osx as osxutils
@@ -41,8 +42,6 @@ def __init__(self, *args, **kws):
             # User gave an --icon=path. If it is relative, make it relative to the spec file location.
             if not os.path.isabs(self.icon):
                 self.icon = os.path.join(CONF['specpath'], self.icon)
-        # Ensure icon path is absolute
-        self.icon = os.path.abspath(self.icon)
 
         Target.__init__(self)
 
@@ -114,6 +113,8 @@ def _check_guts(self, data, last_build):
         return 1
 
     def assemble(self):
+        from PyInstaller.config import CONF
+
         if _check_path_overlap(self.name) and os.path.isdir(self.name):
             _rmtree(self.name)
         logger.info("Building BUNDLE %s", self.tocbasename)
@@ -123,11 +124,14 @@ def assemble(self):
         os.makedirs(os.path.join(self.name, "Contents", "Resources"))
         os.makedirs(os.path.join(self.name, "Contents", "Frameworks"))
 
+        # Makes sure the icon exists and attempts to convert to the proper format if applicable
+        self.icon = normalize_icon_type(self.icon, ("icns",), "icns", CONF["workpath"])
+
+        #Ensure icon path is absolute
+        self.icon = os.path.abspath(self.icon)
+
         # Copy icns icon to Resources directory.
-        if os.path.exists(self.icon):
-            shutil.copy(self.icon, os.path.join(self.name, 'Contents', 'Resources'))
-        else:
-            logger.warning("icon not found %s", self.icon)
+        shutil.copy(self.icon, os.path.join(self.name, 'Contents', 'Resources'))
 
         # Key/values for a minimal Info.plist file
         info_plist_dict = {
diff --git a/PyInstaller/utils/win32/icon.py b/PyInstaller/utils/win32/icon.py
index f59c0c5ff5c..7b363a9d586 100644
--- a/PyInstaller/utils/win32/icon.py
+++ b/PyInstaller/utils/win32/icon.py
@@ -23,6 +23,7 @@
 import PyInstaller.log as logging
 from PyInstaller import config
 from PyInstaller.compat import pywintypes, win32api
+from PyInstaller.building.icon import normalize_icon_type
 
 logger = logging.getLogger(__name__)
 
@@ -203,7 +204,12 @@ def splitter(s):
 
     # Just one source given.
     srcpath, index = srcpath[0]
+
+    # Makes sure the icon exists and attempts to convert to the proper format if applicable
+    srcpath = normalize_icon_type(srcpath, ("exe", "ico"), "ico", config.CONF["workpath"])
+
     srcext = os.path.splitext(srcpath)[1]
+
     # Handle the simple case of foo.ico, ignoring any index.
     if srcext.lower() == '.ico':
         return CopyIcons_FromIco(dstpath, [srcpath])
@@ -217,15 +223,6 @@ def splitter(s):
     else:
         logger.info("Copying icons from %s", srcpath)
 
-    # Bail out quickly if the input is invalid. Letting images in the wrong format be passed to Windows API gives very
-    # cryptic error messages, as it is generally unclear why PyInstaller would treat an image file as an executable.
-    if srcext != ".exe":
-        raise ValueError(
-            f"Received icon path '{srcpath}' which exists but is not in the correct format. On Windows, only '.ico' "
-            f"images or other '.exe' files may be used as icons. Please convert your '{srcext}' file to a '.ico' "
-            "and try again."
-        )
-
     try:
         # Attempt to load the .ico or .exe containing the icon into memory using the same mechanism as if it were a DLL.
         # If this fails for any reason (for example if the file does not exist or is not a .ico/.exe) then LoadLibraryEx
diff --git a/news/6697.feature.rst b/news/6697.feature.rst
new file mode 100644
index 00000000000..5cc7393b039
--- /dev/null
+++ b/news/6697.feature.rst
@@ -0,0 +1,2 @@
+Translate icon images into the correct platform format automatically
+using PIL/Pillow, if installed.
diff --git a/tests/functional/test_basic.py b/tests/functional/test_basic.py
index 093b2d579a4..948202be51e 100644
--- a/tests/functional/test_basic.py
+++ b/tests/functional/test_basic.py
@@ -13,7 +13,6 @@
 import locale
 import os
 import sys
-import shutil
 from pathlib import Path
 import subprocess
 import re
@@ -391,27 +390,6 @@ def test_set_icon(pyi_builder, data_dir):
     pyi_builder.test_source("print('Hello Python!')", pyi_args=args)
 
 
-@pytest.mark.win32
-def test_invalid_icon(tmpdir, data_dir):
-    """
-    Ensure a sane error message is given if the user provides a PNG or other unsupported format of image.
-    """
-    from PyInstaller import PLATFORM, HOMEPATH
-    from PyInstaller.utils.win32.icon import CopyIcons
-
-    icon = os.path.join(data_dir.strpath, 'pyi_icon.png')
-    bootloader_src = os.path.join(HOMEPATH, 'PyInstaller', 'bootloader', PLATFORM, "run.exe")
-    exe = os.path.join(tmpdir, "run.exe")
-    shutil.copy(bootloader_src, exe)
-    assert os.path.isfile(icon)
-    assert os.path.isfile(exe)
-
-    with pytest.raises(
-        ValueError, match="path '.*pyi_icon.png' .* not in the correct format.*convert your '.png' file to a '.ico' .*"
-    ):
-        CopyIcons(exe, icon)
-
-
 def test_python_home(pyi_builder):
     pyi_builder.test_script('pyi_python_home.py')
 
diff --git a/tests/unit/test_normalize_icon_type.py b/tests/unit/test_normalize_icon_type.py
new file mode 100644
index 00000000000..72d8f4ea8d1
--- /dev/null
+++ b/tests/unit/test_normalize_icon_type.py
@@ -0,0 +1,65 @@
+#-----------------------------------------------------------------------------
+# Copyright (c) 2005-2022, PyInstaller Development Team.
+#
+# Distributed under the terms of the GNU General Public License (version 2
+# or later) with exception for distributing the bootloader.
+#
+# The full license is in the file COPYING.txt, distributed with this software.
+#
+# SPDX-License-Identifier: (GPL-2.0-or-later WITH Bootloader-exception)
+#-----------------------------------------------------------------------------
+
+import os
+import sys
+
+import pytest
+
+
+def test_normalize_icon_type(monkeypatch):
+    from PyInstaller.building.icon import normalize_icon_type
+
+    # Currently assumes PIL/Pillow is installed on the testing machine
+
+    workpath = data_dir = "PyInstaller/bootloader/images"
+
+    # Nonexistent image - FileNotFoundError
+
+    icon = "this_is_not_a_file.ico"
+    with pytest.raises(FileNotFoundError):
+        normalize_icon_type(icon, ("ico",), "ico", workpath)
+
+    # Native image - file path is passed through unchanged
+
+    icon = os.path.join(data_dir, 'icon-console.ico')
+    ret = normalize_icon_type(icon, ("ico",), "ico", workpath)
+    if ret != icon:
+        pytest.fail("icon validation changed path even though the format was correct already", False)
+
+    # Alternative image - output is a different file with the correct suffix
+
+    icon = os.path.join(data_dir, 'github_logo.png')
+    ret = normalize_icon_type(icon, ("ico",), "ico", workpath)
+
+    _, ret_filetype = os.path.splitext(ret)
+    if ret_filetype != ".ico":
+        pytest.fail("icon validation didn't convert to the right format", False)
+
+    os.remove(ret)  # cleanup
+
+    # Some random non-image file: Raises an image conversion error
+
+    icon = os.path.join(data_dir, 'pyi_icon.notanicon')
+    with open(icon, "w") as f:
+        f.write("this is in fact, not an icon")
+
+    with pytest.raises(ValueError):
+        normalize_icon_type(icon, ("ico",), "ico", workpath)
+
+    os.remove(icon)  # cleanup
+
+    # Alternative image - after calling monkeypatch.setitem(sys.modules, "PIL", None): Raise the install pillow error
+
+    monkeypatch.setitem(sys.modules, "PIL", None)
+    icon = os.path.join(data_dir, 'github_logo.png')
+    with pytest.raises(ValueError):
+        normalize_icon_type(icon, ("ico",), "ico", workpath)


Code review comments:
diff_hunk: @@ -0,0 +1,65 @@
+#-----------------------------------------------------------------------------
+# Copyright (c) 2005-2022, PyInstaller Development Team.
+#
+# Distributed under the terms of the GNU General Public License (version 2
+# or later) with exception for distributing the bootloader.
+#
+# The full license is in the file COPYING.txt, distributed with this software.
+#
+# SPDX-License-Identifier: (GPL-2.0-or-later WITH Bootloader-exception)
+#-----------------------------------------------------------------------------
+
+import os
+import sys
+
+import pytest
+
+
+def test_normalize_icon_type(monkeypatch):
+    from PyInstaller.building.icon import normalize_icon_type
+
+    # Currently assumes PIL/Pillow is installed on the testing machine
+
+    workpath = data_dir = "PyInstaller/bootloader/images"
path: tests/unit/test_normalize_icon_type.py
text: @user1:
`data_dir` needs to be made independent of working directory and PyInstaller installation location. That's why the Alpine test is failing. The `workpath` should be a temporary directory (see [tmp_path fixture](https://docs.pytest.org/en/7.1.x/how-to/tmp_path.html#tmp-path)) so that we a) have test isolation and b) aren't in danger of accidentally uploading those generated images to PyPI the next time we do a release. Also, this would all be a lot neater using `pathlib.Path()` instead of `os.path`.


```suggestion
    from pathlib import Path
    import PyInstaller
    data_dir = Path(PyInstaller.__file__).with_name("bootloader") / "images"
```

@author:
I think I've got it now.

@user1:
You're rather missing the point of `pathlib`. The idea is that you leave `data_dir` as a `Path()` then later on you can use `icon = data_dir / 'icon-console.ico'` instead of `icon = os.path.join(data_dir, 'icon-console.ico')`. But that doesn't really matter.

@author:
I am new to pathlib, but I wanted to make sure the paths passed to the normalize function are strings, as that's what they will be in use.

Maybe pathlib paths are the same as strings for most things, but they aren't totally equivalent. In my time as a pygame contributor we've we had to put some special case code in to deal with Paths.

diff_hunk: @@ -0,0 +1,65 @@
+#-----------------------------------------------------------------------------
+# Copyright (c) 2005-2022, PyInstaller Development Team.
+#
+# Distributed under the terms of the GNU General Public License (version 2
+# or later) with exception for distributing the bootloader.
+#
+# The full license is in the file COPYING.txt, distributed with this software.
+#
+# SPDX-License-Identifier: (GPL-2.0-or-later WITH Bootloader-exception)
+#-----------------------------------------------------------------------------
+
+import os
+import sys
+
+import pytest
+
+
+def test_normalize_icon_type(monkeypatch):
+    from PyInstaller.building.icon import normalize_icon_type
+
+    # Currently assumes PIL/Pillow is installed on the testing machine
path: tests/unit/test_normalize_icon_type.py
text: @user1:
```suggestion
    pytest.importorskip("PIL")
```
Not any more! 

@author:
I put it down a bit so it could still fail earlier on the tests that don't need PIL.