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

Problem with using ImageTk on Cygwin #5795

Closed
friedrichromstedt opened this issue Oct 25, 2021 · 19 comments · Fixed by #5807
Closed

Problem with using ImageTk on Cygwin #5795

friedrichromstedt opened this issue Oct 25, 2021 · 19 comments · Fixed by #5807
Labels

Comments

@friedrichromstedt
Copy link

While attempting to use ImageTk on Cygwin (64-bit, Windows 10, Python 3.8), I ran into an unhandled Exception. Just attempting to show some black square Pillow Image using a tkinter Canvas causes this failure. I've tried several versions of Pillow, from 8.4.0 down to 5.4.1, all with the same result.

Further description of the dysfunction including a minimal example script can be found at https://github.com/friedrichromstedt/bughunting-02. This repo provides a summary along with detailed information on my work regarding the subject.

I've reported to Cygwin upstream https://sourceware.org/pipermail/cygwin/2021-October/249643.html, but up to now this did not lead to a solution. It might be that the problem is rooted somewhere outside of Pillow, but I would appreciate any pointer.

@radarhere
Copy link
Member

Hi. Let me ask this question - does tkinter work fine by itself?

import tkinter
root = tkinter.Tk()
root.mainloop()

@friedrichromstedt
Copy link
Author

Yes, the following narrowed-down script works well:

import tkinter
from PIL import Image, ImageTk


image = Image.new('RGB', (100, 100))

tk = tkinter.Tk()
canvas = tkinter.Canvas(tk)
canvas.pack()

#photoimage = ImageTk.PhotoImage(image)
#canvas.create_image((0, 0),
#    image=photoimage,
#    anchor='nw',
#)

tk.mainloop()

This script is based on the reproduce.py file in the repo I mentioned.

It is the line photoimage = ImageTk.PhotoImage(image) which triggers the error; commenting out only the call of canvas.create_image does not suffice to make the script succeed.

@DWesl
Copy link
Contributor

DWesl commented Oct 25, 2021

One of the guidelines for reporting bugs on Cygwin is this:

  • Run cygcheck -s -v -r > cygcheck.out and include that file as an attachment in your report. Please do not compress or otherwise encode the output. Just attach it as a straight text file so that it can be easily viewed.
    Note: It is ok to redact sensitive information like username and to remove site-specific environment variables but please note that fact in your email message so that we'll know that the cygcheck output has been modified.

Have you done that, and does anything in there look like it could pose a problem?

From your traceback in the reproducer repository, it looks like the problem is with the line from . import _imagingtk inside a module in the PIL top-level package. What happens if you try to run import PIL._imagingtk in python?

If python -c 'import PIL._imagingtk' fails with a similarly unhelpful error message, it may be helpful to increase the verbosity to see what file it's trying to open when it fails: python -v -c 'import PIL.imagingtk', then python -vv -c 'import PIL._imagingtk' and so on, until it tells you the actual file causing problems. Is that file where you expect it to be?
What are the permissions on that file: in particular, do you have execute permissions? Does cygcheck /path/to/site-packages/PIL/_imagingtk* report any errors (ldd /path/to/site-packages/PIL/_imagingtk* should provide the same information in a different format)?

@friedrichromstedt
Copy link
Author

OK, the diagnostic information you asked for is provided in https://github.com/friedrichromstedt/bughunting-02/tree/v2021-10-26_1#diagnostic-information-26-october-2021. Notice that I am referring to the tagged version to ensure permanent links.

The output of cygcheck -svr looks okay as far as I can tell, as does the result from cygcheck-ing and ldd-ing the extension module DLLs.

