-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Use vendored appdirs and patch it to not use win32com #4945
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
Conversation
Ping? |
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.
Warning - I just spotted #4000 which is a Unicode-specific fix in our version of appdirs. Before merging this change, it's worth confirming that the vendored appdirs doesn't reintroduce that issue.
Nice spot! I'll add back that test somewhere and then look into it. :) |
I spent a bit of time fiddling around. I think I'm gonna break this up into 2 PRs, one for disabling win32com on the vendored appdirs and another for switching over to the vendored appdirs. The reasoning is that I looked at the blame of the current utils.appdirs and it seems like we have more than just #4000 in it. It'll be simpler to break this up. As an example: this PR currently changes the config directory on Windows to If this makes sense, I'll close this one and open two new PRs with these changes separated. |
Okay. Closing. :) |
Isn't the appdirs unicode failure a generic bug that should be reported upstream in accordance with pip policy? I don't think that particular problem is constrained to pip. |
Agreed. I'd also like to see "don't use pywin32" requested upstream - there's as far as I know never a reason to use it as ctypes is available on all supported Windows versions of Python. I'm happy to carry the patch to remove pywin32 temporarily, but long-term I'd like to see it pushed upstream. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Fixes #4874