New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify pillow imports and version detection #59
Simplify pillow imports and version detection #59
Conversation
Works for me :-) |
import Image | ||
except ImportError: | ||
pass | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do these ImportError blocks remain? Is it actually possible to use NML without PIL? Why has the import in real_sprite.py not got an try/except block around it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably possible to use NML without PIL for certain limited cases...but it's not useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the commit message:
Note that in some places, an ImportError for Pillow was and still is
ignored, since Pillow is not required for all codepaths. See commit
5612c5b where this was introduced.
And from that commit's message:
This allows writing townname grfs without having PIL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be prepared to bet money that doesn't work (especially now that Pillow is listed as a requirement in setup.py), but sure, ok :)
Since checks are enabled, a rebase to master is required to include workflow file. |
Previously, this tried various combinations of version variables. However, some of these are deprecated by Pillow and the 6.0.0 release notes recommend simply using `PIL.__version` instead. This attribute is available since Pillow 3.4.0, so there is no need for any compatibility fallbacks. This relates to OpenTTD#29, OpenTTD#39 and OpenTTD#54.
Previously, this tried a fallback to `import Image`, but Pillow has stopped supporting that in version 1.0, so there is no need to keep carrying that around. Note that in some places, an ImportError for Pillow was and still is ignored, since Pillow is not required for all codepaths. See commit 5612c5b where this was introduced.
f77fb65
to
444868c
Compare
Hm, somehow github did not e-mail me about comments to this PR, so I'm a bit late to reply.
I've just rebased and force pushed and all checks were succesful. I would be happy if this could be merged, then I can backport to the Debian package and prevent that from being removed from Debian/testing in a few days due to build failures :-p |
This simplifies the importing and version detection for Pillow by using the imports recommended by Pillow exclusively without fallbacks:
PIL.Image
(supported since before Pillow 1.0) andPIL.__version__
(supported since Pillow 3.4).Changes to the version detection were made in #29 and #54 before, but additional discussion in #54 suggests that the changes are not perfectly aligned with Pillow recommendations yet.
As expectd, the version detection seems to work fine for all versions down to Pillow 3.4.0:
As expected, below 3.4.0,
PIL.__version__
is not available:I've tested the regular imports (i.e. not the version detection, but normal operation) against Pillow 3.4.0 and 6.2.1 using
make -C regression
and by compiling OpenGFX 0.5.5.