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.

Sign up for GitHub

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

Jump to bottom

Simplify pywin32_bootstrap, avoid importing site #1651

Merged
merged 6 commits into from Jan 21, 2021
Merged

Simplify pywin32_bootstrap, avoid importing site #1651

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
979b205
Simplify pywin32_bootstrap, avoid importing site
wkschwartz Jan 15, 2021
d9fb26f
Add changelog entry for #1651
wkschwartz Jan 20, 2021
cb51ca4
Rename variable per review comment
wkschwartz Jan 20, 2021
df3e334
Simplify per review comment
wkschwartz Jan 20, 2021
7a6de27
Use ImportError instead of ModuleNotFoundError
wkschwartz Jan 20, 2021
74c96dc
Avoid importing `os` unless necessary
wkschwartz Jan 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 17 additions & 13 deletions win32/Lib/pywin32_bootstrap.py
View file
Open in desktop
Expand Up @@ -7,22 +7,27 @@
# If Python has `os.add_dll_directory()`, we need to call it with this path.
# Otherwise, we add this path to PATH.
import os
import site

# The directory should be installed under site-packages.

dirname = os.path.dirname
# This is to get the "...\Lib\site-packages" directory
# out of this file name: "...\Lib\site-packages\win32\Lib\pywin32_bootstrap.py".
# It needs to be searched when installed in virtual environments.
level3_up_dir = dirname(dirname(dirname(__file__)))
try:
Exc = ModuleNotFoundError # Introduced in Python 3.6
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this strictly necessary? IIUC, ModuleNotFoundError derives from ImportError and I don't see a good reason to differentiate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My instinct is generally to pick the most specific possible exception for except to match. This avoids hiding errors unrelated to the absence of pywin32_system32. There would be some bizarre circumstances that would lead to such hiding, but the cost of being specific here seems low to me. Anyway, I won't die on this hill. If you insist, I'll just use ImportError.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel too strongly about it, and in general I'd agree, but in this case I feel the added complexity from changing behaviors based on the version, when in practice it's exactly the same behavior regardless, swings the argument to just catching ImportError.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7a6de27

except NameError:
Exc = ImportError

site_packages_dirs = getattr(site, "getsitepackages", lambda: [])()
if level3_up_dir not in site_packages_dirs:
site_packages_dirs.insert(0, level3_up_dir)
try:
import pywin32_system32
except Exc:
path_iterator = iter([])
else:
# We're guaranteed only that __path__: Iterable[str]
# https://docs.python.org/3/reference/import.html#__path__
path_iterator = iter(pywin32_system32.__path__)

for site_packages_dir in site_packages_dirs:
pywin32_system32 = os.path.join(site_packages_dir, "pywin32_system32")
try:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably simplify this by putting the rest of the body under this else - ie, just have a pass on the import error and everything else in the else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think df3e334 is responsive to this.

pywin32_system32 = next(path_iterator)
except StopIteration:
pass
else:
if os.path.isdir(pywin32_system32):
if hasattr(os, "add_dll_directory"):
os.add_dll_directory(pywin32_system32)
Expand All @@ -31,4 +36,3 @@
elif not os.environ["PATH"].startswith(pywin32_system32):
os.environ["PATH"] = os.environ["PATH"].replace(os.pathsep + pywin32_system32, "")
os.environ["PATH"] = pywin32_system32 + os.pathsep + os.environ["PATH"]
break