Enable libtiff Win32 I/O correctly

[kmilos]

Fixes #4237 (for Pillow static builds and wheels)

NB: Requires all downstream projects to synchronize and rebuild.

This is a follow-up to #4890

Changes proposed in this pull request:

• Default to USE_WIN32_FILEIO for internal NMake builds (now in sync w/ Cmake and autotools; see upstream report comment)
• Append USE_WIN32_FILEIO for all Windows builds, not just MinGW

[kmilos]

I presume this fails because libtiff itself is not built as part of CI?

[hugovk]

Built for Windows:

Pillow/.github/workflows/test-windows.yml

Lines 105 to 107 in 31f3f8f

	- name: Build dependencies / LibTiff
	if: steps.build-cache.outputs.cache-hit != 'true'
	run: "& winbuild\\build\\build_dep_libtiff.cmd"

Except installed by pacman for Windows / MSYS MinGW:

Pillow/.github/workflows/test-windows.yml

Line 259 in 31f3f8f

${{ matrix.package }}-libtiff \

[nulano]

LibTIFF was not rebuilt due to the build cache - the tiff.opt file is not part of the cache key, as I assumed any changes there would be in parallel with updating build_prepare.py (changing libtiff versions) or at least .github/workflows/test-windows.yml (e.g. changing the VS version). If you update either of those files the CI should re-run.

xref actions/cache#2

[nulano]

For reference, looks like this is related to conda-forge/poppler-feedstock#73 (comment), conda-forge/libtiff-feedstock#62.

[kmilos]

Correct, trying to figure out what is a consistent way of building libtiff and Pillow on Windows across the board...

OTOH, @cgohlke imagecodecs seems to assume Unix I/O always (ctypedef void* thandle_t), so this PR might actually hurt? (This should already be a problem on MinGW...)

[nulano]

OTOH, @cgohlke imagecodecs seems to assume Unix I/O always (ctypedef void* thandle_t), so this PR might actually hurt? (This should already be a problem on MinGW...)

Windows guarantees sizeof(HFILE)==sizeof(DECLARE_HANDLE(...))==sizeof(void*) so that by itself is not a problem. The problematic function is actually TIFFFdOpen which takes an int with unix I/O, but HFILE with win32 I/O. However, using this function requires obtaining a raw file handle in Python code, so it might not actually be commonly used. I would expect TIFFOpen, which just takes a filename, is used a lot more often.

[kmilos]

AFAICT we don't have HFILE in this case but the DECLARE_HANDLE(thandle_t) macro.

imagecodecs indeed actually uses only TIFFClientOpen so I guess that is safe?

[nulano]

AFAICT we don't have HFILE in this case but the DECLARE_HANDLE(thandle_t) macro.

Oh right, but HFILE is defined as DECLARE_HANDLE(HFILE) in the WinAPI headers, so that is also the same size as all other HANDLEs on Windows (although only the bottom 32-bits are in use for compatibility with WoW).

imagecodecs indeed actually uses only TIFFClientOpen so I guess that is safe?

TIFFFdOpen is implemented by calling TIFFClientOpen with a set of windows- or unix-specific callbacks; imagecodecs provides its own callbacks so it is safe to use it with either version.

The imagecodecs.libtiff.TIFFFdOpen function is not in a private module AFAICT, but this is just passing the issue one level deeper and will most-likely require patching a subset of libraries that use it depending on which version of I/O they assume is in use, if there are any such libraries. (the change is #4237 (comment))

[kmilos]

TIFFFdOpen is implemented by calling TIFFClientOpen with a set of windows- or unix-specific callbacks; imagecodecs provides its own callbacks so it is safe to use it with either version.

Got it, thanks.

Back to the CI cache issue - still don't have any clues how to kickstart it and what to change in the yml files, so maintainers please feel free to modify the PR.

[hugovk]

Because https://github.com/actions/cache doesn't yet have a way to clear caches, one common workaround is to include a -v1- version number in the cache key, which can be easily changed: actions/cache#2

For example:

https://github.com/hugovk/pypistats/blob/5978cf0f935053cefbd4e84736dfb0438bbe0714/.github/workflows/test.yml#L34-L42

[nulano]

It looks like the libtiff maintainers are soon removing have now removed NMake support entirely: https://gitlab.com/libtiff/libtiff/-/merge_requests/223

I have an old work-in-progress branch where I changed the build to use CMake, but didn't proceed due to #4237. Would it be better to switch to always use win32 io or to include the patch from conda in the CMake build?

[kmilos]

Thanks for catching and sharing that.

I'm converting this PR to draft for now, as I guess it'll take some time to converge to the "best" approach.

Here's what I gathered so far:

• MINGW uses Autotools to build libtiff for ages and Win32 I/O has been a default there like it is in Cmake now, and you @nulano have already patched Pillow to support this^
  • If MINGW ever switches to Cmake, there is no impact on Pillow as things stand
• vcpkg ports use Cmake and Win32 I/O unless the build is for UWP (Microsoft Store)
• conda-forge used Cmake for a while, and the Win32 I/O default was only introduced in 4.1 and broke compatibility
  • They have opted to disable it and keep 4.0.x Unix I/O to keep Pillow happy at the time
  • As a consequence poppler also had to be patched, so now they have to maintain at least two patches
  • I guess somebody needs to ping the core team on how they want to proceed
• So far I only know of Pillow and poppler using TIFFFdOpen directly. poppler assumes Win32 I/O with any Windows build sice 2014

IMHO all Windows libtiff builds should use the default Win32 I/O, which would mean some maintenance work for conda-forge to switch or keep patching. It'd be nice to establish if there are more packages affected other than Pillow and poppler...

Ideally, libtiff should have exposed the USE_WIN32_FILEIO define publicly in tiffconf.h instead of only in the private tif_config.h, so 3rd parties can use this directly and we wouldn't have to worry about maintaining exceptions. I think I'll add this to the bug report over there.

^ Rather than hard-coding this in setup.py, is there a way to pass -DUSE_WIN32_FILEIO externally to setup.py build so any Pillow packager can easily choose without patching? I see passing defines is supported for setup.py build_ext command, but don't know setuptools well enough to know if this applies to the build command as well, I haven't seen it in its options list...

[kmilos]

NMake removal is now merged in libtiff master, so it sounds like Pillow will likely make a switch to CMake anyway, no?

Feel free to hijack this PR for it, or close/reject until such a time comes...

[radarhere]

I've added an unrelated nasm update, and the change in test-windows.yml updates the cache key, causing the Windows tests to pass.

[kmilos]

Thanks, I'll remove the draft status should you want to take this for the next release.

I guess the cmake switch won't be necessary until the next libtiff version is released, which is usually a while from now...

[nulano]

I have an old work-in-progress branch where I changed the build to use CMake,

Rebased in #5359.

[hugovk]

#5359 merged instead. Thanks!