pillow: update to version 6.0.0, add Python3 variant #8715

BKPepe · 2019-04-18T16:16:50Z

Maintainer: none
Compile tested: Turris MOX, cortexa53, OpenWrt master
Run tested: Turris MOX, cortexa53, OpenWrt master
together with Create thumbnails from their documentation

Description:

add Python3 variant
update it to version 6.0.0
seafile-seahub, django-simple-captcha: renamed dependency pillow to python-pillow

BKPepe · 2019-04-24T19:20:46Z

Added zlib support and currently supported:

Python 2.7

--------------------------------------------------------------------
PIL SETUP SUMMARY
--------------------------------------------------------------------
version      Pillow 6.0.0
platform     linux2 2.7.16 (default, Mar 29 2019, 23:23:42)
             [GCC 8.2.0]
--------------------------------------------------------------------
--- JPEG support available
*** OPENJPEG (JPEG2000) support not available
--- ZLIB (PNG/ZIP) support available
*** LIBIMAGEQUANT support not available
--- LIBTIFF support available
*** FREETYPE2 support not available
*** LITTLECMS2 support not available
*** WEBP support not available
*** WEBPMUX support not available
--------------------------------------------------------------------

Python 3.7

--------------------------------------------------------------------
PIL SETUP SUMMARY
--------------------------------------------------------------------
version      Pillow 6.0.0
platform     linux2 3.7.2 (default, Mar 29 2019, 23:23:42)
             [GCC 8.2.0]
--------------------------------------------------------------------
--- JPEG support available
*** OPENJPEG (JPEG2000) support not available
--- ZLIB (PNG/ZIP) support available
*** LIBIMAGEQUANT support not available
--- LIBTIFF support available
*** FREETYPE2 support not available
*** LITTLECMS2 support not available
*** WEBP support not available
*** WEBPMUX support not available
--------------------------------------------------------------------

According to their doc: https://pillow.readthedocs.io/en/stable/installation.html

Zlib and libjpeg are required by default.

cc: @cotequeiroz (it is dependency for python-qrcode) and Python maintainers: @jefferyto , @commodo (who would like to take maintainership of this package)

cotequeiroz

LGTM, independently of my suggestion to add the maintainer.

lang/python/pillow/Makefile

commodo · 2019-04-25T07:34:41Z

lang/python/pillow/patches/0001-setup.py-use-patch-from-Debian-and-tweak-it.patch

+                                   define_macros=defs))
+
+-        tk_libs = ['psapi'] if sys.platform == 'win32' else []
+        tk_libs = ['psapi'] if host_platform == 'win32' else []


why does this need to change ?
or is it needed ?
not sure if it will be true, ever ?

I'm not really sure why the original Debian patch replaces sys.platform with host_platform (set from sysconfig.get_platform()), but if I had to guess I would say for cross-compilation reasons.

