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

Compile LibTIFF with CMake on Windows #5359

Merged
merged 3 commits into from
Apr 1, 2021

Conversation

nulano
Copy link
Contributor

@nulano nulano commented Mar 24, 2021

Alternative to #5243, see the discussion there for more information.

Switches winbuild to compile libtiff using CMake which enables USE_WIN32_FILEIO on all win32 platforms by default.
Compiling with NMake will not be supported when the next version of libtiff is released, see https://gitlab.com/libtiff/libtiff/-/merge_requests/223.

nulano and others added 2 commits March 24, 2021 21:31
(cherry picked from commit 3f17d61)
(cherry picked from commit 6e31662)
@nulano nulano changed the title Compile libtiff with CMake Compile LibTIFF with CMake on Windows Mar 24, 2021
@kmilos
Copy link
Contributor

kmilos commented Mar 25, 2021

Great - I reckon 8.2 is a good time to make the switch and I/O API change as any, cmake has been supported by libtiff for a while already...

# FIXME the following define should be detected automatically
# based on system libtiff, see #4237
if PLATFORM_MINGW:
if sys.platform == "win32":
defs.append(("USE_WIN32_FILEIO", None))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if this PR "switches winbuild to compile libtiff using CMake which enables USE_WIN32_FILEIO on all win32 platforms by default"... why do we need this def?

Copy link
Contributor

@kmilos kmilos Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have to keep Pillow's src/libImaging/TiffDecode.c in sync w/ the way libtiff was built, i.e. because of the whole mess behind #4237 (this fix was intoduced in #4890); maybe better leave the comment for posterity?

Unfortunately libtiff doesn't expose this build time defined macro in a public header like it does for some other (similar) stuff, so one has to set it externally...

Copy link
Contributor

@kmilos kmilos Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively we could do away with this case and define completely, and just use _WIN32 in TiffDecode.c directly if we take a leap of faith and assume all Windows libtiff builds will have Win32 I/O enabled from now on, like e.g. Poppler does since 2014.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LibTIFF also checks this define in tiffio.h when typedefing thandle_t. While all alternatives have the same binary size, it is probably safest to define the macro to make sure. It is also probably simpler to set it in setup.py instead of just TiffDecode.c.

I added a longer explanatory comment to this line:

This define needs to be defined if-and-only-if it was defined
when compiling LibTIFF. LibTIFF doesn't expose it in tiffconf.h,
so we have to guess; by default it is defined in all Windows builds.
See #4237, #5243, #5359 for more information.

@radarhere
Copy link
Member

Compiling with NMake will not be supported when the next version of libtiff is released, it might be a good idea to wait for that release before merging this PR.

The downside of waiting is that when it is released (which could happen at any point in time), we will be failing to support the latest libtiff until our next scheduled release.

@kmilos
Copy link
Contributor

kmilos commented Mar 25, 2021

Btw, I found it interesting that the vcpkg build of libitiff turns off Win32 I/O for UWP only... Maybe it really is not available on the phone, don't know that platform well at all...

All other Windows builds I know (libtiff cmake & autotools default, mingw, vcpkg, anaconda...) have it enabled, with the exception of conda-forge that patched it off because of #4237. If we merge this for all Windows (not just mingw), they will have to remove the patch, or keep patching Pillow as well...

@hugovk hugovk merged commit cafd389 into python-pillow:master Apr 1, 2021
@hugovk
Copy link
Member

hugovk commented Apr 1, 2021

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants