Skip to content

cli: update config file paths and plugin dirs on all OSes #3766

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

Merged

Conversation

bastimeyer
Copy link
Member

@bastimeyer bastimeyer commented Jun 1, 2021

According to the XDG base directory specification, only configuration files are supposed to be in the XDG_CONFIG_HOME directory. This means that Streamlink's custom plugin files directory should default to XDG_DATA_HOME and not to XDG_CONFIG_HOME, as plugin files are not configuration files.

  • The first commit therefore refactors the CLI's path constants and enables having a list of default plugin dirs.
  • The second commit adds XDG_DATA_HOME to the PLUGIN_DIRS on POSIX in addition to XDG_CONFIG_HOME for backwards compatibility.
  • I've also reformatted the CLI docs a bit for better readability (check the preview).

This also raises the question whether we should add %APPDATA%\streamlink\config to the CONFIG_FILES list on Windows, in addition to %APPDATA%\streamlink\streamlinkrc. The current config file name comes from the time when Livestreamer was still using the ~/.livestreamerrc config file, which is deprecated anyway (not officially flagged as deprecated, but we could do that too). Yes, these are technically "run command files", hence the "rc" suffix, but the primary/intended config file name on non-Windows systems is already "config" and this helps solving confusion, especially for the average Windows user.

Also not really sure about macOS. It has been treated as non-Windows the whole time (of course), but the XDG base dir spec doesn't really apply to it and they have other conventions (see the new log dir for example). This could also be corrected in another commit. Config path and plugin dir should be ${HOME}/Library/Application Support/streamlink/config and ${HOME}/Library/Application Support/streamlink/plugins respectively if I'm not mistaken.


Question 1:

Add %APPDATA%\streamlink\config in addition to %APPDATA%\streamlink\streamlinkrc (and update config file path in the installer)?

Question 2:

Use correct config and plugin paths on macOS?

Question 3:

Document ~/.streamlinkrc as deprecated in the docs and remove it in the next major release 3.0.0?

@bastimeyer bastimeyer added the CLI label Jun 1, 2021
@bastimeyer bastimeyer force-pushed the cli/config-files-and-plugin-dir branch from 5fccab6 to d37e4e5 Compare June 1, 2021 17:01
@gravyboat
Copy link
Member

Changes look good, in regards to your questions:

  1. Yes.
  2. Yes.
  3. Yes.

@bastimeyer bastimeyer force-pushed the cli/config-files-and-plugin-dir branch from d37e4e5 to fb7814e Compare June 5, 2021 21:01
@bastimeyer bastimeyer changed the title cli: add XDG_DATA_HOME as first plugins dir cli: update config file paths and plugin dirs on all OSes Jun 5, 2021
@bastimeyer
Copy link
Member Author

bastimeyer commented Jun 5, 2021

Summary

  1. Linux/BSD
    Add ${XDG_DATA_HOME:-${HOME}/.local/share}/streamlink/plugins as first plugins dir,
    but keep ${XDG_CONFIG_HOME:-${HOME}/.config}/streamlink/plugins as secondary plugins dir.
  2. Windows
    Add %APPDATA%\streamlink\config as first config file path,
    but keep %APPDATA%\streamlink\streamlinkrc as secondary config file path.
  3. macOS
    • Add ${HOME}/Library/Application Support/streamlink/config as first config file path,
      but keep ${XDG_CONFIG_HOME:-${HOME}/.config}/streamlink/config as secondary
      and ${HOME}/.streamlinkrc as third config file paths.
    • Add ${HOME}/Library/Application Support/streamlink/plugins as first plugins dir,
      but keep ${XDG_CONFIG_HOME:-${HOME}/.config}/streamlink/plugins as secondary plugins dir.

edit: deprecate in a separate PR after merging this one

Officially deprecate

  • Windows
    • %APPDATA%\streamlink\streamlinkrc
  • macOS
    • ${XDG_CONFIG_HOME:-${HOME}/.config}/streamlink/config
    • ${HOME}/.streamlinkrc
    • ${XDG_CONFIG_HOME:-${HOME}/.config}/streamlink/plugins
  • Linux/BSD
    • ${HOME}/.streamlinkrc
    • ${XDG_CONFIG_HOME:-${HOME}/.config}/streamlink/plugins

@bastimeyer
Copy link
Member Author

