Skip to content

cli: make config based args available during early setup #3255

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
merged 1 commit into from
Oct 17, 2020

Conversation

beardypig
Copy link
Member

@beardypig beardypig commented Oct 16, 2020

As arguments can be loaded from a config file these need to be loaded as soon as possible otherwise some options will not have the desired effect. For example, quiet can be specified in .streamlinkrc, however it will not take effect. With this change the default config args are loaded earlier, however plugin specific config files are still loaded later.

Options that will take effect from the config file now include:

  • loglevel **
  • json
  • stream_url
  • subprocess-cmdline
  • quiet
  • stdout
  • output
  • record-and-pipe
  • plugin-dirs

The plugin specific config option behaviour remains unchanged, and loglevel is the only option that is affected.

I do not think it is possible to sensibly load the plugin specific options early, there are too many opportunities for errors. I noticed this was not working as expected because plugin-dirs was not being set from the config file and is the primary reason for this fix.
setup_args is also called twice, and the way I see it setup_config_args should have been called twice as well, as setup_args only does half the job.

** loglevel is slightly different, it would take affect, but only after the initial setup had been completed. This means that if there were debug log messages during the setup they would not have been logged unless loglevel was set on the command line.

@bastimeyer
Copy link
Member

Sounds reasonable.
I'm afraid though that this is a breaking change and needs to be merged later.

Regarding tests, I don't think we have one for streamlink_cli.main.main, do we?

@beardypig
Copy link
Member Author

beardypig commented Oct 16, 2020

Is it a breaking change? It's broken right now... you cannot use some of the command line options from the config file, it was always the intention to be able to set them via config. loglevel and plugin-dirs are particularly useful ones. loglevel=debug in the config file will only take affect once the plugins have been loaded, in contrast to setting it via the command line -l debug where it will show the log messages when loading plugins. This should have always worked.

Regarding tests, I don't think we have one for streamlink_cli.main.main, do we?

We do have some tests, but nothing that covers config loading.

@back-to
Copy link
Collaborator

back-to commented Oct 16, 2020

won't work if you use plugin commands with "first available default config"
which will break streamlink

usage: streamlink [OPTIONS] <URL> [STREAM]streamlink:
error: unrecognized arguments: --twitch-disable-ads --twitch-low-latency ...

# Only load first available default config
for config_file in filter(os.path.isfile, CONFIG_FILES):
config_files.append(config_file)
break

As arguments can be loaded from a config file these need to be loaded as
soon as possible otherwise some options will not have the desired
effect. For example, `quiet` can be specified in `.streamlinkrc`,
however it will not take effect. With this change the default config
args are loaded earlier, however plugin specific config files are
still loaded later.

Options that will take effect from the config file now include:
 - `loglevel`
 - `json`
 - `stream_url`
 - `subprocess-cmdline`
 - `quiet`
 - `stdout`
 - `output`
 - `record-and-pipe`
 - `plugins-dir`

The plugin specific config option behaviour remains unchanged, and
`loglevel` is the only option that is affected.
@beardypig beardypig force-pushed the config-loading-order branch from ce8e4e2 to 55788af Compare October 16, 2020 14:58
@beardypig
Copy link
Member Author

@back-to easily fixed.

@gravyboat gravyboat mentioned this pull request Oct 16, 2020
@bastimeyer bastimeyer changed the title cli: ensure that config based args are available during early setup cli: make config based args available during early setup Oct 17, 2020
@bastimeyer bastimeyer merged commit 3ded794 into streamlink:master Oct 17, 2020
resiproxy pushed a commit to resiproxy/streamlink that referenced this pull request Nov 5, 2020
…3255)

As arguments can be loaded from a config file these need to be loaded as
soon as possible otherwise some options will not have the desired
effect. For example, `quiet` can be specified in `.streamlinkrc`,
however it will not take effect. With this change the default config
args are loaded earlier, however plugin specific config files are
still loaded later.

Options that will take effect from the config file now include:
 - `loglevel`
 - `json`
 - `stream_url`
 - `subprocess-cmdline`
 - `quiet`
 - `stdout`
 - `output`
 - `record-and-pipe`
 - `plugins-dir`

The plugin specific config option behaviour remains unchanged, and
`loglevel` is the only option that is affected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants