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
Use the Windows method to get TCL functions on Cygwin #5807
Changes from all commits
c8391aa
b542da8
29b9239
5a41417
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -31,11 +31,18 @@ | |||
#endif | ||||
#endif | ||||
|
||||
#ifdef _WIN32 | ||||
#if defined(_WIN32) || defined(__CYGWIN__) | ||||
|
||||
#define WIN32_LEAN_AND_MEAN | ||||
#include <Windows.h> | ||||
|
||||
#ifdef __CYGWIN__ | ||||
#undef _WIN64 | ||||
#undef _WIN32 | ||||
#undef __WIN32__ | ||||
#undef WIN32 | ||||
#endif | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder whether it might be better to keep these defined to fix other potential issues? I'm pretty sure a similar issue might happen around the FriBiDi shim (although it is disabled by default). There are also a few functions that are only enabled on Windows platforms, and I don't think there's anything stopping them working on Cygwin. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Running it in the root of the modified sdist shows only one error, a PermissionError when attempting to overwrite a test image in a temporary directory. I haven't figured out how to get that passing, but it seems unrelated to this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Which test is this? But yes, probably unrelated.
I'm also interested in the skips and xfails, but I can take a look at it myself now, as I had a bit of free time just now and I got Cygwin working on GHA (surprisingly easy compared to some other platforms): https://github.com/nulano/Pillow/runs/4105517818?check_suite_focus=true There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, that one. I have a reproducer (create a file named "a.txt" before running): >>> import mmap
>>> with open("a.txt", "r") as in_file:
... mapped = mmap.mmap(in_file.fileno(), 0, access=mmap.ACCESS_READ)
... with open("a.txt", "w") as out_file:
... out_file.write(line)
...
Traceback (most recent call last):
File "<stdin>", line 3, in <module>
PermissionError: [Errno 13] Permission denied: 'a.txt'
Skipped and expected failing tests as of my most recent run:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm guessing you ran this on a system without an X11 desktop environment (I'm not sure how this works on Cygwin...)? This means that TCL was not actually tested at all. If this test passed (and failed without this PR) instead I would say this probably fixes #5795, but with a skip it is unclear. Pillow/.github/workflows/test.yml Line 85 in fcb87ec
I would be interested in whether the test_imagegrab and test_imagewin "Windows only" skips can be made to pass (I'm not sure to what extent Cygwin allows access to WinAPI), but perhaps that is beyond the scope of this PR. There is also an The FriBiDi code that I mentioned in my first comment has to be specifically requested at compilation and is intended for published wheels, so perhaps it is not very important, but if you really feel like checking it, see #5062. However, I certainly intend to test it before submitting the PR to add a Cygwin run on GHA, probably some time before next release (I don't know how much time I will have in the near future). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was through tox. I'm not sure what all I need to do to convince X to work through tox, besides passing the There are I did check that the changes in this PR allowed |
||||
|
||||
#else | ||||
/* For System that are not Windows, we'll need to define these. */ | ||||
|
||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cygwin can't use Python's mmap? Could you provide some more information about this, or link to a discussion about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial comment was based on the above test case failing. In a fresh python session, the below case works fine:
so it looks like having the mmap open while opening the file for writing is a problem, but opening the mmap seems to be fine. Since the test case is nearly this exact sequence of operations, I marked it as XFAIL.
I get a different error using Windows-native python from anaconda:
I have no idea why that error is happening; I have gigabytes of memory free and requested 30 bytes.