Skip to content

Changed how Django detects if Pillow is installed & placed the PIL on th... #1059

Closed
wants to merge 3 commits into from

2 participants

@toastdriven

...e deprecation path.

This is meant to address #19934.

@jacobian jacobian and 1 other commented on an outdated diff May 13, 2013
django/utils/image.py
+ raise ImproperlyConfigured(
+ _(u"The '_imaging' module for the PIL could not be " +
+ u"imported. Please reinstall, ensuring a C compiler is " +
+ u"available.")
+ )
+
+ # Try to import ImageFile as well.
+ try:
+ from PIL import ImageFile as PILImageFile
+ except ImportError:
+ import ImageFile as PILImageFile
+
+ # Finally, warn about deprecation...
+ if PIL_imaging is False:
+ warnings.warn(
+ "Support for the PIL will be removed in Django 1.8.",
@jacobian
Django member
jacobian added a note May 13, 2013

Grammar? Also maybe say "use Pillow instead"?

@toastdriven
toastdriven added a note May 14, 2013

Sorry, in which part is my grammar poor? Should it be a complete sentence?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jacobian jacobian commented on an outdated diff May 13, 2013
django/utils/image.py
+
+ # Assume it's not there.
+ PIL_imaging = False
+
+ try:
+ # Try from the Pillow (or one variant of PIL) install location first.
+ from PIL import Image as PILImage
+ except ImportError as err:
+ try:
+ # If that failed, try the alternate import syntax for PIL.
+ import Image as PILImage
+ except ImportError as err:
+ # Neither worked, so it's likely not installed.
+ raise ImproperlyConfigured(
+ _(u"Neither Pillow nor PIL could be imported. Please install " +
+ u"one of them.")
@jacobian
Django member
jacobian added a note May 13, 2013

Probably should include the ImportError exception here (e.g. "Neither Pillow nor PIL could be imported: %s" % err) just in case the import failed for some weird reason other than a missing install.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jacobian jacobian commented on the diff May 13, 2013
django/utils/image.py
+
+* Pillow:
+ * never has ``_imaging`` under any Python
+ * has the ``Image.alpha_composite``, which may aid in detection
+* PIL
+ * CPython 2.x may have _imaging (& work)
+ * CPython 2.x may *NOT* have _imaging (broken & needs a error message)
+ * CPython 3.x doesn't work
+ * PyPy will *NOT* have _imaging (but works?)
+
+Restated, that looks like:
+
+* Python 2.x
+ * ``_imaging`` *NOT* present
+ * ``Image.alpha_composite`` present - Pillow & working
+ * ``Image.alpha_composite`` *NOT* present - PIL & broken
@jacobian
Django member
jacobian added a note May 13, 2013

Can you explain a bit more about why a missing alpha_composite means that PIL is "broken"? I don't quite follow.

@toastdriven
toastdriven added a note May 14, 2013

The situation is that import Image works (could be either PIL or Pillow), import _imaging does NOT work (either broken PIL or working Pillow) & Image does NOT have an attribute alpha_composite (definitely PIL). Since _imaging isn't present, they have a broken install of the PIL & need to reinstall.

@jacobian
Django member
jacobian added a note May 14, 2013

OK, I'd add that to the docstring to help make that clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jacobian jacobian commented on the diff May 13, 2013
django/utils/image.py
+ )
+
+ # ``Image.alpha_composite`` was added to Pillow in SHA: e414c6 & is not
+ # available in any version of the PIL.
+ if hasattr(PILImage, u'alpha_composite'):
+ PIL_imaging = False
+ else:
+ # We're dealing with the PIL. Determine if we're on CPython & if
+ # ``_imaging`` is available.
+ import platform
+
+ # This is the Alex Approved™ way.
+ # See http://mail.python.org/pipermail//pypy-dev/2011-November/008739.html
+ if platform.python_implementation().lower() == u'cpython':
+ # We're on CPython (likely 2.x). Since a C compiler is needed to
+ # produce a fully-working PIL & will create a ``_
@jacobian
Django member
jacobian added a note May 13, 2013

Seems like perhaps this comment is cut off?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jacobian jacobian commented on an outdated diff May 13, 2013
django/utils/image.py
+ # We're dealing with the PIL. Determine if we're on CPython & if
+ # ``_imaging`` is available.
+ import platform
+
+ # This is the Alex Approved™ way.
+ # See http://mail.python.org/pipermail//pypy-dev/2011-November/008739.html
+ if platform.python_implementation().lower() == u'cpython':
+ # We're on CPython (likely 2.x). Since a C compiler is needed to
+ # produce a fully-working PIL & will create a ``_
+ try:
+ import _imaging as PIL_imaging
+ except ImportError as err:
+ raise ImproperlyConfigured(
+ _(u"The '_imaging' module for the PIL could not be " +
+ u"imported. Please reinstall, ensuring a C compiler is " +
+ u"available.")
@jacobian
Django member
jacobian added a note May 13, 2013

As above, echoing the ImportError is probably a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jacobian
Django member

Generally LGTM, module a few minor nits left inline. The only other thing I'd add is that this deserves a mention in the 1.6 release notes, we should probably encourage people to switch to Pillow in those notes. Good stuff!

@toastdriven

@jacobian Most changes applied, thanks for the feedback. One question - That big comment at the top was mostly for me to keep it all straight while implementing. Should I remove it? Reword it? Leave it for others who might stumble down this unfortunate trail?

@jacobian
Django member

@toastdriven no, I'd keep that comment; it explains why the important dance works the way it does.

LGTM, merge this puppy.

@toastdriven

@jacobian Consider it merged & thanks for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.