Skip to content

Only import ctypes when necessary#3178

Merged
abravalheri merged 3 commits into
pypa:mainfrom
radarhere:ctypes
Mar 23, 2022
Merged

Only import ctypes when necessary#3178
abravalheri merged 3 commits into
pypa:mainfrom
radarhere:ctypes

Conversation

@radarhere

@radarhere radarhere commented Mar 19, 2022

Copy link
Copy Markdown
Contributor

In appdirs.py, ctypes is only imported within a function, rather than always for appdirs.py.

def _get_win_folder_with_ctypes(csidl_name):
import ctypes

This is also done in both copies of _manylinux.py.
def _glibc_version_string_ctypes() -> Optional[str]:
"""
Fallback implementation of glibc_version_string using ctypes.
"""
try:
import ctypes

def _glibc_version_string_ctypes() -> Optional[str]:
"""
Fallback implementation of glibc_version_string using ctypes.
"""
try:
import ctypes

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

@abravalheri abravalheri left a comment

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.

Thank you very much @radarhere, that sounds reasonable.

I never profiled ctypes myself. I am supposing it is costly to import, right?

@abravalheri

Copy link
Copy Markdown
Contributor

@Mergifyio rebase

@radarhere

Copy link
Copy Markdown
Contributor Author

I never profiled ctypes myself. I am supposing it is costly to import, right?

I don't know - my presumption was that this was because ctypes may not be installed.

Allow me to do the rebase.

@abravalheri

Copy link
Copy Markdown
Contributor

Allow me to do the rebase.

Thank you very much! It seems that there is something wrong with @Mergifyio 😅

I don't know - my presumption was that this was because ctypes may not be installed.

I see, this is even a more important reason.

@abravalheri

abravalheri commented Mar 23, 2022

Copy link
Copy Markdown
Contributor

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 😅

@radarhere

Copy link
Copy Markdown
Contributor Author

Thanks for the news fragment. No further changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants