jamulussoftware / jamulus

Jamulus enables musicians to perform real-time jam sessions over the internet.
https://jamulus.io
Other
1k stars 224 forks source link

Interaction between command line arguments and ini file #555

Open corrados opened 4 years ago

corrados commented 4 years ago

For me, the headless Jamulus server on the Linux command line should behave like any other GNU tool. You configure it with command line arguments and that's it. No hidden ini file changes the settings.

The situation is different if I run the client/server with a GUI on, e.g., Windows. For that operation my expectation is that the software stores/recovers my settings I set in the GUI.

pljones and I have a different opinion on that. See the discussion here https://github.com/corrados/jamulus/issues/370#issuecomment-683264655 and here https://github.com/corrados/jamulus/issues/370#issuecomment-683267114 and here https://github.com/corrados/jamulus/issues/370#issuecomment-683275779.

I think we need some more opinions on that matter. Please discuss the expected behaviour in this Issue.

WolfganP commented 4 years ago

My take is similar to the scattered comments around:

A complement to certain jamulus/system announcements may be the use of the chat window to push for important notifications (ie server outdated)

pljones commented 4 years ago

Just to be clear, the scenarios being consider have the following use cases (this describes my understanding of the current intended behaviour):

The first two cases I have no issue with.

However, to me, regardless of whether or not I'm using a GUI, if I've loaded some settings from an ini file and some settings from the command line, the command line settings should not -- automatically -- overwrite what's in the ini file.

Either those fields in the GUI need to be highlighted and a specific action to confirm the value taken (i.e. manually change to something else or some "confirm" action to remove the highlight) -- or the field would be better blocked from editing.

corrados commented 4 years ago

this describes my understanding of the current intended behaviour [...] If I run with an ini file, but without a gui, settings should not be written. (Load an inifile with Central Server Jazz, pass -e Rock, the inifile stays as Jazz.)

No, this is not the intended behaviour. If a settings file is given, the command line arguments always overwrite the settings in the ini file and also the command line parameter is stored in that ini file.

However, to me, regardless of whether or not I'm using a GUI, if I've loaded some settings from an ini file and some settings from the command line, the command line settings should not -- automatically -- overwrite what's in the ini file.

Hm, we had this discussion before. I thought we agreed that we apply the simple rule that command line arguments always overwrite ini settings and that command line arguments are stored in the ini file.

Either those fields in the GUI need to be highlighted and a specific action to confirm the value taken (i.e. manually change to something else or some "confirm" action to remove the highlight) -- or the field would be better blocked from editing.

I would not like such a GUI behaviour. If I specify a value on the command line, I want it to be applied to the GUI directly. That is the intention of the command line argument. No need to confirm this.

pljones commented 4 years ago

If I run with an ini file, but without a gui, settings should not be written. (Load an inifile with Central Server Jazz, pass -e Rock, the inifile stays as Jazz.)

No, this is not the intended behaviour. If a settings file is given, the command line arguments always overwrite the settings in the ini file and also the command line parameter is stored in that ini file.

That's not what I've written says.

I said: if I pass a command line setting, it overrides any value in any inifile passed (or defaulted).

pljones commented 4 years ago

If I specify a value on the command line, I want it to be applied to the GUI directly. That is the intention of the command line argument. No need to confirm this.

That's not what I've written says. The value would be visible in the GUI. It would be highlighted, in fact. Quite present, quite visible, quite effective. No need to "confirm" anything to get the behaviour requested from the command line setting.

corrados commented 4 years ago

If I run with an ini file, but without a gui, settings should not be written. (Load an inifile with Central Server Jazz, pass -e Rock, the inifile stays as Jazz.)

No, this is not the intended behaviour. If a settings file is given, the command line arguments always overwrite the settings in the ini file and also the command line parameter is stored in that ini file.

That's not what I've written says. I said: if I pass a command line setting, it overrides any value in any inifile passed (or defaulted).

What I wanted to say in my comment is: You write: "If I run with an ini file [...] settings should not be written" -> If a settings file is given, the current settings should be stored in that file. But you write that it should not.

corrados commented 4 years ago

If I specify a value on the command line, I want it to be applied to the GUI directly. That is the intention of the command line argument. No need to confirm this.

That's not what I've written says. The value would be visible in the GUI. It would be highlighted, in fact. Quite present, quite visible, quite effective. No need to "confirm" anything to get the behaviour requested from the command line setting.

