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.

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

Drop the internal copy of "appdirs" to use the vendored one #6040

Closed
pradyunsg opened this issue Nov 25, 2018 · 6 comments
Closed

Drop the internal copy of "appdirs" to use the vendored one #6040

pradyunsg opened this issue Nov 25, 2018 · 6 comments
Labels
auto-locked Outdated issues that have been locked by automation project: vendored dependency Related to a vendored dependency type: refactor Refactoring code

Comments

@pradyunsg
Copy link
Member

See #4945 and #4874 (comment) for context.

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.

@pradyunsg pradyunsg added type: refactor Refactoring code project: vendored dependency Related to a vendored dependency labels Nov 25, 2018
@pradyunsg pradyunsg added this to the Internal Cleansing milestone Nov 25, 2018
@pradyunsg
Copy link
Member Author

This will mostly involve figuring out which patches have been made to pip's copy of appdirs and merging them upstream.

@pradyunsg
Copy link
Member Author

pradyunsg commented Jan 3, 2019

Okay -- figured those things out. Now, things to do are:

  • propose PRs for 4000, 4302 upstream
  • Minor typo fixes, if relevant
  • figure out how to do 4100 without any upstream changes

Notes:

#3275 -- Use os.path.join; instead of os.sep.join([])
#4000 -- Unicode fixes
#4100 -- Added "~/.config/pip/pip.conf" on MacOS
#4302 -- Escape sequences
#3790 -- Typo fixes (merged in ActiveState/appdirs#60)
#3136 -- can revert in pip; all latest minor versions fixed the bug.
#3231 -- typos (appdirs' code unchanged)
#3099 -- no action needed; removed a not-used function
#2467 -- Remove dependency on pywin32 as ctypes should always be available (this has been proposed already; pip currently carries a patch for this)
f63d1e3 -- I don't think any changes to upstream here.

@pradyunsg
Copy link
Member Author

More context: The relevant changes have been proposed upstream and, well, upstream appdirs doesn't seem to have much activity.

@uranusjr has filed #7501, patching our vendored appdirs instead, with our enhancements. IMO that's the better route to go here.

@uranusjr
Copy link
Member

It’s a little unfortunate how little interest upstream has to various improvements. I guess the library is probably “finished” from their perspective.

@uranusjr
Copy link
Member

So, with #7501 done, should we eliminate pip._internal.utils.appdirs completely? I actually kind of like how the module encapsulates pip’s usage to appdirs in a self-documenting way—no appauthors, different roaming preferences, return multiple site config dirs, etc.

@pradyunsg
Copy link
Member Author

No, I don't think so, for the same reason as you pointed out. Let's close this issue. :)

Thanks a lot for tackling this @uranusjr! ^>^

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jan 24, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation project: vendored dependency Related to a vendored dependency type: refactor Refactoring code
Projects
None yet
Development

No branches or pull requests

2 participants