I think we should probably continue replacing sys.platform with host_platform as well, but instead set host_platform = "linux". In theory, this could be compiling on macOS where sys.platform (for host Python) would be `"darwin" (with similar changes on other host OSes).

commodo · 2019-04-25T07:36:01Z

lang/python/pillow/patches/0001-setup.py-use-patch-from-Debian-and-tweak-it.patch

+-                if (sys.platform in ["win32", "darwin"] and
+-                        _find_library_file(self, "libtiff")):
+-                    feature.tiff = "libtiff"
+            feature.tiff= "tiff"


if you just add feature.tiff= "tiff" here and leave the removed lines, it has the same effect
makes patch smaller

Suggested change

+ feature.tiff= "tiff"

+ feature.tiff= "tiff" # openwrt: always use tiff

If we are to do this, I would suggest to add a comment--inline to keep your intention to keep the patch small--to indicate we're trumping the above code. It should make it easier to understand what's going on.

commodo · 2019-04-25T07:36:27Z

lang/python/pillow/patches/0001-setup.py-use-patch-from-Debian-and-tweak-it.patch

+-                elif (sys.platform == "win32" and
+-                      _find_library_file(self, "libjpeg")):
+-                    feature.jpeg = "libjpeg"  # alternative name
+            feature.jpeg = "jpeg"


same comment here for leaving removed lines here

commodo · 2019-04-25T07:36:30Z

lang/python/pillow/patches/0001-setup.py-use-patch-from-Debian-and-tweak-it.patch

+-                elif (sys.platform == "win32" and
+-                      _find_library_file(self, "zlib")):
+-                    feature.zlib = "zlib"  # alternative name
+            feature.zlib = "z"


same comment here for leaving removed lines here

I wonder if hard-coding feature.zlib = "z" (and the other lines for the other libraries) is really necessary. If the paths are correct, setup should be able to find the correct libraries. (I would again replace sys.platforms with host_platform as in the Debian patch.)

If it isn't able to find the libraries, we should find out why and fix that.

commodo · 2019-04-25T07:37:02Z

lang/python/pillow/patches/0001-setup.py-use-patch-from-Debian-and-tweak-it.patch

+-
+-        # on Windows, look for the OpenJPEG libraries in the location that
+-        # the official installer puts them
+-        if sys.platform == "win32":


maybe just leave these lines here for win32;
makes patch smaller

Actually can leave all the different branches in place (and replace sys.platforms with host_platform as in the Debian patch), since none of them should match.

commodo · 2019-04-25T07:38:20Z

lang/python/pillow/patches/0001-setup.py-use-patch-from-Debian-and-tweak-it.patch

+         #
+-        # add platform directories
+-
+-        if self.disable_platform_guessing:


for this if block removal; same idea: instead of removing it, just add

self.disable_platform_guessing = True

and all is well, patch is smaller;

it could be that self.disable_platform_guessing can be disabled via some CLI arg;
i would need to check

commodo · 2019-04-25T07:39:42Z

lang/python/pillow/patches/0001-setup.py-use-patch-from-Debian-and-tweak-it.patch

+                     _add_directory(library_dirs, d)
+
+-        prefix = sysconfig.get_config_var("prefix")
+        prefix = os.environ.get('PYTHONXCPREFIX')


curios: is this necessary ?
typically the prefix can be obtained from the CONFIGURE_ARGS reasonably ok
if this isn't true, we can try to fix it
i wouldn't mind having this here either; but if we could not patch this, it would be better

Also, it already reads from several environment variables, which should have all the paths it needs?

Also... I don't see where PYTHONXCPREFIX is defined.

I would be interested to see what sysconfig.get_config_var("prefix") returns.

lang/python/pillow/Makefile

cotequeiroz · 2019-04-26T16:50:47Z

@BKPepe
Please add a commit updating python-qrcode as well.

You may want to add a PROVIDES:=pillow line to python-pillow to avoid breakage for people who built their own dependent packages. You may add a note that this will be removed in the future (next upgrade?).

jefferyto · 2019-04-29T08:56:52Z

lang/python/pillow/patches/0001-setup.py-use-patch-from-Debian-and-tweak-it.patch

+-TIFF_ROOT = None
+-FREETYPE_ROOT = None
+TIFF_ROOT = os.path.join(os.environ['STAGING_DIR'], "/usr/lib")
+FREETYPE_ROOT = os.path.join(os.environ['STAGING_DIR'], "/usr/lib")


It looks like build_extensions() already checks for environment variables, so you can set JPEG_ROOT, ZLIB_ROOT, etc. as variables from the Makefile instead of patching here.

But more importantly, it's suppose to use pkg-config to find these paths. I'd be interested to see why it doesn't work and perhaps fix that, instead of manually setting the paths.

jefferyto · 2019-04-29T09:08:21Z

lang/python/pillow/patches/0001-setup.py-use-patch-from-Debian-and-tweak-it.patch

+                            _add_directory(self.compiler.include_dirs,
+                                           line.strip())
+        finally:
+            os.unlink(tmpfile)


I know this comes from the Debian patch, but AFAICT this function (add_gcc_paths()) isn't called anywhere. Can you try removing this?

jefferyto · 2019-04-29T11:25:09Z

lang/python/pillow/patches/0001-setup.py-use-patch-from-Debian-and-tweak-it.patch

+                     _add_directory(library_dirs, d)
+
+-        prefix = sysconfig.get_config_var("prefix")
+        prefix = os.environ.get('PYTHONXCPREFIX')


Also, it already reads from several environment variables, which should have all the paths it needs?

Also... I don't see where PYTHONXCPREFIX is defined.

I would be interested to see what sysconfig.get_config_var("prefix") returns.

jefferyto · 2019-04-29T11:28:48Z

lang/python/pillow/patches/0001-setup.py-use-patch-from-Debian-and-tweak-it.patch

+-
+-        # on Windows, look for the OpenJPEG libraries in the location that
+-        # the official installer puts them
+-        if sys.platform == "win32":


Actually can leave all the different branches in place (and replace sys.platforms with host_platform as in the Debian patch), since none of them should match.

jefferyto · 2019-04-29T11:31:24Z

lang/python/pillow/patches/0001-setup.py-use-patch-from-Debian-and-tweak-it.patch

+-                elif (sys.platform == "win32" and
+-                      _find_library_file(self, "zlib")):
+-                    feature.zlib = "zlib"  # alternative name
+            feature.zlib = "z"


I wonder if hard-coding feature.zlib = "z" (and the other lines for the other libraries) is really necessary. If the paths are correct, setup should be able to find the correct libraries. (I would again replace sys.platforms with host_platform as in the Debian patch.)

If it isn't able to find the libraries, we should find out why and fix that.

jefferyto · 2019-04-29T11:36:26Z

lang/python/pillow/patches/0001-setup.py-use-patch-from-Debian-and-tweak-it.patch

+                                   define_macros=defs))
+
+-        tk_libs = ['psapi'] if sys.platform == 'win32' else []
+        tk_libs = ['psapi'] if host_platform == 'win32' else []


I'm not really sure why the original Debian patch replaces sys.platform with host_platform (set from sysconfig.get_platform()), but if I had to guess I would say for cross-compilation reasons.

I think we should probably continue replacing sys.platform with host_platform as well, but instead set host_platform = "linux". In theory, this could be compiling on macOS where sys.platform (for host Python) would be `"darwin" (with similar changes on other host OSes).

