Opened 7 years ago

Closed 7 years ago

#15539 closed enhancement (fixed)

use Pillow instead of PIL

Reported by: ohanar Owned by:
Priority: major Milestone: sage-6.1
Component: packages: standard Keywords:
Cc: Merged in:
Authors: R. Andrew Ohana Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: u/ohanar/pillow (Commits) Commit: f005905d5f6722838ac0c386f976f7116df2071c
Dependencies: Stopgaps:

Description (last modified by ohanar)

PIL is has not been maintained an a few years. Pillow is the canonical fork of PIL that most distributions have switch to using, we should do the same.

Upstream Tarball: Pillow-2.2.2.tar.gz

Change History (34)

comment:1 Changed 7 years ago by ohanar

  • Status changed from new to needs_review

comment:2 Changed 7 years ago by ohanar

  • Description modified (diff)

comment:3 Changed 7 years ago by git

  • Commit changed from 6690d9792a30924d89eb921f9e3905a204cb5690 to 2c055bd4a696b07e5d9071686a00da1d58ee0838

Branch pushed to git repo; I updated commit sha1. New commits:

2c055bdPillow: fix patch to work against 2.2.2

comment:4 follow-up: Changed 7 years ago by fbissey

sage/plot/plot3d/base.pyx needs to be fixed as well: import Image needs to be replaced by from PIL import Image

comment:5 in reply to: ↑ 4 Changed 7 years ago by ohanar

Replying to fbissey:

sage/plot/plot3d/base.pyx needs to be fixed as well

part of this commit: http://git.sagemath.org/sage.git/commit/?h=u/ohanar/pillow&id=6df0adb542b651cdfde5c9088edf18522963aff0

comment:6 follow-up: Changed 7 years ago by fbissey

Great, one less patch in sage-on-gentoo. I don't see that commit attached to this ticket. Is it attached to another ticket or work in progress?

comment:7 in reply to: ↑ 6 Changed 7 years ago by ohanar

Replying to fbissey:

Great, one less patch in sage-on-gentoo. I don't see that commit attached to this ticket.

It's attached to this ticket -- you can see all the commits on the branch for this ticket at http://git.sagemath.org/sage.git/log/?h=u/ohanar/pillow.

comment:8 follow-up: Changed 7 years ago by fbissey

OK, I am not properly used to the way git and trac integrate. Hit me on the head with it a few more times and I'll get it.

I see you ditched the variables for binary distribution (were they even used anymore with the old pil? apparently yes.). Good riddance, did you "steal" the setup.py patch from somewhere else? Gentoo has a similar one. I think the only missing from that ticket is the removal of pil itself.

comment:9 in reply to: ↑ 8 ; follow-ups: Changed 7 years ago by ohanar

Replying to fbissey:

I see you ditched the variables for binary distribution (were they even used anymore with the old pil? apparently yes.).

They weren't even the standard binary environment variables, and they relied on experimental spkgs.

Good riddance, did you "steal" the setup.py patch from somewhere else?

It's a patch that I submitted upstream (was originally based on the development branch - hence broken, but now fixed).

Gentoo has a similar one. I think the only missing from that ticket is the removal of pil itself.

I see no reason to delete PIL -- it can just be an optional/experiment spkg.

comment:10 in reply to: ↑ 9 Changed 7 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

Replying to ohanar:

Replying to fbissey:

I think the only missing from that ticket is the removal of pil itself.

I see no reason to delete PIL -- it can just be an optional/experiment spkg.

I see no reason to keep something that is, for all purpose, dead. But the point is minor and I am quite ready to give this a positive review.

comment:11 Changed 7 years ago by vbraun

  • Status changed from positive_review to needs_info

The tarball works but don't we lower-case the packages? E.g. "r" instead of "R".

comment:12 Changed 7 years ago by fbissey

I must say I am not aware of any such policies in sage but it is not uncommon in linux distro to "prefer" lower case, funny you brought R up because it is usually one of the exception.

comment:13 Changed 7 years ago by vbraun

