Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use the Windows method to get TCL functions on Cygwin #5807
Changes from 2 commits
c8391aa
b542da8
29b9239
5a41417
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
python selftest.py
reports no errors before or after this change, if that's what you're asking.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.
I mean
python3 -m pytest -v Tests
in the root of the git checkout.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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Which test is this? But yes, probably unrelated.
Edit: Looks like it might be
Tests/test_image.py::TestImage::test_readonly_save
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.
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.
Yes, that one. I have a reproducer (create a file named "a.txt" before running):
Skipped and expected failing tests as of my most recent run:
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.
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
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).
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.
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) withPYTHONPATH
set to include the changed version of Pillow runs those tests, producing 27 skips and four expected failures.There are
xorg-server-common
andxinit
packages on Cygwin, which I use on a regular basis locally and through ssh forwarding.xvfb-run
seems to be inxorg-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.