search

mumble-voip / mumble

Mumble is an open-source, low-latency, high quality voice chat software.
https://www.mumble.info
Other
6.14k stars 1.1k forks source link

Refactor CLI argument parsing #6063

Open Krzmbrzl opened 1 year ago

Krzmbrzl commented 1 year ago

Context

Currently we are performing manual parsing of command line flag, which incurs extra work on us and leads to non-conforming behavior (to Unix standards). Plus it bloats the main functions and just looks rather horrible.

Description

Instead of doing this manually, we should instead make use of one of the many existing libraries out there. My personal favorite at this point would be https://github.com/CLIUtils/CLI11

Mumble component

Both

OS-specific?

No

Additional information

https://github.com/mumble-voip/mumble/issues/6037#issuecomment-1433305266

rviu commented 1 year ago

Hi, I would like to take this up.

If I understand it correctly, the existing code for parsing needs to be replaced with the CLI11 library. I just went through CLI11 and it seems like only one header file needs to be included to use it.

Could you point me to the file where the existing code for parsing is present? Also, if there is anything else apart from the linked comment relevant to this issue that I need to take a look at, could you also mention that?

Thanks

Krzmbrzl commented 1 year ago

Great to hear that! :+1:

If I understand it correctly, the existing code for parsing needs to be replaced with the CLI11 library. I just went through CLI11 and it seems like only one header file needs to be included to use it.

Exactly. CLI11 can be added as a submodule (git submodule add) in the 3rdParty directory and then we only have to tell cmake to include it before referencing it.

Could you point me to the file where the existing code for parsing is present?

Client: https://github.com/mumble-voip/mumble/blob/82bcd1eb3d53aa9bfc1f6ff539961b0c29336266/src/mumble/main.cpp#L212-L443

Server: https://github.com/mumble-voip/mumble/blob/82bcd1eb3d53aa9bfc1f6ff539961b0c29336266/src/murmur/main.cpp#L292-L434

Also, if there is anything else apart from the linked comment relevant to this issue that I need to take a look at, could you also mention that?

Nah, nothing particular. Only this: I think you don't (and shouldn't really) worry about porting the non-POSIX-conforming CLI argument style (e.g. only a single dash for long (aka: not only a single letter) options). I think it's about time we made a proper cut here and move to the style that most programs use (in the Unix world anyway). CLI11 should take care of that though, so simply don't try to work around its defaults :D

I would recommend creating a draft pull request early on, so we can have a first glance at it as soon as possible to make sure we all agree on the direction this should take.

rviu commented 1 year ago

Thanks for the reply! I've started working on this (https://github.com/mumble-voip/mumble/pull/6143).

A question I have is how to test the changes that I make. Is being able to successfully build sufficient?

Krzmbrzl commented 1 year ago

A question I have is how to test the changes that I make. Is being able to successfully build sufficient?

Well, that's a good start but you'll also have to test whether the new CLI options work as intended (either by observing whether they cause Mumble to act accordingly or by introducing printouts to the code to verify that the expected code path is being taken)

WOLFI3654 commented 9 months ago

I guess with the closing of #6143 this is still on the plate. I'd happily take on.