-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Do not always require ctypes in tests #6141
Conversation
Is it possible to run our test suite without setuptools? Isn't setuptools needed to build/install Pillow for it to be testable? |
Ok, I've taken out the setuptools change. |
@@ -404,6 +403,8 @@ class TestEmbeddable: | |||
"not from shell", | |||
) | |||
def test_embeddable(self): | |||
import ctypes |
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.
Any reason to move this here (and in the other file)? Is it sometimes slow to import?
PEP 8 says to put them at the top, but I think it's fine to move slow imports to where they're needed, if they're not always 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.
ctypes is an optional module, so it may not be supported by the user's Python installation. If they are on macOS or Linux, we shouldn't block them from running our test suite because a dependency for Windows tests are missing.
If that sounds obscure, my personal motivation for this is that when trying to add musllinux to pillow-wheels, I found that ctypes was failing. After my attempt to fix ctypes failed, I thought I'd try and remove ctypes instead, since it wasn't really needed for that build.
I also discovered I'm not the only one who has thought about optionally importing ctypes. Most of the time, setuptools only imports ctypes where it is needed.
If you think this isn't a generally helpful thing though, we can close it and I can go back to trying to fix ctypes on musllinux.
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.
Ah, if it's optional, that's a good reason. And if it helps unblock musllinux, that's also a very good reason! Thanks!
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 was able to fix ctypes for musllinux after all, in multi-build/docker-images#32
Pillow/Tests/test_image_access.py
Line 8 in f3a3b20
This import at the top level of test_image_access.py, but is only used on non-CI Windows. This PR moves the import so that if a user doesn't have setuptools installed, that doesn't stop them from running our test suite.
test_image_access.py and test_imagewin_pointers.py similarly always import ctypes for tests that only run on Windows. I have moved those imports as well.
test_file_libtiff.py imports ctypes to calculate libtiff will transform a specific value into. I've just run that calculation now, recorded the value and removed the import.