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

Fix #39: Add compatibility with >=pillow-7.0.0 #54

Merged
merged 1 commit into from Nov 9, 2019

Conversation

@ConiKost
Copy link
Contributor

ConiKost commented Oct 23, 2019

Changed PILLOW_VERSION to PIL.version,
as it got removed with >=pillow-7.0.0.

In order not to break compatibility with older versions,
PILLOW_VERSION will be used as a fallback.

Closes: #39
Signed-off-by: Conrad Kostecki conrad@kostecki.com

@Eddi-z
Copy link
Contributor

Eddi-z commented Oct 23, 2019

Commit message should probably start with "Fix #39: ..."

Also, does it still work with older pillow, or is this a new minimum requirement?

@ConiKost
Copy link
Contributor Author

ConiKost commented Oct 23, 2019

Also, does it still work with older pillow, or is this a new minimum requirement?

At least, with version 6.x is working. I can't test version 5 and older. But If I understand the docs correctly, it should be the case.

@ConiKost ConiKost changed the title Add compatibility with >=pillow-7.0.0 Fix #39: Add compatibility with >=pillow-7.0.0 Oct 23, 2019
nml/version_info.py Outdated Show resolved Hide resolved
nml/version_info.py Outdated Show resolved Hide resolved
Copy link
Contributor

planetmaker left a comment

That patch does not fly as it breaks nml for older versions of PIL / pillow:

nmlc ERROR: nmlc: An internal error has occurred:
nmlc-version: 2019-10-23-g909c09fd
Error: (AttributeError) "module 'PIL.Image' has no attribute 'version'".
Command: ['/home/ingo/bin/nmlc', '--version']
Location: File "/home/ingo/ottd/grfdev/nml/nml/version_info.py", line 142, in get_lib_versions

PIL: 1.1.7
PLY: 3.9

@ConiKost
Copy link
Contributor Author

ConiKost commented Oct 23, 2019

Thanks. But PILLOW_VERSION, as in current master, works for you?

@planetmaker
Copy link
Contributor

planetmaker commented Oct 23, 2019

Thanks. But PILLOW_VERSION, as in current master, works for you?

Yes:
78cd84c+* ± nmlc --version
2019-10-20-g78cd84ce
Library versions encountered:
PLY: 3.9
PIL: 4.0.0
78cd84c+* ± python3 -V
Python 3.5.3

@ConiKost
Copy link
Contributor Author

ConiKost commented Oct 23, 2019

@planetmaker Could you try latest variant?

Changed PILLOW_VERSION to PIL.__version__,
as it got removed with >=pillow-7.0.0.

In order not to break compatibility with older versions,
PILLOW_VERSION will be used as a fallback.

Closes: #39
Signed-off-by: Conrad Kostecki <conrad@kostecki.com>
@planetmaker
Copy link
Contributor

planetmaker commented Oct 23, 2019

@planetmaker Could you try latest variant?

It's PILLOW_VERSION (you are missing one L). That fixed it works for me.

@ConiKost
Copy link
Contributor Author

ConiKost commented Oct 23, 2019

It's PILLOW_VERSION (you are missing one L). That fixed it works for me.

already fixed ;-)

@matthijskooijman
Copy link
Contributor

matthijskooijman commented Oct 29, 2019

That patch does not fly as it breaks nml for older versions of PIL / pillow:
PIL: 1.1.7

But the README says the minimum supported pillow version is 5.2, so why is this relevant? It will (I expect) then just break elsewhere? Or are older versions actually supported after all?

@ConiKost
Copy link
Contributor Author

ConiKost commented Oct 29, 2019

PIL =! Pillow. Version 1.1.7 is for historic reasons.

Pillow 5.2.0 also reports 1.1.7 with PIL.VERSION.
-> https://pillow.readthedocs.io/en/stable/releasenotes/5.2.0.html

@matthijskooijman
Copy link
Contributor

matthijskooijman commented Nov 5, 2019

PIL =! Pillow. Version 1.1.7 is for historic reasons.

Ah, of course, I misread the comment then. @planetmaker, what pillow version were you using when you posted:

Error: (AttributeError) "module 'PIL.Image' has no attribute 'version'".

Looking more closely, it seems this PR now uses PIL.Image.__version (which is apparently not available in the version the @planetmaker tested). However, IIUC, the docs recommend using PIL.__version__ instead, which should be available since version 3.4 (see also #39 (comment))

@LordAro LordAro merged commit 70c0b31 into OpenTTD:master Nov 9, 2019
@matthijskooijman
Copy link
Contributor

matthijskooijman commented Nov 9, 2019

@LordAro I'm not actually sure that the code is now checking the right (or at least recommended) version number, see my previous comment. @ConiKost, what do you think?

@ConiKost
Copy link
Contributor Author

ConiKost commented Nov 9, 2019

@matthijskooijman I guess, it depends on the specific version, which he has.

But currently, as far I understand the code, both, PIL.Image.__version__ and PIL.__version__ are tried, before doing a fallback. So this should be fine, as it is?

@matthijskooijman
Copy link
Contributor

matthijskooijman commented Nov 10, 2019

AFAICS, PIL.__version__ is never tried, only PIL.Image.__version__ and Image.__version__.

nml/nml/version_info.py

Lines 140 to 154 in abb7a3a

try:
from PIL import Image
try:
versions["PIL"] = Image.__version__
except AttributeError:
versions["PIL"] = Image.PILLOW_VERSION
except ImportError:
try:
import Image
try:
versions["PIL"] = Image.__version__
except AttributeError:
versions["PIL"] = Image.PILLOW_VERSION
except ImportError:
versions["PIL"] = "Not found!"

Looking at the code gain, I also wonder if the fallback to import Image is still needed? I haven't looked in detail, but before I found that PIL.__version__ was introduced in pillow 3.4.0 (python-pillow/Pillow@7bfb252) and it seems that PIL.Image was also available already in that version (and the minimium pillow version for nml is now 5.something, right?).

@ConiKost
Copy link
Contributor Author

ConiKost commented Nov 11, 2019

FWICS, you seem to be right, that import Image could not be needed anymore.
About version 5, I wasn't the one, who said said ;-) I only was asked, to add 5.2 as a minimum version, so I did. I don't know the code, so I can't judge on that.

matthijskooijman added a commit to matthijskooijman/nml that referenced this pull request Nov 17, 2019
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.
@matthijskooijman
Copy link
Contributor

matthijskooijman commented Nov 17, 2019

Thanks. I've opened PR #59 further simplifying the Pillow imports. As for the minimum version, I've opened issue #58 for that.

matthijskooijman added a commit to matthijskooijman/nml that referenced this pull request Nov 29, 2019
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.
LordAro added a commit that referenced this pull request Nov 29, 2019
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.
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.

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