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

Changed the default cache directory on Windows. #5537

Merged
merged 1 commit into from Feb 10, 2021

Conversation

mmaeusezahl
Copy link
Contributor

@mmaeusezahl mmaeusezahl commented Feb 7, 2021

I recently used pyinstaller on Windows and later noticed that there were about 200MB of files syncing into my corporate Roaming Profile.

I think that the default cache directory should not be in the Roaming profile (%APPDATA% i.e. C:\Users\<UserName>\AppData\Roaming\pyinstaller) but much rather in the Local profile (%LOCALAPPDATA% i.e. C:\Users\<UserName>\AppData\Local\pyinstaller) by default, since there is no need to synchronize them between computers.

I know that I can change the behavior by setting the PYINSTALLER_CONFIG_DIR or disable syncing that folder, but I think that it would be better to adhere to Microsofts recommendation in terms of what to use the Roaming profile for. Also please note that this is of course opinionated/controversial as there is a lot of other (commercial) software that is doing it wrong too...

As a reference please see this similar case where I contributed in the same manner to the Jedi project davidhalter/jedi#1575

@bwoodsend
Copy link
Member

Ouch, that really would put a lot of software in the wrong. I personally think you're nuts synchronising a folder that's widely reated as a dumping ground for caches, settings and temporary files but oh well.

Fire a news item in there then we're good to go.

@bwoodsend
Copy link
Member

One thing to consider then if we do move this folder, we should probably clear up the old one. I'm not sure if it'll be less messy to slip in a shutil.rmtree(old_cache_dir) or just tell users that they may delete that folder.

The default cache directory should not be in the Roaming profile
(%APPDATA%) but much rather in the Local profile (%LOCALAPPDATA%)
since there is no need to synchronize it between computers.
@mmaeusezahl
Copy link
Contributor Author

mmaeusezahl commented Feb 8, 2021

Ouch, that really would put a lot of software in the wrong. I personally think you're nuts synchronising a folder that's widely reated as a dumping ground for caches, settings and temporary files but oh well.

Tbh, I'm not an expert on that matter either. And - as I noted - this is somewhat opionated or even disputed.

The idea is of course that you can log into different terminals and already find your settings in place. I'm basing my "bugfix" on this Microsoft doc combined with this information from the .NET Framework and this superuser.com thread.

Oh and there is also this gem, where (if I read in between the lines), Microsoft tells you that you'd be best off by backing up the LocalAppData to the cloud too, thereby essentially rendering the point of the roaming profile useless.

Fire a news item in there then we're good to go.

I added one. I tried to follow the wording standards from the contribution guideline. I also added the comment, that it will be safe to delete the Roaming folder for future releases of pyinstaller.

One thing to consider then if we do move this folder, we should probably clear up the old one. I'm not sure if it'll be less messy to slip in a shutil.rmtree(old_cache_dir) or just tell users that they may delete that folder.

I would strongly advocate against some deletion/moving shenanigans for multiple reasons:

  • What, if someone uses two versions of pyinstaller (different venvs) on the same PC?
  • What, if - in the future - pyinstaller actually wants to use roaming AppData for whatever reason?
  • Normally, if pyinstaller gets uninstalled, the folder stays anyway...

So overall I would say there is really two choices:

  1. Leave it as it is, don't merge this PR and essentially do what a lot of other big projects are doing (e.g. Mozilla). I would have full understanding, there is essentially no harm done and there are good workarounds.
  2. Change it now, be somewhat idealistic and possibly save some future users some head-aches ;)

I'd say it's quite safe to merge this PR, but who knows if it will break someone setup...

(Just another side-note: pip is also using LOCALAPPDATA)

@bwoodsend
Copy link
Member

I would strongly advocate against some deletion/moving shenanigans for multiple reasons:

  • What, if someone uses two versions of pyinstaller (different venvs) on the same PC?
  • What, if - in the future - pyinstaller actually wants to use roaming AppData for whatever reason?
  • Normally, if pyinstaller gets uninstalled, the folder stays anyway...

Good point.

@Legorooj Legorooj merged commit c117eba into pyinstaller:develop Feb 10, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants