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

This fixes two issues with the caching on Windows / APPDATA #1575

Merged
merged 1 commit into from
May 10, 2020
Merged

This fixes two issues with the caching on Windows / APPDATA #1575

merged 1 commit into from
May 10, 2020

Conversation

mmaeusezahl
Copy link

The problem is that Jedi starts to fill the organizational storage space in %APPDATA% on Windows. In my case (with just minimal use) I have a 400MB Jedi folder that gets synced across our organizational network each time I log in (I am using Visual Studio Code which in turn uses Jedi).

Fixes

  • The cache directory should really be %LOCALAPPDATA%
  • ~ is not a meaningful directory on Windows. It should really be
    os.path.expanduser('~'). To be honest it is probably always safe to
    assume that os.getenv('LOCALAPPDATA') executes to something sensible
    on any Windows system that hasn't been tampered with.

Reasoning and a historical side note:

This issue was already reported once here: #926

It got then partially fixed, but only for parso in a different place (thinking of which, the second issure should probably also get ported there as well): davidhalter/parso#1

Someone also temporarily fixed it in pythonVSCode (now maintained as microsoft/vscode-python): DonJayamanne/pythonVSCode#1035

But it got lost in microsoft/vscode-python@20a7bd5 when Jedi was introduced as an external dependency much rather than pulling it in as external code.

 * the cache directory should really be %LOCALAPPDATA%
 * ~ is not a meaningful directory on Windows. It should really be
   os.path.expanduser('~'). To be honest it is probably always safe to
   assume that os.getenv('LOCALAPPDATA') executes to something sensible
   on any Windows system that hasn't been tampered with.
@davidhalter
Copy link
Owner

davidhalter commented May 10, 2020

Good points. That was really my fault. I simply forgot that Jedi has also a separate path (it's debatable if that makes sense).

Thanks for the research and this fix!

I'm merging, because I believe that everybody forgot about this. It would still be nice if @micbou could verify that this is correct.

@davidhalter davidhalter merged commit 1115cbd into davidhalter:master May 10, 2020
davidhalter added a commit to davidhalter/parso that referenced this pull request May 10, 2020
See also 1115cbd94dcae6fb7b215c51f0407333c92c956e in Jedi and the PR in davidhalter/jedi#1575
@davidhalter
Copy link
Owner

I also patched it in parso: 15403fd998c2e60f9f9dc7def819dfc0ff0926f8. I don't think it matters much since as you say os.getenv('LOCALAPPDATA') should always be something meaningful. However now they should both be very similar.

@mmaeusezahl
Copy link
Author

Maybe it would be sensible to use the appdirs package in the first place instead of having roughly the same (error prone to future Windows cache locations) code in two code-bases?

Thanks for the quick merge!

@davidhalter
Copy link
Owner

That's not really an option, because I don't want dependencies for something as small as that.

@mmaeusezahl
Copy link
Author

Alright I try to keep it that way too (usually) 👍

Looking at the code of appdirs, the main difference seems to be that it tries to call a low-level Windows function instead of os.getenv. I can't really tell when this would be necessary -- it's a thing that Python would hardly support anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants