Add support for config file #75
Conversation
|
Nice. What should the syntax be for a command to determine the output instead of a fixed output string? If the config had both, which would take precedence? I don't think it is important to implement arbitrary output command handling now, but i think we should have a plan for the syntax and behavior. |
I think that depends, if we want to have a set of hardcoded default choosers or not. If we have one, we need a way to hard override the chooser with a given monitor. If you could disable all choosers (like adding an empty chooser entry) it would be irrelevant, if you first skip the choosers and then take the set monitor or choose the set monitor in the first place. So i would let the hardcoded output be priority.
For an arbitrary output chooser, we would just need two options in the file. chooser_cmd and chooser_type. Those would override the hardcoded ones. Question is, if we want to fail screencast, if no monitor is selected, or if we still pick the "first" one? I could rebase #59 onto this MR. |
I was thinking of landing this one first, then going back to #59 and just adding the needed config entries. My initial thoughts for config would be chooser_type = <first | fixed | command>
chooser_cmd = <string representation of command to run, should accept list of outputs on stdin and output the choice on stdout, ignored for all types but command>
chooser_value = <string representation of output string, ignores for all types other than fixed>I wonder if we should define an ini section for these settings. I think it makes sense to fall back to |
|
I added my thought on how to config the chooser in #59. |
include/config.h
Outdated
|
|
||
| #include "logger.h" | ||
|
|
||
| struct config_general { |
There was a problem hiding this comment.
Do we really need that many structs? Can't we just have a single struct with two fields?
There was a problem hiding this comment.
No, it's more like a personal preference, to have one struct for each portal to create some kind of namespace separation. Currently all options could be in one struct.
|
@emersion |
include/config.h
Outdated
| }; | ||
|
|
||
| struct config_logger { | ||
| enum LOGLEVEL loglevel; |
There was a problem hiding this comment.
I wonder, is there a use-case for setting the log level in the config file? The logging level mostly exists for debugging purposes, which means manually starting xdpw.
For instance, Sway doesn't have an option to set it from the config file.
There was a problem hiding this comment.
My idea was to put every option we have into one central config structure. If we want, we can read it from the configfile, of not, we wont.
There was a problem hiding this comment.
I don't think that's worthwhile if that's just adding code for no real purpose.
There was a problem hiding this comment.
I kind of like this because starting xdpw manually can be a pain. This would change verbosity in journald if it is started via dbus activation as a systemd user service.
This would mean people running master could keep log verbosity up all the time without overriding the service file.
There was a problem hiding this comment.
I'm not sure I understand the use-case here.
You almost never want DEBUG/TRACE levels when running with D-Bus activation, because that'd fill up your hard drive. When running on master, you likely don't have the version you compiled installed, otherwise it would conflict with the one from your package manager. Even if you want DEBUG/TRACE and you have master installed to /usr, then you'd need to edit your config file, restart xdpw (how?), reproduce the bug, then use journalctl to watch/collect the logs, then revert the config file change.
It sounds a lot easier to just run xdpw from the build directory, with the log level flag, redirecting the output to a file.
There was a problem hiding this comment.
@emersion I think the problem is that when we ask people in issues to give us logs, they often get confused by the process of killing all instances of xdp/xdpw and starting both from the command line in such a way that the right instances are picked up and used. They're not often building from source. I've seen people suggesting overriding the systemd units just to get logs. If you're developing xdpw, running from the command line is a must, but for regular users, this is confusing.
I think your recent patch to add the -r flag will help a lot with this and may invalidate my concerns. Maybe, in addition to this, improved documentation would help in the FAQ or README.
Even when I'm testing a PR, I typically hack a PKGBUILD together that points to another fork so that I can rely on d-bus activation for a few days to test out the stability of the changes. It is nice to have logging up a little (not TRACE level, but DEBUG isn't too bad). This helps for more pervasive issues that are harder to catch other than by regular use.
The more I write this out, the more I think wanting a config file option for this is the exception and not the rule. Overriding the systemd unit may not be such a bad thing to have to do in the exceptional case.
There was a problem hiding this comment.
I think adding the -r flag will make this issue invalid, since the default loglevel is error, so the log will hopefully contain the reason of failure and for a more detailed log the installed version of xdpw can be used with --reload -l DEBUG/TRACE.
The only usecase left would be people which want reduce the loglevel to QUIET.
But i would guess, that that's not a very useful or often used choice.
include/config.h
Outdated
| #include "logger.h" | ||
|
|
||
| struct config_general { | ||
| char *configfile; |
There was a problem hiding this comment.
Do we really need this in the config file struct?
There was a problem hiding this comment.
Same idea, it's one option, which will only be set by commandline parameter (or maybe environment variable). This should (can) not be read from the configfile, but it also serves as a placeholder for general settings, which might/will come later.
Also it's nice for debugging, if you can dump the state/all settings of xdpw.
There was a problem hiding this comment.
I'd rather have this as a separate variable.
There was a problem hiding this comment.
ok also regarding the loglevel: the config should only contain configs for each portal?
There was a problem hiding this comment.
i would remove the loglevel from config and the configfile setting with it's corresponding structs, so only the output_name for the screecast struct would stay
There was a problem hiding this comment.
Yeah, that sounds good to me. We'll add more config options as we go.
There was a problem hiding this comment.
(Side note, it's always easier to add new config options, than to remove existing ones. Removing config options means we break user configurations.)
|
As a user testing this out, my primary gripe with this PR is that it needs documentation. I had to run xdpw with tracelogging on to figure out what files it actually tries to load. :/ A short manual page would be ideal. Also, could reasonable defaults be hardcoded into the system? Like, if there's something to choose, use slurp automatically unless configured otherwise, and if it fails, default to picking the first display? I generally prefer software to do something sane without having to configure it. |
|
@oranenj Added a note about the locations at the description of this PR. Documentation will either follow in the wiki or with #67 as soon as this is merged. Thanks for pointing that out. I didn't realize, that those MR's are widely used. I'm also preferring sane defaults and if you don't have a config file the output chooser should try 3 different choosers (I admit, we could differentiate if the screencast was declined, or if the chooser was not installed), but please feel free to suggest your idea at #59. |
|
Re: documentation, man pages are planned. I've been working on them for what feels like a decade, but I keep getting side tracked. They'll come out soon and we will update them with this PR if they land first, and they will include this PR if they land second. Also, this PR will likely get merged with the output selection PR, so both will need to be ready before either is stabilized. In the future, updating docs should be a criteria for accepted PRs, but I doubt docs will ever get written before something is ready to be merged. I like sane defaults as well, but I'm not a big fan of hardcoding the names of other programs into a codebase. We've already got a hard dependency on grim (for screenshots), so slurp doesn't feel like a big stretch. I'm very wary of adding any others, but I might be in the minority on this one. I think we will always gracefully fall back to the first display and have a CLI flag that takes precedence over any chooser, just in case. |
|
@danshick I totally agree with you on dependencies. In this case though, slurp wouldn't be as much a dependency as an extremely convenient integration, and since it's sort of part of the "wlroots ecosystem" it feels justified. If you don't feel like hardcoding it in C, one option might be to ship a "default configuration" in /usr/share somewhere that's not meant to be edited (and will be overwritten by package managers) which will be used as a last resort. |
I like this idea a lot. Also lets different distros ship a custom default config if they so choose. What does everyone else think? @emersion @columbarius? |
|
Depends... if we make slurp (with the specific commit) a hard dependency, we could ship a default config in /user/share with an slurp entry. If not, we would ship an almost empty config file such that xdpw falls back to the default path. That way, I would not ship a config, but try to create the most sane default possible and rely on the distribution to ship something in etc reflecting their choice. |
|
I might be a good idea to nudge distributions towards using the defaults from upstream. Otherwise, you might end up having to support users using very different configurations just because distros happen to pick whatever they think is best at the moment of packaging if upstream has no opinion on the matter. |
|
Since we are writing stuff for a library which could be used to write window managers and desktop environments i don't think there is something like a good upstream default, since we can assume very little as preinstalled. With a config file, we can only support one output chooser, while currently we have three assumptions as default, if the config has no configured chooser.
|
src/core/config.c
Outdated
|
|
||
| void init_config(char *configfile, struct xdpw_config *config) { | ||
| if (configfile == NULL) { | ||
| configfile = get_config_path(); |
There was a problem hiding this comment.
This memory is never free'd. This wouldn't be the biggest memory leak in xdpw, that said.
There was a problem hiding this comment.
Shouldn't this memory be freed here at the end of main?
Well better not make that a contest ^^'
There was a problem hiding this comment.
This assignment only changes the local configfile variable. The variable in main is unaffected.
There was a problem hiding this comment.
damn, yeah screwed that up. Should work now.
src/core/config.c
Outdated
|
|
||
| void init_config(char *configfile, struct xdpw_config *config) { | ||
| if (configfile == NULL) { | ||
| configfile = get_config_path(); |
There was a problem hiding this comment.
Maybe we should check configfile != NULL at this point. get_config_path can return NULL.
There was a problem hiding this comment.
Yeah, but afaics libiniparser could handle NULL as a path to the config file and will insert the defaults as if this file is empty. If you want to avoid this, I could write another function setting the default values manually.
There was a problem hiding this comment.
Hm, looking at the code, it doesn't seem like it?
There was a problem hiding this comment.
(IMHO just set the defaults in main when initializing the config variable)
There was a problem hiding this comment.
It happens here https://github.com/ndevilla/iniparser/blob/f858275f7f307eecba84c2f5429483f9f28007f8/src/iniparser.c#L419
About defaulting in main is, i can't detect if something is a default or was set via commandline parameter.
src/core/config.c
Outdated
| } | ||
|
|
||
| static void config_parse_file(const char *configfile, struct xdpw_config *config) { | ||
| dictionary *d = iniparser_load(configfile); |
There was a problem hiding this comment.
We should check for a NULL d here, in case iniparser_load failed.
There was a problem hiding this comment.
Same here, if d is NULL iniparser_get* will just return the fallback.
There was a problem hiding this comment.
OK, but if iniparser failed to read the file we want a proper error message
There was a problem hiding this comment.
Added logging, but don't know how to supress the log from iniparser. We could check before and skip it when the config file is null, or just have the line in the log.
There was a problem hiding this comment.
There is the line
iniparser: cannot open (NIL) or path to file
in stderr if iniparser_load fails.
There was a problem hiding this comment.
Maybe don't call iniparser_load if configfile == NULL?
There was a problem hiding this comment.
I moved all checks of configfile to the part where we load it with iniparser_load, so we don't have to check for the same things multiple times.
src/core/main.c
Outdated
| @@ -217,6 +226,8 @@ int main(int argc, char *argv[]) { | |||
| } | |||
|
|
|||
| // TODO: cleanup | |||
| finish_config(&config); | |||
| free((char *)configfile); | |||
There was a problem hiding this comment.
Something that we need to free can't be const
There was a problem hiding this comment.
Hope i got the const and not const declaration right this time.
|
Thanks for the squash and merge! |
Resolves #60
Note: while this is not merged and man pages are not ready, the locations for a config file are in order:
The system wide config can be in another place, if SYSCONFDIR differs from /etc