I would be in favor of having everything lower case, otherwise it'll be annoying to type on the commandline build/pkgs/PyCrypto etc... But then its not a big deal if you insist on capital letters for Pillow ;-)

comment:14 Changed 7 years ago by fbissey

I will leave the final word to our fellow author on the capital "P" not a big deal but I appreciate your reasons.

comment:15 in reply to: ↑ 9 Changed 7 years ago by jhpalmieri

Replying to ohanar:

Replying to fbissey:

I see you ditched the variables for binary distribution (were they even used anymore with the old pil? apparently yes.).

They weren't even the standard binary environment variables, and they relied on experimental spkgs.

Good riddance, did you "steal" the setup.py patch from somewhere else?

It's a patch that I submitted upstream (was originally based on the development branch - hence broken, but now fixed).

Gentoo has a similar one. I think the only missing from that ticket is the removal of pil itself.

I see no reason to delete PIL -- it can just be an optional/experiment spkg.

I think it would make more sense to remove build/pkgs/pil/: we don't have any files in build/pkgs for other optional/experimental packages, do we? I actually don't know how optional packages are implemented: still as spkgs? Is that going to change?

I would also vote for a lowercase "p".

comment:16 Changed 7 years ago by fbissey

Not all optional/experimental packages have been converted but yes I think they are supposed to go in build/pkgs. If you have a look you'll see some optional stuff already.

comment:17 Changed 7 years ago by jhpalmieri

Oh, I see. Then keep the directory, of course.

comment:18 Changed 7 years ago by ohanar

IMO, it would be better to have package names be the same as the upstream version (including capitalization), I would also vote to have this changed for other packages (such as R or Python). (Also, it is not hard to setup your shell [well at least zsh] to be case-insensitive for tab completion.) However, it seems like most disagree, so I can change this when I have a chance.

Actually, I think a good reason to remove PIL would be conflicts from having both Pillow and PIL installed side-by-side (or PIL instead of Pillow). I would rather not have to maintain support for both libraries.

comment:19 Changed 7 years ago by jhpalmieri

I think that the naming convention must still be the old SPKG naming rule: only lowercase letters, numbers, and underscores. I personally wouldn't object to changing this convention, but that needs to be discussed on sage-devel, not here.

If we end up changing it, then because of the brain-dead way the standard OS X file system treats upper and lower case, I think we can never have two packages whose name only differs in capitalization. So then we should have commands like sage -i PKG_NAME ignore case, to make sage -i Pillow and sage -i pillow (for example) perform the same function.

comment:20 Changed 7 years ago by vbraun

Since Pillow is a backward-compatible replacement for PIL I'm also in favor of removing PIL.

comment:21 Changed 7 years ago by git

  • Commit changed from 2c055bd4a696b07e5d9071686a00da1d58ee0838 to adc90cead77a91f17ea85dcbbfe77d5aba881465

Branch pushed to git repo; I updated commit sha1. New commits:

adc90cePillow -> pillow
8987381remove PIL (it has been replaced by Pillow)

comment:22 Changed 7 years ago by ohanar

  • Status changed from needs_info to needs_review

comment:23 Changed 7 years ago by fbissey

  • Status changed from needs_review to positive_review

I think that covers all the issue raised for this particular package. So back to positive review. Not who to put in reviewer(s) though.

Version 0, edited 7 years ago by fbissey (next)

comment:24 follow-up: Changed 7 years ago by vbraun

checksums.ini still has capital-P(illow) in it...

comment:25 Changed 7 years ago by vbraun

The tarball is already on the server, for the record. Please make sure that it downloads...

comment:26 in reply to: ↑ 24 Changed 7 years ago by ohanar

Replying to vbraun:

checksums.ini still has capital-P(illow) in it...

Yes, that is a fully conscious decision, as it matches the tarball that is provided by upstream. I think it is much better to just use the upstream tarball in this case, rather than untaring, changing the name of the untared directory, and then retaring (which is what one must do if they decapitalize Pillow in checksums.ini). This is part of the reason why I think that matching upstreams capitalization is preferable.

