Appdirs vendor should not use win32api

[ghost]

_vendor/appdirs.py imports win32api. It should not because it loads a c library.

[pfmoore]

It's in conditional code that falls back to ctypes, which is always available. So there's no issue.

Pip doesn't have a prohibition against importing C code, just against vendoring it. And while we don't typically rely on non-vendored modules outside of the stdlib, it's fine if vendored modules do so as long as the external module is optional (which this is).

[ghost]

pip actually failed when I installed a version of pywin32 that I was hacking on. I consider that to be a bug.

Edit: the failure was that it couldn't remove the win32api DLL because it was locked.

[pfmoore]

Hmm, presumably the failure was because of a problem with your modified pywin32? Nevertheless, I agree - as appdirs is vendored, I'd suggest raising the issue with them. Given that ctypes is always available, I see no reason for having that pywin32 code, but if they do want to retain it, having an option to ignore pywin32 and/or fall back to ctypes on errors seems reasonable.

[benoit-pierre]

I was more surprised by the fact that pip has 2 versions of appdirs: pip/_vendor/appdirs.py and pip/_internal/utils/appdirs.py

[ghost]

Hmm, presumably the failure was because of a problem with your modified pywin32?

The failure was because I modified the import mechanism to load pywin32 C extensions just like every other C extension that Python uses. This is a somewhat technical and detailed topic, but IMO the modified pywin32 was perfectly valid.

[ghost]

@pfmoore I'm not sure that this is an appdirs issue. It works fine except when pip uses the code (because of what pip is doing).

[pfmoore]

@xoviat Can you clarify? The only place in pip which loads pywin32 is in pip\_vendor\appdirs.py. That's a vendored copy of appdirs, so any fix needs to be made upstream.

@benoit-pierre Agreed that's weird. Looks like pkg_resources (which we vendor) uses appdirs, so we need to vendor appdirs for them. Pip itself uses its own modified copy that doesn't import pywin32.

[ghost]

That's correct. The code works for every other application, except pip which may be removing DLLs. Is it their responsibility to fix problems that are unique to pip?

[ghost]

What appears to have happened is that somewhere along the way, someone discovered this issue and fixed it. Then sometime later, a vendored copy was brought in that reintroduced the problem.

[pradyunsg]

Pip itself uses its own modified copy that doesn't import pywin32.

I think we should patch appdirs that we vendor and use that instead then?

[pfmoore]

@xoviat

except pip which may be removing DLLs

I don't know how this could be happening. I'm not even sure what you mean by "removing DLLs". If you can provide a precise explanation of what you think is happening, that would help.

@pradyunsg

I think we should patch appdirs that we vendor and use that instead then?

I don't think we should. My point is that pip itself isn't using the vendored appdirs at all. So we (in theory) don't care. Our vendored pkg_resources appears to need appdirs, so we provide it in our vendored modules for pkg_resources' purposes. My preference would be for pkg_resources and appdirs to resolve this themselves, and not involve pip. This is based on an assumption that if a user has a pywin32 installation that they are hacking on the way @xoviat has, then it should work.

If @xoviat can establish that a standalone pkg_resources works fine and yet the version vendored in pip fails in the same circumstances, then it's a pip issue. But until it's confirmed that it only happens in the vendored situation, my preference is to assume it's an issue in the upstream packages.

[ghost]

I don't know how this could be happening.

pip removes the DLLs when it uninstalls pywin32. However, the win32api DLL can be locked in certain circumstances.

[pfmoore]

OK, so now you really need to follow the standard "reporting a bug" process. Please provide the exact sequence of commands you used, describe what you expected to happen, and what actually happened.

[ghost]

It's

pip install win32compat
pip uninstall -y win32core win32compat

[ghost]

Traceback:

  File "c:\python36\lib\shutil.py", line 384, in _rmtree_unsafe
    _rmtree_unsafe(fullname, onerror)
  File "c:\python36\lib\shutil.py", line 384, in _rmtree_unsafe
    _rmtree_unsafe(fullname, onerror)
  File "c:\python36\lib\shutil.py", line 384, in _rmtree_unsafe
    _rmtree_unsafe(fullname, onerror)
  File "c:\python36\lib\shutil.py", line 389, in _rmtree_unsafe
    onerror(os.unlink, fullname, sys.exc_info())
  File "c:\python36\lib\site-packages\pip\utils\__init__.py", line 114, in rmtree_errorhandler
    func(path)