jefferyto · 2019-04-29T11:39:03Z

lang/python/pillow/Makefile

+  CATEGORY:=Languages
+  TITLE:=The friendly PIL fork
+  URL:=https://python-pillow.org/
+  DEPENDS:=+libjpeg +libtiff +zlib


libfreetype was also a dependency before... Are we sure it isn't still necessary (especially since this is a dependency of django-simple-captcha and captchas involve text...)?

I couldn't compile with libfreetype, that's why I removed it.

jefferyto · 2019-04-29T11:47:15Z

Hmm... starting a review meant my comments are displayed separately from @commodo's. Wish I knew that beforehand.

BKPepe · 2019-04-29T11:49:17Z

Guys, you really give me a hard time. :-) Anyway, hard-coding feature.zlib = "z" works and without it doesn't work. I was fighting with it for a while, but if you enable debug it will show you more outputs. Not sure, if I would be able to look at all the feedback today.

jefferyto · 2019-04-29T11:57:27Z

@BKPepe 😂 if I have time later I might try compiling this as well

BKPepe · 2019-04-29T12:11:28Z

It's not funny. :( I was so happy that I was able to compile Pillow for Python3 and this is not what I expected. :D

jefferyto · 2019-04-29T12:16:01Z

If it's any consolation, I'm laughing with you not at you 😆

jefferyto · 2019-05-01T22:26:27Z

I've tried compiling this, and I was able to do so with these changes:

Add back libfreetype as a dependency (I had no issues when compiling this)
Add this to the Makefile (options are from the documentation):
```
PYTHON3_PKG_SETUP_GLOBAL_ARGS += build_ext \
	--enable-zlib \
	--enable-jpeg \
	--enable-tiff \
	--enable-freetype \
	--disable-lcms \
	--disable-webp \
	--disable-webpmux \
	--disable-jpeg2000 \
	--disable-imagequant \
	--disable-platform-guessing

PYTHON_PKG_SETUP_GLOBAL_ARGS:=$(PYTHON3_PKG_SETUP_GLOBAL_ARGS)
```
I think it's better to explicitly enable and disable features here, because setup will throw an error if it could not find the library for an enabled feature, and disabling unwanted features means it won't accidentally detect other libraries that may be compiled in the same buildroot.

I also like --disable-platform-guessing as it bypasses most of the auto-detection of library directories in platform-specific locations.
Starting with the Debian patch:
- Replace:
```
from sysconfig import get_platform
host_platform = get_platform()
```
  with:
```
host_platform = "linux"
```
  With --disable-platform-guessing, replacing sys.platform with host_platform is almost unnecessary, except there is still one place where setup may think it is compiling for darwin and try to link to the "wrong" library.
- Remove the add_gcc_paths() method; it isn't called anywhere.
- Add CPPFLAGS to line 334, so that the updated code is:
```
        for k in ('CFLAGS', 'CPPFLAGS', 'LDFLAGS'):
```
  This is the main reason why setup wasn't able to find the libraries; it is looking for -I (and also -L) options in CFLAGS and LDFLAGS to find includes and library directories, but with OpenWrt our -I directives are defined in CPPFLAGS. I have already submitted this upstream (Include CPPFLAGS when searching for libraries python-pillow/Pillow#3819).
  
  As I mentioned earlier, setup also uses pkg-config to find include/library directories; here I found some other issues (the libjpeg and libtiff packages do not include their pkg-config data file during InstallDev, the zlib pkg-config data file variables are not properly replaced by buildroot's pkg-config) that I'll try to resolve with some other PRs, but for this package just including CPPFLAGS is good enough.

@commodo

- Add @commodo as maintainer Co-Authored-By: Jeffery To <jeffery.to@gmail.com> Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>

BKPepe · 2019-07-09T20:11:02Z

Many thanks to @jefferyto for his work. I updated pillow to version 6.1.0, which includes his fix. Removed entirely Debian patch as it was replacing sys.platform with host_platform and it's not necessary except one thing, which was mention in comment #8715 (comment), but we would see if any issues about it will appear.

--------------------------------------------------------------------
PIL SETUP SUMMARY
--------------------------------------------------------------------
version      Pillow 6.1.0
platform     linux 3.7.4 (default, May 17 2019, 14:51:37)
             [GCC 8.3.0]
--------------------------------------------------------------------
--- JPEG support available
*** OPENJPEG (JPEG2000) support not available
--- ZLIB (PNG/ZIP) support available
*** LIBIMAGEQUANT support not available
--- LIBTIFF support available
--- FREETYPE2 support available
*** LITTLECMS2 support not available
*** WEBP support not available
*** WEBPMUX support not available
--------------------------------------------------------------------

BKPepe · 2019-07-09T20:27:33Z

Tested on master, both variants of Pillow with example Create thumbnails.
@jefferyto , @commodo any thoughts before merging?

commodo · 2019-07-10T07:52:04Z

Thank you :)