I was referring to your comment: "some "confirm" action to remove the highlight". Ok, I think I misunderstood that sentence. You say to remove the highlight but I interpreted it that the parameter is only applied when confirmed.

Anyway, I do not see a need for a highlight. I just makes the code more complex with little benefit. Do you know of any user who complained that a command line setting has overwritten his inifile setting?

WolfganP commented 4 years ago

I think the agreement is that CLI arguments have the highest precedence. That should be clearly stated in the wiki's reference page for advanced users to take into account (https://github.com/corrados/jamulus/wiki/Command-Line-Options makes no mention of priorities/precedence as of now).

I don't see the need to complicate the code and highlight conflicting values in GUI mode, as we should assume the user who customized the command line invocation has some technical knowledge and read the wiki to know cause and effect of his/her customization (or at least know that messing with configs around w/o understanding is not a good thing :-) )

pljones commented 4 years ago

Based on Volker's clarifications:

I'll skip the "with gui" ones - basically, we write back always for them. It's here I'll highlight a concern. Would someone running headless using a command line that specified an inifile expect that file to get updated from a command line parameter? I'd say not.

Alternatively, headless should work the same as with the gui -- use the default inifile or the specified inifile and write back. (Essentially, this is the change I'd made in the other thread.)

Not some inconsistent half-way house, sometimes writing back and sometimes not.

Maybe I use the GUI to edit the welcome message. I don't specify an inifile, so the default inifile is edited. Now I run my server headless. Does it pick up my message? Hm, no. Oh, I suddenly must supply the inifile name or it's ignored. OK. I pass the inifile. But my server doesn't get registered as default central is full. Fine, I'll try Rock. No, that's no good as I'm not running that specific a server. I'll drop the override Rock. But now it keeps trying to use Rock. Weird. Even weirder if the user had tried switching before specifying the inifile as they'll have not had the same behaviour.

pljones commented 4 years ago

Let's extend the above. To use the GUI, I run a remote X session. I can't do that from my mobile phone too well but I've got ssh okay.

So I've worked on my welcome message. It's quite extensive. Shows various bits of useful information on how best to get things going and so forth. I'm happy with it and start up the server.

A couple of days later, reading Facebook from my phone, I find I've put the wrong url for a link and need to stop it showing - but I'm away from my computer. Still, I login via ssh and I quickly append -w '' to the command line settings. Simple.

Back at my computer, I just want to correct that URL and let the message display again.

But based on the above, that -w '' was destructive -- it deleted the entire welcome message from the inifile! I have to start all over again, my creative juices wasted.

pljones commented 4 years ago

Another example. An SMTP server. It's headless. Let's say v0.0.1 has no configuration file support. It'll start up, defaults to listening on port 23 and dumps all mail received into a dead letter basket. It takes three command line settings: port, local domain and outbound server. If you pass local domain, it now knows to check the local user names for matches when the host name of the incoming email matches and deliver to that user's mailbox. Everything else to dead letter, still... Outbound domain lets you say where to send emails received from the local domain when the to field isn't in the local domain. Simple mail transport at its best. Port, of course, lets you override the port.

So this is simple. But life isn't. I want subdomains. I've multiple local servers on my network and want to handle that. Suddenly it's all too much for the command line settings. I introduce a config file. Of course, you don't need to use it if your life is simple.

Now, I can pick from a few things:

... or ... I can allow a command line setting to specify a config file. If it's not specified, I can check a default location for a config file. And if I've still not got one, revert to the "simple world" behaviour.

That last one seems most flexible. It also seems to me the most common approach used in servers I've seen (although many now are so complex the "simple world" behaviour isn't an option).

OK, so I either have or have not got a config file. I go off and read it. But what if I've got more command line settings?

Well, "obviously" they take priority over the content of the file.

Let's say I've got my live SMTP server running on port 23. I want to run a few tests with pretty much the same config but a different outbound server. Easy - specify the existing live config file and override the outbound server. Hm. And the port, otherwise it'll clash. Sorted.

Right? Would you expect the inifile to be updated? I wouldn't.

corrados commented 4 years ago

Well, both of your examples are fine with the current implementation. I just checked: For the current headless server (i.e. nogui) there is no settings support implemented at all. I.e. even the --inifile does not work with the current code. That makes things easy, since no setting file is involved, just command line arguments.

You may argue that the inifile makes sense for extensive welcome messages. But there is a "hidden feature" which we may make more visible to the Jamulus users: https://github.com/corrados/jamulus/blob/master/src/server.cpp#L397. You can use a file instead of the welcome message. E.g.: ./Jamulus -s -n -w /home/myuser/jamuluswelcomemessage