comment:27 follow-up: Changed 7 years ago by vbraun

Ok, good. But it still doesn't build.

comment:29 in reply to: ↑ 27 Changed 7 years ago by ohanar

Replying to vbraun:

Ok, good. But it still doesn't build.

Works fine on OSX (case-insensitive). The problem is probably because the package was misplaced on boxen (should be .../Pillow/Pillow-*.tar.gz not .../pillow/Pillow-*.tar.gz). (Again, less breakage if we just matched upstream's capitalization).

comment:30 follow-up: Changed 7 years ago by vbraun

Surely the OSX filesystem has no influence on whether *downloading* http://www.sagemath.org/packages/upstream//Pillow/Pillow-2.2.2.tar.gz works or not? If it works for you then it must be because you have the tarball already in your upstream directory.

comment:31 in reply to: ↑ 30 Changed 7 years ago by ohanar

Replying to vbraun:

Surely the OSX filesystem has no influence on whether *downloading* http://www.sagemath.org/packages/upstream//Pillow/Pillow-2.2.2.tar.gz works or not?

Maybe it doesn't have to do with the OSX filesystem (although, maybe python's urllib stuff builds to be case-insensitive on a case-insensitive filesystem).

If it works for you then it must be because you have the tarball already in your upstream directory.

No, I purposely deleted it to test, and it indeed downloaded a new tarball.

comment:32 Changed 7 years ago by vbraun

Ok, seems to download now. But build fails on a bunch of older machines (bsd, eno, cicero). Missing dependency?

Found local metadata for pillow-2.2.2
Attempting to download package pillow-2.2.2
>>> Trying to download http://www.sagemath.org/packages/upstream//pillow/Pillow-2.2.2.tar.gz
[............................................................]
Checksum: 053337a612dd16ec6f1f6fc544374ca5fe65ae2c vs 053337a612dd16ec6f1f6fc544374ca5fe65ae2c
pillow-2.2.2
====================================================
Setting up build directory for pillow-2.2.2
Finished set up
****************************************************
Host system:
Darwin bsd.local 10.8.0 Darwin Kernel Version 10.8.0: Tue Jun  7 16:32:41 PDT 2011; root:xnu-1504.15.3~1/RELEASE_X86_64 x86_64
****************************************************
C compiler: gcc
C compiler version:
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/Users/buildbot/build/sage/bsd-1/sage_git/build/local/libexec/gcc/x86_64-apple-darwin10.8.0/4.7.3/lto-wrapper
Target: x86_64-apple-darwin10.8.0
Configured with: ../src/configure --prefix=/Users/buildbot/build/sage/bsd-1/sage_git/build/local --with-local-prefix=/Users/buildbot/build/sage/bsd-1/sage_git/build/local --with-gmp=/Users/buildbot/build/sage/bsd-1/sage_git/build/local --with-mpfr=/Users/buildbot/build/sage/bsd-1/sage_git/build/local --with-mpc=/Users/buildbot/build/sage/bsd-1/sage_git/build/local --with-system-zlib --disable-multilib --disable-nls  
Thread model: posix
gcc version 4.7.3 (GCC) 
****************************************************
patching file setup.py
Traceback (most recent call last):
  File "setup.py", line 19, in <module>
    from setuptools import Extension, setup, find_packages
ImportError: No module named setuptools
Error building / installing Pillow

real    0m0.069s
user    0m0.038s
sys     0m0.028s
************************************************************************
Error installing package pillow-2.2.2
************************************************************************

comment:33 Changed 7 years ago by git

  • Commit changed from adc90cead77a91f17ea85dcbbfe77d5aba881465 to f005905d5f6722838ac0c386f976f7116df2071c
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

f005905pillow: depends on setuptools

comment:34 Changed 7 years ago by vbraun

  • Resolution set to fixed
  • Status changed from needs_review to closed
Note: See TracTickets for help on using tickets.