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

Do not always require ctypes in tests #6141

Merged
merged 2 commits into from
Mar 22, 2022
Merged

Conversation

radarhere
Copy link
Member

from setuptools.command.build_ext import new_compiler

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.

@hugovk
Copy link
Member

hugovk commented Mar 18, 2022

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.

Is it possible to run our test suite without setuptools? Isn't setuptools needed to build/install Pillow for it to be testable?

@radarhere radarhere changed the title Do not always require ctypes and setuptools in tests Do not always require ctypes in tests Mar 19, 2022
@radarhere
Copy link
Member Author

Ok, I've taken out the setuptools change.

@@ -404,6 +403,8 @@ class TestEmbeddable:
"not from shell",
)
def test_embeddable(self):
import ctypes
Copy link
Member

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.

Copy link
Member Author

@radarhere radarhere Mar 22, 2022

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.

Copy link
Member

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!

Copy link
Member Author

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

@hugovk hugovk merged commit d0f1f66 into python-pillow:main Mar 22, 2022
@radarhere radarhere deleted the imports branch March 22, 2022 22:17
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 this pull request may close these issues.

None yet

2 participants