Importing PIL._imagingtk fails. It produces the RuntimeError: No such process exception (cf. the traceback in https://github.com/friedrichromstedt/bughunting-02/tree/v2021-10-26_1#description-of-the-bug-and-of-the-expected-behaviour). The _tkinter.TclError: invalid command name "PyImagingPhoto" exception seems to be caused by the photoimage = ImageTk.PhotoImage(image) line in the reproduction script. It is unclear to me how the from . import _imagingtk instruction results from handling the initial TclError.

@radarhere
Copy link
Member

Interesting. I'm guessing that from PIL import _tkinter_finder also fails.

Does

import tkinter
tkinter.__file__

fail?

@friedrichromstedt
Copy link
Author

friedrichromstedt commented Oct 26, 2021

None of these fails.

See:

$ python
Python 3.8.10 (default, May 20 2021, 11:41:59)
[GCC 10.2.0] on cygwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from PIL import _tkinter_finder
>>>

and

$ python
Python 3.8.10 (default, May 20 2021, 11:41:59)
[GCC 10.2.0] on cygwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import tkinter
>>> tkinter.__file__
'/usr/lib/python3.8/tkinter/__init__.py'

The two modules/packages can also be imported in the same python session in any order.

@friedrichromstedt
Copy link
Author

Some more observations:

So maybe Pillow is not the actual culprit. I've also downgraded Tcl/Tk from recent 8.6.11 to 8.6.8 in the abovementioned section, to no avail. However, since Pillow is the piece of code which yields the exception, I have the hope that some helpful conclusion can be drawn here.

@friedrichromstedt
Copy link
Author

Any further ideas how to track down this issue?

@DWesl
Copy link
Contributor

DWesl commented Nov 2, 2021

Pillow/src/_imagingtk.c

Lines 54 to 55 in a215f72

/* This will bomb if interp is invalid... */
TkImaging_Init(interp);

and

Pillow/src/Tk/tkImaging.c

Lines 380 to 397 in 95cff6e

void *
_dfunc(void *lib_handle, const char *func_name) {
/*
* Load function `func_name` from `lib_handle`.
* Set Python exception if we can't find `func_name` in `lib_handle`.
* Returns function pointer or NULL if not present.
*/
void *func;
/* Reset errors. */
dlerror();
func = dlsym(lib_handle, func_name);
if (func == NULL) {
const char *error = dlerror();
PyErr_SetString(PyExc_RuntimeError, error);
}
return func;
}

look to be the only places that could fail on import.

import tkinter
tk = tkinter.Tk()
canvas = tkinter.Canvas(tk)
canvas.pack()
photoimage = tkinter.PhotoImage(file="/path/to/image.png")
canvas.create_image((0, 0), image=photoimage, anchor='nw')
tk.mainloop()

works fine.

I'm not sure how to check further what's going on beyond this.

An interesting comment —

Pillow/src/Tk/tkImaging.c

Lines 224 to 230 in 95cff6e

/*
* On Windows, we can't load the tkinter module to get the Tcl or Tk symbols,
* because Windows does not load symbols into the library name-space of
* importing modules. So, knowing that tkinter has already been imported by
* Python, we scan all modules in the running process for the Tcl and Tk
* function names.
*/

— suggests that this is one of the times that "Cygwin uses Windows linking semantics" is relevant: I suspect Pillow is currently trying to import symbols defined in the Tcl/Tk shared libraries from Python's _tkinter shared library. It would be interesting to have the Cygwin build of Pillow use the Windows code path and see if that fixes the issue.

@nulano
Copy link
Contributor

nulano commented Nov 2, 2021

An interesting comment —

Note that this is in an #if defined(_WIN32) || ... block, so this is used on Cygwin (according to this, Cygwin defines _WIN32).

Edit: I didn't read the page closely enough: the macro may be defined based on the selected compile configuration.

@DWesl
Copy link
Contributor

DWesl commented Nov 3, 2021

Note that this is in an #if defined(_WIN32) || ... block, so this is used on Cygwin (according to this, Cygwin defines _WIN32).

Apparently Cygwin does sometimes define _WIN32, but that has to be specifically requested:

$ echo | gcc -x c - -E -dM | grep -i WIN
#define __WINT_MAX__ 0xffffffffU
#define __WINT_MIN__ 0U
#define __SIZEOF_WINT_T__ 4
#define __CYGWIN__ 1
#define __WINT_TYPE__ unsigned int
#define __WINT_WIDTH__ 32

I don't think that flag got passed in any of the Pillow builds mentioned in the reproduction repository.

@DWesl
Copy link
Contributor

DWesl commented Nov 3, 2021

@friedrichromstedt, do the changes suggested in #5807 fix the problem for you?

@friedrichromstedt
Copy link
Author

friedrichromstedt commented Nov 5, 2021

The branch https://github.com/DWesl/Pillow/tree/tkimaging-on-cygwin (29b9239) referenced in #5807 solved the problem on my machine.

Detailed information about my proceeding is available on https://github.com/friedrichromstedt/bughunting-02/tree/v2021-11-05_2#proposal-of-a-fix-from-upstream.

Thanks a lot for all the work which has been spent solving this issue.

I noticed that the version of the branch I used (29b9239) is one commit behind the current https://github.com/DWesl/Pillow/tree/tkimaging-on-cygwin. Shall I test once more?

@DWesl
Copy link
Contributor

DWesl commented Nov 5, 2021

The latest commit should just skip a test that won't work on Cygwin. Is whatever code inspired your test case working now?

@friedrichromstedt
Copy link
Author

The latest commit should just skip a test that won't work on Cygwin.

I noticed that even with DWesl@29b9239 the selftests pass. You can find the logs in https://github.com/friedrichromstedt/bughunting-02/blob/v2021-11-05_2/Logs2/b01%20make%20install.txt, context in https://github.com/friedrichromstedt/bughunting-02/tree/v2021-11-05_2#proposal-of-a-fix-from-upstream.

Is whatever code inspired your test case working now?

Yes! I tested carefully, so it needed some time. Thanks again!

@DWesl
Copy link
Contributor

DWesl commented Nov 8, 2021

I am told that at some point the recommended way to run Pillow's tests changed from python selftest.py to python -m pytest Tests. tox does both; the documentation doesn't seem to mention either.

@hugovk
Copy link
Member

hugovk commented Nov 9, 2021

Yes, selftest.py has always* been a quick subset, the comment at the top is "minimal sanity check". Use pytest for the full test suite.

There's some more info in https://github.com/python-pillow/Pillow/blob/main/Tests/README.rst


* selftest.py was added in PIL 1.1.2 in 2001, the main (custom framework) test suite was added in 2013, converted to unittest ran with nose in 2014, switched to pytest as a runner in 2017, and converted tests to pytest in 2020.

@friedrichromstedt
Copy link
Author

From what I see on #5807 you already tested diligently. From your comment on 5 Nov 2021 I gather that you also did the X-related tests? Should I test once more on my machine? I am starting the X server through a Cygwin-supplied shortcut file (which is invoking <P:\ath\to\cygwin64>\bin\run.exe --quote /usr/bin/bash.exe -l -c "cd; exec /usr/bin/startxwin").

@DWesl
Copy link
Contributor

DWesl commented Nov 11, 2021

Probably once through the Pillow test suite is enough. I also use startxwin to start my X server.

Is whatever code inspired your test case working now?

Yes! I tested carefully, so it needed some time. Thanks again!

Thank you for testing.

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

Successfully merging a pull request may close this issue.

5 participants