jefferyto

Glad it worked out 😂

BKPepe marked this pull request as ready for review April 24, 2019 19:21

BKPepe force-pushed the pillow branch from 784a7bf to 2c38b62 Compare April 24, 2019 19:21

cotequeiroz approved these changes Apr 24, 2019

View reviewed changes

lang/python/pillow/Makefile Show resolved Hide resolved

BKPepe force-pushed the pillow branch 3 times, most recently from 63adb6a to 602a7e6 Compare April 24, 2019 22:44

commodo reviewed Apr 25, 2019

View reviewed changes

lang/python/pillow/Makefile Show resolved Hide resolved

commodo reviewed Apr 25, 2019

View reviewed changes

lang/python/pillow/Makefile Outdated Show resolved Hide resolved

BKPepe force-pushed the pillow branch from 602a7e6 to 6418dd4 Compare April 26, 2019 12:16

jefferyto reviewed Apr 29, 2019

View reviewed changes

BKPepe mentioned this pull request Jul 5, 2019

django packages: convert to python packaging format + add python3 variant #9400

Merged

BKPepe force-pushed the pillow branch 3 times, most recently from 3f17009 to a1c5a3f Compare July 9, 2019 19:59

pillow: add Python3 variant and update to 6.1.0

f265bfb

- Add @commodo as maintainer Co-Authored-By: Jeffery To <jeffery.to@gmail.com> Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>

BKPepe force-pushed the pillow branch from a1c5a3f to f265bfb Compare July 9, 2019 20:06

commodo approved these changes Jul 10, 2019

View reviewed changes

jefferyto approved these changes Jul 10, 2019

View reviewed changes

BKPepe merged commit d9b46c2 into openwrt:master Jul 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pillow: update to version 6.0.0, add Python3 variant #8715

pillow: update to version 6.0.0, add Python3 variant #8715

BKPepe commented Apr 18, 2019 •

edited

BKPepe commented Apr 24, 2019

cotequeiroz left a comment

commodo Apr 25, 2019

jefferyto Apr 29, 2019

commodo Apr 25, 2019

cotequeiroz Apr 25, 2019

commodo Apr 25, 2019

commodo Apr 25, 2019

jefferyto Apr 29, 2019

commodo Apr 25, 2019

jefferyto Apr 29, 2019

commodo Apr 25, 2019

commodo Apr 25, 2019

jefferyto Apr 29, 2019

cotequeiroz commented Apr 26, 2019

jefferyto Apr 29, 2019

jefferyto Apr 29, 2019

jefferyto Apr 29, 2019

jefferyto Apr 29, 2019

jefferyto Apr 29, 2019

jefferyto Apr 29, 2019

jefferyto Apr 29, 2019

BKPepe Apr 29, 2019

jefferyto commented Apr 29, 2019

BKPepe commented Apr 29, 2019

jefferyto commented Apr 29, 2019

BKPepe commented Apr 29, 2019

jefferyto commented Apr 29, 2019

jefferyto commented May 1, 2019

BKPepe commented Jul 9, 2019

BKPepe commented Jul 9, 2019

commodo commented Jul 10, 2019

jefferyto left a comment

	+ feature.tiff= "tiff"
	+ feature.tiff= "tiff" # openwrt: always use tiff

pillow: update to version 6.0.0, add Python3 variant #8715

pillow: update to version 6.0.0, add Python3 variant #8715

Conversation

BKPepe commented Apr 18, 2019 • edited

BKPepe commented Apr 24, 2019

cotequeiroz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cotequeiroz commented Apr 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefferyto commented Apr 29, 2019

BKPepe commented Apr 29, 2019

jefferyto commented Apr 29, 2019

BKPepe commented Apr 29, 2019

jefferyto commented Apr 29, 2019

jefferyto commented May 1, 2019

BKPepe commented Jul 9, 2019

BKPepe commented Jul 9, 2019

commodo commented Jul 10, 2019

jefferyto left a comment

Choose a reason for hiding this comment

BKPepe commented Apr 18, 2019 •

edited