Opened 9 years ago
Closed 9 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, GitHub, GitLab) | Commit: | f005905d5f6722838ac0c386f976f7116df2071c |
Dependencies: | Stopgaps: |
Description (last modified by )
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 9 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 9 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 9 years ago by
Commit: | 6690d9792a30924d89eb921f9e3905a204cb5690 → 2c055bd4a696b07e5d9071686a00da1d58ee0838 |
---|
comment:4 follow-up: 5 Changed 9 years ago by
sage/plot/plot3d/base.pyx needs to be fixed as well: import Image needs to be replaced by from PIL import Image
comment:5 Changed 9 years ago by
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: 7 Changed 9 years ago by
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 Changed 9 years ago by
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: 9 Changed 9 years ago by
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 follow-ups: 10 15 Changed 9 years ago by
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 Changed 9 years ago by
Reviewers: | → François Bissey |
---|---|
Status: | needs_review → 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 9 years ago by
Status: | positive_review → needs_info |
---|
The tarball works but don't we lower-case the packages? E.g. "r" instead of "R".
comment:12 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
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 Changed 9 years ago by
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 9 years ago by
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:18 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
Since Pillow is a backward-compatible replacement for PIL I'm also in favor of removing PIL.
comment:21 Changed 9 years ago by
Commit: | 2c055bd4a696b07e5d9071686a00da1d58ee0838 → adc90cead77a91f17ea85dcbbfe77d5aba881465 |
---|
comment:22 Changed 9 years ago by
Status: | needs_info → needs_review |
---|
comment:23 Changed 9 years ago by
Status: | needs_review → positive_review |
---|
I think that covers all the issue raised for this particular package. So back to positive review. Not sure who to put in reviewer(s) though.
comment:25 Changed 9 years ago by
The tarball is already on the server, for the record. Please make sure that it downloads...
comment:26 Changed 9 years ago by
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:28 Changed 9 years ago by
comment:29 Changed 9 years ago by
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: 31 Changed 9 years ago by
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 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
Commit: | adc90cead77a91f17ea85dcbbfe77d5aba881465 → f005905d5f6722838ac0c386f976f7116df2071c |
---|---|
Status: | positive_review → needs_review |
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
f005905 | pillow: depends on setuptools
|
comment:34 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | needs_review → closed |
Branch pushed to git repo; I updated commit sha1. New commits:
Pillow: fix patch to work against 2.2.2