FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Use the Windows method to get TCL functions on Cygwin by DWesl · Pull Request #5807 · python-pillow/Pillow · GitHub

/ Pillow Public

Use the Windows method to get TCL functions on Cygwin #5807

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.

Sign up for GitHub

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

Merged
radarhere merged 4 commits into from
Dec 29, 2021
Conversation 12 Commits 4 Checks 0 Files changed
Merged

Use the Windows method to get TCL functions on Cygwin #5807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8391aa
Use the Windows functions to get TCL functions on Cygwin.
DWesl Nov 3, 2021
b542da8
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 3, 2021
29b9239
Suggestion: use 'var in tuple' instead of chained comparisons.
DWesl Nov 4, 2021
5a41417
Skip test_readonly_save on Cygwin.
DWesl Nov 4, 2021
File filter

Filter by extension

Filter by extension .c  (1) .h  (1) .py  (2) All 3 file types selected
Only manifest files
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
4 changes: 4 additions & 0 deletions Tests/test_image.py
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,10 @@ def test_internals(self):
assert not im.readonly

@pytest.mark.skipif(is_win32(), reason="Test requires opening tempfile twice")
@pytest.mark.skipif(
sys.platform == "cygwin",
reason="Test requires opening an mmaped file for writing",
)

radarhere Dec 28, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Cygwin can't use Python's mmap? Could you provide some more information about this, or link to a discussion about it?

DWesl Dec 28, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality 
$ echo "Lorem itsum dolor sit amet" > a.txt
$ python
>>> import mmap
>>> with open("a.txt", "r") as in_file:
...     with mmap.mmap(in_file.fileno(), 30, access=mmap.ACCESS_COPY) as mapped:
...         with open("a.txt", "w") as out_file:
...             out_file.write(mapped)
...
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
PermissionError: [Errno 13] Permission denied: 'a.txt'

My initial comment was based on the above test case failing. In a fresh python session, the below case works fine:

>>> with open("a.txt", "r") as in_file:
...     with open("a.txt", "w") as out_file:
...         out_file.write(in_file.read())
...
0

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:

>>> import mmap
>>> with open("a.txt", "r") as in_file:
...     with mmap.mmap(in_file.fileno(), 30, access=mmap.ACCESS_COPY) as mapped:
...         with open("a.txt", "w") as out_file:
...             out_file.write(mapped)
...
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
OSError: [WinError 8] Not enough memory resources are available to process this command

I have no idea why that error is happening; I have gigabytes of memory free and requested 30 bytes.
def test_readonly_save(self, tmp_path):
temp_file = str(tmp_path / "temp.bmp")
shutil.copy("Tests/images/rgb32bf-rgba.bmp", temp_file)
Expand Down
2 changes: 1 addition & 1 deletion setup.py
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -887,7 +887,7 @@ def build_extensions(self):
else:
self._remove_extension("PIL._webp")

tk_libs = ["psapi"] if sys.platform == "win32" else []
tk_libs = ["psapi"] if sys.platform in ("win32", "cygwin") else []
self._update_extension("PIL._imagingtk", tk_libs)

