Use LOCALAPPDATA for config/cache directories on Windows#30975
Use LOCALAPPDATA for config/cache directories on Windows#30975QuLogic merged 1 commit intomatplotlib:mainfrom
Conversation
timhoffm
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
Please also check whether there are other parts of the documentation that may need an update: https://matplotlib.org/stable/search.html?q=MPLCONFIGDIR
|
Thanks for the review @timhoffm! I've addressed both suggestions: switched to pathlib for mkdir and added an explicit assertion for the expected configdir path. |
| # Clear cached values | ||
| matplotlib.get_configdir.cache_clear() | ||
| matplotlib.get_cachedir.cache_clear() |
There was a problem hiding this comment.
This does not work. But why do you need it? You run get_configdir() in a separate subprocess.
|
What is the path for back compatibility? We do not want to break users that have config they are relying on reading without warning and allow users to switch version of mpl without having to duplicate their config. |
|
Thanks @timhoffm and @tacaswell for the feedback. Changes made:
|
|
This compatibility looks reasonable to me. We can later decide whether we want to actively push towards the new location. |
|
The fallback logic all looks correct to me, but could you please add a test for the case where %USERPROFILE%.matplotlib already exists? That wouldn't otherwise be exercised in CI, and I don't think too many of us on the dev team are on windows to verify that the migration is working. |
scottshambaugh
left a comment
There was a problem hiding this comment.
Looks like you need to rebase to main, but otherwise this looks good to me. Thanks for putting it together!
|
Actually it looks like CI was unhappy for some other reason. Please rebase or re-push to kick off the tests again, and I'll merge after they pass. |
e610dcc to
2a0795f
Compare
|
There's a script issue in the step "Filter C coverage". |
bc01142 to
e1a4328
Compare
|
@VedantMadane Can you please squash this to one commit? I agree with @QuLogic there is a bunch of thrashing on tests.yml that is added and then removed. |
|
@tacaswell if everything else is ok, can't we just squash-merge? |
e1a4328 to
ce421a8
Compare
|
Technically yes. Either way I would like some clarity from @VedantMadane about what the (undone) changes were trying to do. |
@tacaswell some test for other other operating system was failing. I worked on Windows specific functionality and was unsure how it could affect other OS' tests. I looked around other PRs to find if this was a problem with the test itself or something I did which was inadvertently affecting those. I was experimenting with things I could do to get the tests to pass. These were the changes that I later reverted. |
On Windows, the default configuration and cache directories now use %LOCALAPPDATA%\matplotlib instead of %USERPROFILE%\.matplotlib, following Windows application data storage conventions. This PR adds backwards compatibility by checking if %USERPROFILE%\.matplotlib already exists before using the new location. Closes matplotlib#20779
ce421a8 to
6dd3a69
Compare
|
Thanks @VedantMadane! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again. |
Summary
On Windows, the default configuration and cache directories now use
%LOCALAPPDATA%\matplotlibinstead of%USERPROFILE%\.matplotlib, following Windows application data storage conventions.This addresses the issue where matplotlib was placing its config folder directly in the user's home directory (
%USERPROFILE%), which is not the recommended location on Windows.Changes
_get_config_or_cache_dir()to check for Windows (sys.platform == 'win32') and use%LOCALAPPDATA%when available%USERPROFILE%\.matplotlibifLOCALAPPDATAis not setget_configdir()andget_cachedir()to document the Windows behaviorReferences
Closes #20779