Not sure if the OSes should be properly split up in the docs. I've added macOS as another row to the three tables, but kept the "Unix-like (POSIX)" rows, which implies that macOS is included. This avoids having redundant paths, but also doesn't show that the regular POSIX paths are deprecated on macOS. So should the "Unix-like (POSIX)" rows be renamed to "Linux/BSD" and those paths added to the macOS rows (as deprecated)?

@gravyboat
Copy link
Member

So should the "Unix-like (POSIX)" rows be renamed to "Linux/BSD" and those paths added to the macOS rows (as deprecated)?

I'd say this is a good plan. It clarifies things further and leads to less confusion which is always a plus.

- use pathlib.Path instead of strings
- rename PLUGINS_DIR to PLUGIN_DIRS and turn into list
- call expanduser() for all `--plugin-dirs` dirs
- reformat CLI docs
- fix tests
Keep XDG_CONFIG_HOME for backwards compatibility
but keep "streamlinkrc" as secondary file for backwards compatiblity
Keep old paths for backwards compatiblity.
Also fix logs dir (see --logfile).

New default config path:
~/Library/Application Support/streamlink/config

New default plugins dir:
~/Library/Application Support/streamlink/plugins
@bastimeyer bastimeyer force-pushed the cli/config-files-and-plugin-dir branch from fb7814e to ce06efd Compare June 6, 2021 00:00
@bastimeyer
Copy link
Member Author

bastimeyer commented Jun 6, 2021

Updated the docs (with additional stylesheet updates) and properly split up the path definitions between macOS and Linux/BSD, which also means that the new XDG_DATA_HOME plugins dir is not part of the macOS plugins dir list, only the old/deprecated XDG_CONFIG_HOME one. I've updated my previous comment accordingly.

@bastimeyer
Copy link
Member Author

Do we need deprecation log messages when loading stuff from those paths?

@gravyboat
Copy link
Member

gravyboat commented Jun 6, 2021

@bastimeyer I feel like it's overkill but I've also learned people don't read the release notes and like to complain later. Will people read the CLI output noting as much? I'm not sure. If it's not too much work it's probably worth adding so we can point at release notes and CLI output when someone inevitably complains.

@bastimeyer bastimeyer force-pushed the cli/config-files-and-plugin-dir branch from ce06efd to 63fe719 Compare June 6, 2021 13:09
@bastimeyer
Copy link
Member Author

bastimeyer commented Jun 6, 2021

I've added the deprecation log messages. See my updated commit and its message.

  • For some reason, logging a warning message still prints the message even while in json output mode, where loglevel should be none. That's why it's logging to info instead.
  • It only logs deprecated plugin paths when at least one plugin was successfully loaded. Load failures are being logged separately in all cases.
  • No tests, as there weren't any tests for load_plugins, setup_args and setup_config_args prior to my changes and I don't want to write them. The changes are simple enough though.
  • There's some optimization that can be done in the setup_args and setup_config_args. This can be done later and tests added as well.

@bastimeyer
Copy link
Member Author

If you want, we can deprecate these paths in another PR. That would probably be better for transparency reasons.

@bastimeyer bastimeyer force-pushed the cli/config-files-and-plugin-dir branch from 63fe719 to 7ec8a89 Compare June 6, 2021 14:56
@gravyboat
Copy link
Member

gravyboat commented Jun 6, 2021

@bastimeyer Should the deprecation message include information about where the new config directory is? Seems like just telling people the dir is deprecated without saying where to move things to might lead to some confusion.

If you want, we can deprecate these paths in another PR. That would probably be better for transparency reasons.

Agreed.

@bastimeyer
Copy link
Member Author

I'll add a docs-reference to the log messages in the deprecation PR once this one has been merged. As you can see, I removed the deprecation commit, so this PR here should be good unless someone spots a mistake.

@bastimeyer bastimeyer added the PR: rebase commits Commits can be included as is on the base branch via a rebase (will alter SHAs) label Jun 6, 2021
@gravyboat gravyboat merged commit 565397e into streamlink:master Jun 6, 2021
@bastimeyer bastimeyer deleted the cli/config-files-and-plugin-dir branch June 6, 2021 18:50
404NetworkError added a commit to 404NetworkError/scoop-extras that referenced this pull request Jul 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI PR: rebase commits Commits can be included as is on the base branch via a rebase (will alter SHAs)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants