-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Only import ctypes when necessary #3178
Conversation
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.
Thank you very much @radarhere, that sounds reasonable.
I never profiled ctypes
myself. I am supposing it is costly to import, right?
@Mergifyio rebase |
I don't know - my presumption was that this was because ctypes may not be installed. Allow me to do the rebase. |
Thank you very much! It seems that there is something wrong with @Mergifyio 😅
I see, this is even a more important reason. |
Hi @radarhere, I added a news fragment according to https://setuptools.pypa.io/en/latest/development/developer-guide.html#alright-so-how-to-add-a-news-fragment. Please feel free to change. Oops, sorry for the typo 😅 |
Thanks for the news fragment. No further changes. |
In appdirs.py, ctypes is only imported within a function, rather than always for appdirs.py.
setuptools/pkg_resources/_vendor/appdirs.py
Lines 506 to 507 in 02f3821
This is also done in both copies of _manylinux.py.
setuptools/setuptools/_vendor/packaging/_manylinux.py
Lines 154 to 159 in 02f3821
setuptools/pkg_resources/_vendor/packaging/_manylinux.py
Lines 154 to 159 in 02f3821
Summary of changes
This PR applies this pattern to windows_support.py as well, only importing ctypes within
hide_file()
.This will also save non-Windows users from always requiring ctypes (yes, windows_support.py is imported regardless of platform).
Pull Request Checklist
changelog.d/
.(See documentation for details)