eonpatapon / mpDris2

MPRIS V2.1 support for mpd
GNU General Public License v3.0
210 stars 46 forks source link

configs: Give config file priority over envvars #137

Closed ferdnyc closed 3 years ago

ferdnyc commented 3 years ago

The previous configuration code gave the MPD_HOST and MPD_PORT environment variables priority over mpDris2's own config file, when it comes to settings. I feel that's backwards, especially for a daemon that runs with the user's environment, but not interactively — whatever's set in its config file should trump the generic MPD environment variables, which make more sense as a last-resort fallback.

Also, because there's potential for the user to set a password (but not host) in their config file, but then have MPD_HOST set to a password@host string, the code will let the MPD_HOST password override the one from the config file (iff the host part is being used, because it was not set by other means) — but it'll log a warning about it, to let the user know that the settings conflict caused some of their configs to be ignored/overridden.

smcv commented 3 years ago

The previous configuration code gave the MPD_HOST and MPD_PORT environment variables priority over mpDris2's own config file, when it comes to settings. I feel that's backwards

This is consistent with most programs that have configuration files. The order of precedence is normally: built-in defaults < configuration file < environment variables < command-line options.

Is there a situation where the environment variables being taken as highest-precedence are a concrete problem?

(I am not a maintainer of mpdris2, but I do maintain its package in Debian.)

eonpatapon commented 3 years ago

I also don't see the problem of env variables having highest precedence, it is a standard behaviour

ferdnyc commented 3 years ago

The previous configuration code gave the MPD_HOST and MPD_PORT environment variables priority over mpDris2's own config file, when it comes to settings. I feel that's backwards

This is consistent with most programs that have configuration files. The order of precedence is normally: built-in defaults < configuration file < environment variables < command-line options.

Is there a situation where the environment variables being taken as highest-precedence are a concrete problem?

In my case, yes. I have environment variables set in my user environment so they're applied the same way by any MPD client I might decide to use interactively, whether it be mpc, ncmpc, qmpdclient, the old copy of gmpc I still have installed (assuming it works, which isn't likely)... what have you.

But when mpDris2 runs non-interactively as a shell extension, I would like to give it a different configuration than the one those (generic, client-agnostic) environment variables contain.

I guess that's the primary difference. With most programs that support envvar configuration, the only reason for those variables to be set is if they're being used to configure the software. That's not the case here, as the variables are in no way specific to mpDris2, and there are plenty of other reasons they might be set.

grawity commented 3 years ago

But mpDris2 is not a shell extension, it's a standalone background service. You can specify custom environment variables in its systemd .service unit, or whichever other method you use to start it.

ferdnyc commented 3 years ago

But mpDris2 is not a shell extension, it's a standalone background service.

You're right, sorry. Slip of the brain.

You can specify custom environment variables in its systemd .service unit, or whichever other method you use to start it.

No argument here, there are other ways to address this, and I will take one of those. Withdrawn.

I still don't really agree, though. Let me try one more analogy, via the Git configuration variable core.pager: Quoting the git-config(1) man page:

       core.pager
           Text viewer for use by Git commands (e.g., less). The value is
           meant to be interpreted by the shell. The order of preference is
           the $GIT_PAGER environment variable, then core.pager configuration,
           then $PAGER, and then the default chosen at compile time (usually
           less).

Because of their multi-client nature, I view MPD_HOST and MPD_PORT more like the PAGER envvar — a generic fallback. They're not the same as a targeted, single-purpose variable like GIT_PAGER.

I'd certainly have no objections if there were MPDRIS2_HOST and MPDRIS2_PORT variables with a higher precedence than the config file, because there's no reason for those variable to be set other than to override the configuration.