Skip to content
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

Merged
merged 2 commits into from Nov 29, 2019

Conversation

@matthijskooijman
Copy link
Contributor

matthijskooijman commented Nov 17, 2019

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) and PIL.__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:

(venv)matthijs@grubby:~/docs/src/upstream/nml$ pip freeze|grep Pillow
Pillow==6.2.1
(venv)matthijs@grubby:~/docs/src/upstream/nml$ nmlc --version | grep PIL
PIL: 6.2.1


(venv)matthijs@grubby:~/docs/src/upstream/nml$ pip freeze|grep Pillow
Pillow==5.2.0
(venv)matthijs@grubby:~/docs/src/upstream/nml$ nmlc --version | grep PIL
PIL: 5.2.0

(venv)matthijs@grubby:~/docs/src/upstream/nml$ pip freeze|grep Pillow
Pillow==5.0.0
(venv)matthijs@grubby:~/docs/src/upstream/nml$ nmlc --version | grep PIL
PIL: 5.0.0

(venv)matthijs@grubby:~/docs/src/upstream/nml$ pip freeze|grep Pillow
Pillow==3.4.0
(venv)matthijs@grubby:~/docs/src/upstream/nml$ nmlc --version | grep PIL
PIL: 3.4.0

As expected, below 3.4.0, PIL.__version__ is not available:

(venv)matthijs@grubby:~/docs/src/upstream/nml$ pip freeze|grep Pillow
Pillow==3.3.3
(venv)matthijs@grubby:~/docs/src/upstream/nml$ nmlc --version | grep PIL
nmlc ERROR: nmlc: An internal error has occurred:
nmlc-version: 2019-11-09-gabb7a3a7M
Error:    (AttributeError) "module 'PIL' has no attribute '__version__'".
Command:  ['/home/matthijs/docs/src/upstream/nml/venv/bin/nmlc', '--version']
Location: File "/home/matthijs/docs/src/upstream/nml/nml/version_info.py", line 140, in get_lib_versions

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.

@ConiKost
Copy link
Contributor

ConiKost commented Nov 18, 2019

Works for me :-)

import Image
except ImportError:
pass
pass

This comment has been minimized.

Copy link
@LordAro

LordAro Nov 23, 2019

Member

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?

This comment has been minimized.

Copy link
@andythenorth

andythenorth Nov 24, 2019

Contributor

It's probably possible to use NML without PIL for certain limited cases...but it's not useful.

This comment has been minimized.

Copy link
@matthijskooijman

matthijskooijman Nov 29, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@LordAro

LordAro Nov 29, 2019

Member

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 :)

@glx22
Copy link
Contributor

glx22 commented Nov 24, 2019

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 #29, #39 and #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.
@matthijskooijman matthijskooijman force-pushed the matthijskooijman:simplify-pillow-imports branch from f77fb65 to 444868c Nov 29, 2019
@matthijskooijman
Copy link
Contributor Author

matthijskooijman commented Nov 29, 2019

Hm, somehow github did not e-mail me about comments to this PR, so I'm a bit late to reply.

Since checks are enabled, a rebase to master is required to include workflow file.

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

@LordAro LordAro merged commit 2407691 into OpenTTD:master Nov 29, 2019
12 checks passed
12 checks passed
Python 3.5 on ubuntu-latest
Details
Python 3.6 on ubuntu-latest
Details
Python 3.7 on ubuntu-latest
Details
Python 3.8 on ubuntu-latest
Details
Python 3.5 on macOS-latest
Details
Python 3.6 on macOS-latest
Details
Python 3.7 on macOS-latest
Details
Python 3.8 on macOS-latest
Details
Python 3.5 on windows-2016
Details
Python 3.6 on windows-2016
Details
Python 3.7 on windows-2016
Details
Python 3.8 on windows-2016
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.