WolfganP commented 4 years ago

You may argue that the inifile makes sense for extensive welcome messages. But there is a "hidden feature" which we may make more visible to the Jamulus users: https://github.com/corrados/jamulus/blob/master/src/server.cpp#L397. You can use a file instead of the welcome message. E.g.: ./Jamulus -s -n -w /home/myuser/jamuluswelcomemessage

Just added to the cli wiki page.

pljones commented 4 years ago

I said:

Based on Volker's clarifications:

  • If I run without an ini file, and without a gui, settings should not be written. (Pass -e Rock, no inifile updates.)
  • If I run with an ini file, but without a gui, settings should be written. (Load an inifile with Central Server Default, pass -e Rock, the inifile gets update to Rock.)

Response:

For the current headless server (i.e. nogui) there is no settings support implemented at all. I.e. even the --inifile does not work with the current code.

OK, so what you're saying is that it doesn't work the way you'd previously said it worked now you come to try it out.

I'd have said that means it's confusing even to you -- or it's been broken at some point (possibly unintentionally when the HEADLESS support went in).

But in that case, it does become consistent. We get: with GUI, uses inifile always; without does not use inifile ever. That's simple enough.

pljones commented 4 years ago

I'm happy for this to be closed now the help has been updated.

ann0see commented 2 years ago

Re opening to allow discussion for @pgScorpio

pgScorpio commented 2 years ago

Re opening to allow discussion for @pgScorpio

Thanks @ann0see, I didn't notice this issue until now. (Since this subject is discussed in multiple places we maybe better start a real discussion on Settings and ini file handling?)

As I mentioned before in other issues/PR's I'm also working on a new implementation for settings and commandline parameter handling. And, as I see It, there would be no propper reason why a headless build should not be able to use an ini file. (Maybe only if an --inifile parameter is given ?)

Regarding commandline parameters overwriting ini file settings I think a commandline parameter should overide the ini setting not overwrite. So if a commandline parameter is given that value will be used for that session, but the value written to the ini file will not be changed, so when running the next time without the commandline parameter the original ini file value will be used again. When running in GUI mode and a commandline parameter is given the commandline parameter will also override the ini file value, until the value is changed via the GUI. In that case the ini file value will be changed and the commandline parameter no longer overrides.

The complete Settings/Commandline parameter handling overhaul I'm working on would already allow this, and it also allows to store commandline parameters (See also here ), even those that who's values are normally not stored in the ini file. (ipv6, clientname, etc...) These stored commandline parameters will behave as if given on the commandline, so also not changing any normal ini file values. The new implementation will also allow to have a sectioned ini file that could contain client settings as wel as server settings and also could have separate sections under "client" and "server" for i.e. user profile, fader settings and audio device settings, making the ini file much more structured.

pljones commented 2 years ago

https://github.com/pljones/jamulus/tree/feature/next-big-thing-for-settings https://github.com/pljones/jamulus/blob/feature/next-big-thing-for-settings/src/options.h https://github.com/pljones/jamulus/blob/feature/next-big-thing-for-settings/src/options.cpp

pgScorpio commented 2 years ago

@pljones

https://github.com/pljones/jamulus/tree/feature/next-big-thing-for-settings/src https://github.com/pljones/jamulus/blob/feature/next-big-thing-for-settings/src/options.h https://github.com/pljones/jamulus/blob/feature/next-big-thing-for-settings/src/options.cpp

I'm aware of your work on settings and commandline options, and I already asked you multiple times to work on this together (until now, unfortunately without any reply). I'm afraid you're still going in the wrong direction... Did you already have a look at my solution ? cmdlnoptions.h cmdlnoptions.cpp settings.h settings.cpp Though it's a similar approach, I think it's more straight forward, more versatile, faster and easier to maintain.

I would also appreciate the opinions of others on these two approaches.

pljones commented 1 year ago

There's two main things I don't like:

My "options" approach deals with both of those. Each option provides its own validation (based on some "built in" methods and allowing where something really special should happen, e.g. --ctrlmidich). Each option will also get stored in any inifile, with the command line always overriding for that execution (i.e. not persisting -- though I think I hadn't quite finished that when I had to stop work).

Admittedly, it's ghastly complicated and pretty horrible code currently, and I tried to include splitting the GUI and Client/Server code more cleanly at the same time, meaning a pretty much total rewrite... Which rather stopped me when too many changes to Jamulus were made for my rebases to be clean...