build_ext.build_extensions(self)
Expand Down
2 changes: 1 addition & 1 deletion src/Tk/tkImaging.c
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ TkImaging_Init(Tcl_Interp *interp) {

#define TKINTER_FINDER "PIL._tkinter_finder"

#if defined(_WIN32) || defined(__WIN32__) || defined(WIN32)
#if defined(_WIN32) || defined(__WIN32__) || defined(WIN32) || defined(__CYGWIN__)

/*
* On Windows, we can't load the tkinter module to get the Tcl or Tk symbols,
Expand Down
9 changes: 8 additions & 1 deletion src/libImaging/ImPlatform.h
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -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

nulano Nov 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I wonder whether it might be better to keep these defined to fix other potential issues?
It would be helpful to get the output of a full test run on Cygwin, or even adding to the CI (I don't have the time to try this at the moment).

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.

DWesl Nov 4, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

python selftest.py reports no errors before or after this change, if that's what you're asking.

nulano Nov 4, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I mean python3 -m pytest -v Tests in the root of the git checkout.

DWesl Nov 4, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

nulano Nov 4, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

a PermissionError when attempting to overwrite a test image in a temporary directory

Which test is this? But yes, probably unrelated.
Edit: Looks like it might be Tests/test_image.py::TestImage::test_readonly_save

shows only one error

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 are some failing tests (I haven't looked into them yet), but all dependencies seem to be detected correctly at least.

DWesl Nov 4, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

a PermissionError when attempting to overwrite a test image in a temporary directory

Which test is this? But yes, probably unrelated. Edit: Looks like it might be Tests/test_image.py::TestImage::test_readonly_save

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'

shows only one error

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 are some failing tests (I haven't looked into them yet), but all dependencies seem to be detected correctly at least.

Skipped and expected failing tests as of my most recent run:

====================== short test summary info =======================
SKIPPED [1] Tests/test_file_jpeg.py:877: Windows only
SKIPPED [1] Tests/test_file_tiff.py:732: Windows only
SKIPPED [1] Tests/test_image.py:196: Test requires opening an mmapped file for writing
SKIPPED [1] Tests/test_image_access.py:398: Failing on AppVeyor / GitHub Actions when run from subprocess, not from shell
SKIPPED [1] Tests/test_image_fromqimage.py:38: Qt bindings are not installed
SKIPPED [1] Tests/test_image_fromqimage.py:43: Qt bindings are not installed
SKIPPED [1] Tests/test_image_fromqimage.py:48: Qt bindings are not installed
SKIPPED [1] Tests/test_image_fromqimage.py:53: Qt bindings are not installed
SKIPPED [1] Tests/test_image_fromqimage.py:58: Qt bindings are not installed
SKIPPED [1] Tests/test_imagedraw.py:974: failing
SKIPPED [2] Tests/test_imagefontctl.py:233: fails with this font
SKIPPED [1] Tests/test_imagegrab.py:13: requires Windows or macOS
SKIPPED [1] Tests/test_imagegrab.py:37: X connection failed: error 1
SKIPPED [1] Tests/test_imagegrab.py:39: tests missing XCB
SKIPPED [1] Tests/test_imagegrab.py:77: Windows only
SKIPPED [1] Tests/test_imagegrab.py:87: Windows only
SKIPPED [1] Tests/test_imageqt.py:15: Qt bindings are not installed
SKIPPED [1] Tests/test_imageqt.py:44: Qt bindings are not installed
SKIPPED [1] Tests/test_imageqt.py:49: Qt bindings are not installed
SKIPPED [1] Tests/test_imageshow.py:44: Only run on CIs; hangs on Windows CIs
SKIPPED [4] Tests/test_imagetk.py:30: TCL Error: couldn't connect to display ":0.0"
SKIPPED [1] Tests/test_imagewin.py:37: Windows only
SKIPPED [1] Tests/test_imagewin.py:47: Windows only
SKIPPED [1] Tests/test_imagewin.py:58: Windows only
SKIPPED [1] Tests/test_imagewin.py:72: Windows only
SKIPPED [1] Tests/test_imagewin.py:87: Windows only
SKIPPED [1] Tests/test_qt_image_qapplication.py:50: Qt bindings are not installed
SKIPPED [1] Tests/test_qt_image_toqimage.py:15: Qt bindings are not installed
XFAIL Tests/test_decompression_bomb.py::TestDecompressionBomb::test_exception_ico
  different exception
XFAIL Tests/test_file_palm.py::test_p_mode
  Palm P image is wrong
XFAIL Tests/test_image_resample.py::TestCoreResampleAlphaCorrect::test_levels_rgba
  Current implementation isn't precise enough
XFAIL Tests/test_image_resample.py::TestCoreResampleAlphaCorrect::test_levels_la
  Current implementation isn't precise enough
========== 2867 passed, 32 skipped, 4 xfailed, 4 warnings in 231.29s (0:03:51) ==========

nulano Nov 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

TCL Error: couldn't connect to display ":0.0"

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.
It may be possible to test this using xvfb-run as is done on some Ubuntu CIs (and what I plan to add to the Cygwin job)

Pillow/.github/workflows/test.yml

Line 85 in fcb87ec

xvfb-run -s '-screen 0 1024x768x24' .ci/test.sh
or with a Windows X11 server such as Xming.

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 #ifdef _WIN32 in _imagingcms.c which is also related to the Windows display, but I guess it doesn't have a test or it would show up in the above.

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).

DWesl Nov 5, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That was through tox. I'm not sure what all I need to do to convince X to work through tox, besides passing the DISPLAY variable through. Running pytest myself (not through tox) with PYTHONPATH set to include the changed version of Pillow runs those tests, producing 27 skips and four expected failures.

There are xorg-server-common and xinit packages on Cygwin, which I use on a regular basis locally and through ssh forwarding. xvfb-run seems to be in xorg-server-extra (cygcheck -p xcfb-run).

I did check that the changes in this PR allowed import PIL._imagingtk to work, and the person who posted the original issue said this change fixed their STC as well as the bug they narrowed down to that STC.

#else
/* For System that are not Windows, we'll need to define these. */

Expand Down
Back | FazBrowse Home | New Git URL