feat: Use platformdirs for XDG support#2541
Conversation
|
Thanks, will review when able. |
|
Shout-out to @ArnabTechiee for inspiring me to actually get this fixed, I hope that between the two of us we find a better solution here. |
|
Ah, oops -- I keep forgetting the |
|
I see I'm missing some steps, and I'm noticing some breakage in pytest. Marking this as a draft in the meantime. |
|
OK, I have ironed out the bugs visible to me. I've passed the code through |
|
Anything blocking this being merged? |
|
Do you need me to rebase this, or do you want to merge to dev directly? |
|
I ended up rebasing -- it mostly went cleanly, however I noticed the lines I was modifying in |
|
Ah, wait, I think I mismerged the change to |
|
... and a |
e3d67c2 to
14210bb
Compare
Instead of reinventing the wheel, just use the standard apis here. In fact, this was the approach introduced in 4f49e8e, and the platformdirs dependency has stuck around since. This should have the added bonus of making our configuration more idiomatic in Windows and Mac. Furthermore, make tests use temp dirs for their configuration, and in particular check for both idiomatic and non-idiomatic (backwards-compatible) configurations. Finally, use this opportunity to split the directories used according to use (using XDG variables for reference): - Under XDG_CONFIG_HOME: config.json - Under XDG_CACHE_HOME: the spotify and spotipy caches and the temp dir - Under XDG_DATA_HOME: the ffmpeg binary and web-ui - Under XDG_STATE_HOME: the web sessions and the logs Also use the occasion to rename .spotipy -> spotipy and .spotify_cache -> spotify_cache -- there's no need to doubly hide such directories, given the spotify directories are likely to be hidden anyway. API changes (grepped the codebase to make sure there are no remaining references, but these can be reverted if needed): - Remove: spotdl.utils.config.get_spotdl_path This no longer makes sense, now spotdl's files aren't concentrated in a single directory - Rename: spotdl.utils.config.get_cache_path -> get_spotipy_client_cache_path Made the stylistic choice to reserve the generic names get_{config,cache,data,state}_path for the "roots" of those kinds of paths, and to otherwise name the functions after the directories' usecase. Reordered the functions accordingly. Avoided the obvious name of get_spotipy_cache_path to distance it from the existing get_spotify_cache_path. - Add: spotdl.utils.ffmpeg.get_local_ffmpeg_path Gives the path to where the local ffmpeg binary would be, regardless of whether it exists -- used for deduplicating logic in spotdl.utils.ffmpeg, see below. - Add: tests.conftest.config_paths, tests.conftest.config_dirs, tests.conftest.populate_config config_paths gives a single place where the list of all paths spotdl uses can be listed, along with their current default values. populate_config takes a set of paths and populates them with the default config -- this is used in eg test_config to populate the various permutations of configurations. We write the default config JSON to those directories so spotdl has something to work with. config_dirs sets up a temp dir configured for XDG with all the spotdl paths populated. This also permits cutting down on the use of monkeypatching in other tests -- see test_init, test_ffmpeg and test_metadata Drive-by improvements: - spotdl.utils.ffmpeg Deduplicate the logic in get_local_ffmpeg and download_ffmpeg for finding the path to the local ffmpeg. - spotdl.web.api Use more idiomatic path construction, and in particular use the / operator instead of embedding / in strings passed to the Path constructor (should be more reliable) Implementation notes: - spotdl.utils.config.get_path_fallback The current implementation of the file fallback picks its behaviours according to whether the parent directory exists. For our current purposes, this is sufficient (and necessary, given we want to signal when a config file is missing), but this does mean we forgo being able to move config files into subdirectories (eg `~/.spotdlrc` -> `~/.spotdl/spotdlrc`) - spotdl.utils.config.old_spotdl_dirs In order for this list to be affected by the monkeypatching to `os.environ` in tests, we need to wrap it in a function so it gets reevaluated every time. This way, it picks up the changes to the environment. A hack, but I see no better way around it. - test_config To be able to parametrize over the choice of fixtures, request.getfixturevalue needed to be used. This is slightly black magic, but it seems well-supported. Closes: spotDL#2058, spotDL#2387 Updates: spotDL#2514, spotDL#2540
|
OK, done. Ended listing the fallback paths in |
Silverarmor
left a comment
There was a problem hiding this comment.
Thanks for rebasing. Will try merge this by end of month.
ditto comment throughout the whole of config.py re variable names
|
|
||
| return get_path_fallback( | ||
| *( | ||
| r / d |
There was a problem hiding this comment.
Could we use proper variable names here?
| return get_spotdl_path() / ".spotipy" | ||
| return get_path_fallback( | ||
| *( | ||
| [r / d for r in [pd.user_data_path(appname="spotdl")] for d in ["utils"]] |
There was a problem hiding this comment.
same here, r d` not intuitive
There was a problem hiding this comment.
Pull request overview
This PR migrates spotDL’s path handling to platformdirs to provide idiomatic XDG Base Directory support (and better Windows/macOS defaults), while keeping backward compatibility for legacy directory layouts. It also updates tests and documentation to reflect the new directory structure and path APIs.
Changes:
- Replace bespoke path logic with
platformdirsand split paths by purpose (config/cache/data/state/log). - Refactor ffmpeg local-path handling and update web session storage to use state directories.
- Update tests to run against temporary XDG-style directories and validate both modern and legacy layouts; update CLI/docs text accordingly.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
spotdl/utils/config.py |
Introduces platformdirs-based path helpers and legacy fallbacks; restructures config/cache/data/state/log locations. |
spotdl/utils/ffmpeg.py |
Deduplicates local ffmpeg path logic via get_local_ffmpeg_path() and switches storage under the new utils/data directory. |
spotdl/utils/spotify.py |
Updates Spotipy cache path usage to the renamed cache-path helper. |
spotdl/web/api.py |
Moves web session directory handling from the old single “spotdl dir” to the new state directory. |
spotdl/web/routes.py |
Same as above for routes that create session output directories. |
spotdl/console/web.py |
Updates session cleanup to use the new state directory path. |
spotdl/utils/arguments.py |
Updates help strings for new default config/cache locations. |
tests/conftest.py |
Adds XDG temp-dir fixture helpers and config population utilities for path-related tests. |
tests/utils/test_config.py |
Reworks config-path tests to cover legacy, new-XDG, and fully-populated-XDG scenarios. |
tests/utils/test_ffmpeg.py |
Switches tests to use XDG temp dirs and updated output locations. |
tests/utils/test_metadata.py |
Switches tests to use XDG temp dirs and updated output locations. |
tests/test_init.py |
Removes old get_spotdl_path monkeypatching in favor of the new XDG temp-dir fixture. |
docs/usage.md |
Updates docs to reflect new default config/cache paths and examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return ( | ||
| get_path_fallback( | ||
| *([pd.user_config_path(appname="spotdl")] + old_spotdl_dirs()) | ||
| ) | ||
| / "config.json" |
| pd.user_data_path(appname="spotdl") / "web" / "static", | ||
| *( | ||
| r / d | ||
| for r in old_spotdl_dirs() | ||
| for d in ["web-ui", "src" / "spotdl" / "web" / "static"] |
| return default | ||
|
|
||
|
|
||
| def elemIf(x: T, p: bool) -> T: |
| home = Path(os.environ['HOME']) | ||
| os.chdir(home) | ||
|
|
| home = Path(os.environ['HOME']) | ||
| os.chdir(home) | ||
|
|
|
Hrm. The copilot suggestions, especially the correctness suggestions, are correct and need a little evaluation... |
|
Not sure it is correct about the |
|
Let me know if you want these squashed before merge, BTW |
As pointed out by Copilot: - / is only supported for Paths, not strs, cast first to get access - The fallback logic for the config file path was branching based on whether the config _directory_ existed, not the config file. Co-Authored-By: Copilot
As pointed out by Copilot, the type hint is hiding the fact that it returns an iterable, not a raw value. Also fix style. Co-Authored-By: Copilot
Instead of calling os.chdir within the test itself, which might leak between tests. Co-Authored-By: Copilot
|
Sorry this took a while, I've been busy. |
Description
Use
platformdirsto compute the paths tospotdl's directories instead of rolling our own code here.Instead of reinventing the wheel, just use the standard apis here.
In fact, this was the approach introduced in 4f49e8e, and the platformdirs dependency has stuck around since.
This should have the added bonus of making our configuration more idiomatic in Windows and Mac.
Furthermore, make tests use temp dirs for their configuration, and in particular check for both idiomatic and non-idiomatic (backwards-compatible) configurations.
Finally, use this opportunity to split the directories used according to use (using XDG variables for reference):
XDG_CONFIG_HOME:config.jsonXDG_CACHE_HOME: the spotify and spotipy caches and the temp dirXDG_DATA_HOME: the ffmpeg binary and web-uiXDG_STATE_HOME: the web sessions and the logsAlso use the occasion to rename
.spotipy->spotipyand.spotify_cache->spotify_cache-- there's no need to doubly hide such directories, given the spotify directories are likely to be hidden anyway.API changes (grepped the codebase to make sure there are no remaining references, but these can be reverted if needed):
Remove:
spotdl.utils.config.get_spotdl_pathThis no longer makes sense, now spotdl's files aren't concentrated in a single directory
Rename:
spotdl.utils.config.get_cache_path->get_spotipy_client_cache_pathMade the stylistic choice to reserve the generic names
get_{config,cache,data,state}_pathfor the "roots" of those kinds of paths, and to otherwise name the functions after the directories' usecase. Reordered the functions accordingly.Avoided the obvious name of
get_spotipy_cache_pathto distance it from the existingget_spotify_cache_path.Add:
spotdl.utils.ffmpeg.get_local_ffmpeg_pathGives the path to where the local ffmpeg binary would be, regardless of whether it exists -- used for deduplicating logic in
spotdl.utils.ffmpeg, see below.Add:
tests.conftest.config_paths,tests.conftest.config_dirsconfig_pathsgives a single place where the list of all paths spotdl uses can be listed, along with their current default values.populate_configtakes a set of paths and populates them with the default config -- this is used in egtest_configto populate the various permutations of configurations. We write the default config JSON to those directories so spotdl has something to work with.config_dirssets up a temp dir configured for XDG with all the spotdl paths populated. This also permits cutting down on the use of monkeypatching in other tests -- seetest_init,test_ffmpegandtest_metadataDrive-by improvements:
spotdl.utils.ffmpegDeduplicate the logic in
get_local_ffmpeganddownload_ffmpegfor finding the path to the local ffmpeg.spotdl.utils.webUse more idiomatic path construction, and in particular use the
/operator instead of embedding/in strings passed to thePathconstructor (should be more reliable)Implementation notes:
spotdl.utils.config.get_path_fallbackThe current implementation of the file fallback picks its behaviours according to whether the parent directory exists. For our current purposes, this is sufficient (and necessary, given we want to signal when a config file is missing), but this does mean we forgo being able to move config files into subdirectories (eg
~/.spotdlrc->~/.spotdl/spotdlrc)spotdl.utils.config.old_spotdl_dirsIn order for this list to be affected by the monkeypatching to
os.environin tests, we need to wrap it in a function so it gets reevaluated every time. This way, it picks up the changes to the environment. A hack, but I see no better way around it.test_configTo be able to parametrize over the choice of fixtures,
request.getfixturevalueneeded to be used. This is slightly black magic, but it seems well-supported.Related Issue
Closes: #2058, #2387
Updates: #2514, #2540
Motivation and Context
This adds full XDG Base Directory support, as well as making our configuration idiomatic on Windows and Mac. This also future-proofs us if we intend to port
spotdlto other platforms -- I noteplatformdirsmentions support for BSD and Android.Moreover, this fixes the testsuite when running on XDG-configured systems.
How Has This Been Tested?
Used the
uv run pytest -vvvfrom.github/workflows/tests.ymlto sniff-test I didn't break anything. Am checking with CI to test non-linux platforms.My machine is an XDG-enabled Arch Linux box,
uvis running in a venv with CPython 3.12.11To check my API changes for breakage, I ran greps to find all occurrences of the functions removed/renamed and corrected them -- these should pose no issues
Types of Changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
I'm flagging this pre-emptively -- I have changed the interface of
spotdl.utils.configa little, but I don't know if we're guaranteeing stability of that interface to anyone except us?Checklist