Use LOCALAPPDATA for config/cache directories on Windows#30975
Use LOCALAPPDATA for config/cache directories on Windows#30975VedantMadane wants to merge 2 commits intomatplotlib:mainfrom
Conversation
timhoffm
left a comment
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
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. |
VedantMadane
force-pushed
the
fix-windows-config-directory
branch
from
dde2cbb to
e610dcc
Compare
- Moves Windows config/cache to %LOCALAPPDATA%\matplotlib - Maintains backwards compatibility with %USERPROFILE%\.matplotlib - Fixes LCOV reporting on ARM runners - Fixes Ruff linting in tests - Corrects API change documentation filename
VedantMadane
force-pushed
the
fix-windows-config-directory
branch
from
e610dcc to
2a0795f
Compare
|
There's a script issue in the step "Filter C coverage". |
There was a problem hiding this comment.
Why is this file changed? It doesn't apply to Windows.
There was a problem hiding this comment.
That file was changed because it contained a fatal YAML syntax error (a duplicated else block) within the "Filter C coverage" step.
Even though that specific workflow currently only runs on Linux and macOS runners, a syntax error in any workflow file can cause GitHub Actions to fail to parse the repository's CI configuration correctly leading to all checks failing.
Tim mentioned the script issue in the step Filter C coverage which is defined in that file.
Fixing this syntax error was required to resolve the "12 checks failing" allowing the CI pipeline to actually execute and report results for my changes.
There was a problem hiding this comment.
@QuLogic you good with this? Your request changes is blocking at the moment
There was a problem hiding this comment.
No, this explanation does not make any sense. There is no duplicated else block being removed in this PR, because this PR introduced it in the first commit. There is no explanation why things are now being quoted, and if it is necessary, it's still unrelated to this PR.
6 participants
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