PermissionError: [WinError 5] Access is denied: 'C:\\Users\\User\\AppData\\Local\\Temp\\pip-y623lrwy-uninstall\\pyth
on36\\lib\\site-packages\\win32\\types.cp36-win_amd64.pyd'

[ghost]

If you want to go the long way around, dig into the C code, trace all of the locks, you'll eventually find that the simplest solution is to simply comment out that code in appdirs.py and use the ctypes version. But whatever floats your boat.

[benoit-pierre]

Maybe pip could patch appdirs._get_win_folder to not use pywin32, for example (untested):

 src/pip/_internal/__init__.py | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git i/src/pip/_internal/__init__.py w/src/pip/_internal/__init__.py
index 532edd26..4b991c60 100755
--- i/src/pip/_internal/__init__.py
+++ w/src/pip/_internal/__init__.py
@@ -38,6 +38,20 @@ else:
         else:
             securetransport.inject_into_urllib3()
 
+
+if sys.platform == "win32":
+    from pip._vendor import appdirs
+    try:
+        from ctypes import windll
+        appdirs._get_win_folder = appdirs._get_win_folder_with_ctypes
+    except ImportError:
+        try:
+            import com.sun.jna
+            appdirs._get_win_folder = appdirs._get_win_folder_with_jna
+        except ImportError:
+            appdirs._get_win_folder = appdirs._get_win_folder_from_registry
+
+
 from pip import __version__
 from pip._internal import cmdoptions
 from pip._internal.exceptions import CommandError, PipError

[pradyunsg]

I don't think we should.

I don't really care much about it unless this can break with a sequence of pip install and uninstall. it's a pip bug if pip install xyz followed by pip uninstall xyz can break pip.

If it does break, pip should patch appdirs to unconditionally use ctypes... or that should happen upstream. I have a slight preference for latter.

---

If you want to go the long way around, dig into the C code, trace all of the locks, you'll eventually find that the simplest solution is to simply comment out that code in appdirs.py and use the ctypes version. But whatever floats your boat.

This feels uncalled for. Please don't make a comment that might put others off. We're all trying to help each other here on our own free time. Let's keep our tones positive. :)

Anyway, I don't want to derail the discussion here nor do I think I'd have any major inputs on this so I'll recede to spectating this issue now.

[ghost]

This feels uncalled for. Please don't make a comment that might put others off.

Did not mean to put anyone off with my comment. Sorry about that.

[dstufft]

Isn't this just the pip can't import C code on Windows, because Python holds a lock to the dll file and thus we error out trying to remove the file on uninstall? IIRC we've commented out simplejson and pyopenssl support on Windows for the same reason?

[ghost]

Isn't this just the pip can't import C code on Windows, because Python holds a lock to the dll file and thus we error out trying to remove the file on uninstall?

Yes, it's exactly that.

IIRC we've commented out simplejson and pyopenssl support on Windows for the same reason?

Yes, this is an excellent solution. OK to implement?

[dstufft]

What's importing pip._vendor.appdirs?

[ghost]

pip/src/pip/_vendor/pkg_resources/__init__.py

Line 73 in 87d2735

from pip._vendor import appdirs

[pradyunsg]

Isn't this just the pip can't import C code on Windows, because Python holds a lock to the dll file and thus we error out trying to remove the file on uninstall?

Yep.

---

pkg_resources uses appdirs. pip has it's own copy of appdirs in utils.appdirs. I'll repeat that I think it's a good idea to patch the vendored appdirs to behave the way we want and switch to using that.

[pfmoore]

Isn't this just the pip can't import C code on Windows, because Python holds a lock to the dll file

Yes, it appears so. Apologies for any confusion I may have caused, but I didn't have any idea that's the issue that was being reported.

Rather than patching the vendored appdirs (which I don't have an issue with now I know what the problem is), could we patch the vendored pkg_resources to use our version of appdirs, and avoid vendoring appdirs at all? After all, pkg_resources is only using user_cache_dir, and we have our own implementation of that.

Or we could monkeypatch it at runtime, as follows (untested):

# Monkeypatch pkg_resources to use our version of appdirs
import pip._vendor.pkg_resources
import pip._internal.utils.appdirs
pip._vendor.pkg_resources.appdirs = pip._internal.utils.appdirs

[ghost]

I don't think it's possible to monkeypatch pkg_resources because it may initialize the working set on import.

[pradyunsg]

I think we should drop pip._internal.utils.appdirs and patch the vendored appdirs and use it. Thoughts?

[pfmoore]

Yeah, overall that seems like the best option.