Simplify pywin32_bootstrap, avoid importing site #1651

wkschwartz · 2021-01-15T19:42:29Z

Since pywin32 has dropped support for Python 2.7 since version 300 and support for Python 3.0 through 3.4 since version 223, we can now rely on PEP-420 semantics when importing names of directories available on sys.path and not containing an __init__.py. This applies to to pywin32_system32. The previous version of win32.lib.pywin32_boostrap manually searched site-packages directories, stopping at the first one found. Now we pass this responsibility off to Python's import machinery. When we import pywin32_system32, if successful, pywin32_system32 will likely be a _frozen_importlib_external._NamespacePath object. We obtain its first entry, being careful that it didn't support sequence indexing until Python 3.7 or so (and in any case, that's all undocumented; the documentation promises only that __path__: Iterable[str]). This first entry is the path to the directory containing the DLLs, just as before, so the remainder of the code is unchanged.

The only semantic differences between this implementation and the previous one is that

we no longer import site (always better to import less!), and
pywin32_system32 is no longer required to be in a site-packages directory

Just as with the previous implementation, this one does not raise an exception if pywin32_system32 cannot be found.

I did one annoying thing in this patch to minimize the diff, which is that I redefine the variable pywin32_system32: first it refers to a namespace package module object, and then later it refers to the str directory name. It might be reasonable after initial review of the patch to rename the latter instance of the variable to something like path.

Finally, I added no tests and no change log entry, but can if necessary.

Since pywin32 has dropped support for Python 2.7 since version 300 and support for Python 3.0 through 3.4 since version 223, we can now rely on [PEP-420] semantics when importing names of directories available on `sys.path` and not containing an `__init__.py`. This applies to to `pywin32_system32`. The previous version of `win32.lib.pywin32_boostrap` manually searched site-packages directories, stopping at the first one found. Now we pass this responsibility off to Python's import machinery. When we `import pywin32_system32`, if successful, `pywin32_system32` will likely be a `_frozen_importlib_external._NamespacePath` object. We obtain its first entry, being careful that it didn't support sequence indexing until Python 3.7 or so (and in any case, that's all undocumented; the [documentation promises] only that `__path__: Iterable[str]`). This first entry is the path to the directory containing the DLLs, just as before, so the remainder of the code is unchanged. The only semantic differences between this implementation and the previous one is that 1. we no longer import `site` (always better to import less!), and 2. `pywin32_system32` is no longer _required_ to be in a site-packages directory Just as with the previous implementation, this one does not raise an exception if `pywin32_system32` cannot be found. I did one annoying thing in this patch to minimize the diff, which is that I redefine the variable `pywin32_system32`: first it refers to a namespace package module object, and then later it refers to the `str` directory name. It might be reasonable after initial review of the patch to rename the latter instance of the variable to something like `path`. Finally, I added no tests and no change log entry, but can if necessary. [PEP-420]: https://www.python.org/dev/peps/pep-0420/#specification [documentation promises]: https://docs.python.org/3/reference/import.html#__path__

mhammond

Thanks, I don't see a problem with this approach. I think renaming the var makes sense, and I'd also like a changelog entry just to help people should this have unintended consequences.

mhammond · 2021-01-20T02:13:23Z

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


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

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.

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.

Fixed in 7a6de27

mhammond · 2021-01-20T02:16:11Z

win32/Lib/pywin32_bootstrap.py


-for site_packages_dir in site_packages_dirs:
-    pywin32_system32 = os.path.join(site_packages_dir, "pywin32_system32")
+try:


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?

I think df3e334 is responsive to this.

mhammond#1651 (review)

mhammond#1651 (comment) This very slightly changes semantics: if `pywin32_system32` is found to have multiple entries in `__path__` and the first one is not an existing directory, now the search will continue whereas before it would give up.

Per review comment: mhammond#1651 (comment)

Normally I wouldn't push the line on PEP 8 for importing `os`, but `win32.lib.pywin32_bootstrap` is imported by `site` at Python startup via pywin32.pth. Programs that care a lot about startup time might appreciate our caution here.

mhammond

Awesome, thanks!

wkschwartz · 2021-01-20T04:00:39Z

I believe I've responded to everything. 74c96dc is not strictly necessary, just seemed like a good idea. See the commit message. I can revert it if you'd prefer.

jiasli · 2021-05-08T03:53:17Z

There is still one small behavior change.

Previously it is possible to use the system python to manually call pywin32_bootstrap.py within a virtual env:

python D:\cli\env38\Lib\site-packages\win32\lib\pywin32_bootstrap.py
site_packages_dirs: ['D:\\cli\\env38\\Lib\\site-packages', 'C:\\Users\\xxx\\AppData\\Local\\Programs\\Python\\Python38', 'C:\\Users\\xxx\\AppData\\Local\\Programs\\Python\\Python38\\lib\\site-packages']

Now import pywin32_system32 will trigger ImportError because pywin32_system32 is not known by the system python.

However, this is no big deal as

pywin32_bootstrap is automatically imported twice by

pywin32/pywin32.pth

Line 7 in 5f1028f

import pywin32_bootstrap

The code of pywin32_bootstrap.py is only executed the first time it is imported, so site.getsitepackages() only returns

['C:\\Users\\xxx\\AppData\\Local\\Programs\\Python\\Python38', 'C:\\Users\\xxx\\AppData\\Local\\Programs\\Python\\Python38\\lib\\site-packages']

That's why the original patch is necessary:

pywin32/win32/Lib/pywin32_bootstrap.py

Lines 20 to 22 in 40a8f25

    
           site_packages_dirs = getattr(site, "getsitepackages", lambda: [])() 
        
           if level3_up_dir not in site_packages_dirs: 
        
               site_packages_dirs.insert(0, level3_up_dir)

using the system python to call something inside a virtual env is a strange usage after all! (I shouldn't have done this.)

Nice work @wkschwartz @mhammond! Really appreciate what you have been doing.

mhammond requested changes Jan 20, 2021

View reviewed changes

wkschwartz added 5 commits January 19, 2021 21:38

Add changelog entry for mhammond#1651

d9fb26f

Rename variable per review comment

cb51ca4

mhammond#1651 (review)

Simplify per review comment

df3e334

mhammond#1651 (comment) This very slightly changes semantics: if `pywin32_system32` is found to have multiple entries in `__path__` and the first one is not an existing directory, now the search will continue whereas before it would give up.

Use ImportError instead of ModuleNotFoundError

7a6de27

Per review comment: mhammond#1651 (comment)

Avoid importing os unless necessary

74c96dc

Normally I wouldn't push the line on PEP 8 for importing `os`, but `win32.lib.pywin32_bootstrap` is imported by `site` at Python startup via pywin32.pth. Programs that care a lot about startup time might appreciate our caution here.

mhammond approved these changes Jan 20, 2021

View reviewed changes

mhammond merged commit f3f55ab into mhammond:master Jan 21, 2021

wkschwartz deleted the bootstrap branch January 21, 2021 04:27

jiasli mentioned this pull request May 8, 2021

{Packaging} Remove pypiwin32 and bump pywin32 to 300 Azure/azure-cli#18012

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify pywin32_bootstrap, avoid importing site #1651

Simplify pywin32_bootstrap, avoid importing site #1651

wkschwartz commented Jan 15, 2021

mhammond left a comment

mhammond Jan 20, 2021

wkschwartz Jan 20, 2021

mhammond Jan 20, 2021

wkschwartz Jan 20, 2021

mhammond Jan 20, 2021

wkschwartz Jan 20, 2021

mhammond left a comment

wkschwartz commented Jan 20, 2021

jiasli commented May 8, 2021

Simplify pywin32_bootstrap, avoid importing site #1651

Simplify pywin32_bootstrap, avoid importing site #1651

Conversation

wkschwartz commented Jan 15, 2021

mhammond left a comment

Choose a reason for hiding this comment

mhammond Jan 20, 2021

Choose a reason for hiding this comment

wkschwartz Jan 20, 2021

Choose a reason for hiding this comment

mhammond Jan 20, 2021

Choose a reason for hiding this comment

wkschwartz Jan 20, 2021

Choose a reason for hiding this comment

mhammond Jan 20, 2021

Choose a reason for hiding this comment

wkschwartz Jan 20, 2021

Choose a reason for hiding this comment

mhammond left a comment

Choose a reason for hiding this comment

wkschwartz commented Jan 20, 2021

jiasli commented May 8, 2021