BUILD(shared): build CLI11 into shared target #6243

WOLFI3654 · 2023-10-20T15:18:35Z

Implements #6063

Checks

My commits follow the commit guidelines

WOLFI3654 · 2023-10-20T15:31:47Z

Just started working on this. There are two questions I already have:

How are translations handled for the flags itself and the flag descriptions?
Should be flags be rewritten to follow general cli standards? from my understanding "-third-party-licenses" would be a bit odd since the single dash is mostly only used for shorthands ( in this case something like -tpl)

WOLFI3654 · 2023-10-20T16:19:27Z

2. Should be flags be rewritten to follow general cli standards?

Probably interesting for this as well: CLI11 does not permit single args that have more than one character

WOLFI3654 · 2023-10-20T17:34:34Z

Also I am having difficulties getting --help to work under windows. It looks like this never really worked in the first place.

Hartmnt · 2023-10-20T17:46:09Z

How are translations handled for the flags itself and the flag descriptions?

It looks like neither flags nor descriptions are currently translated. I would say translating the flags itself is not something we want. Translating the descriptions would make sense, but it would require to wrap all descriptions into QObject::tr() call. The actual translations are then community created outside of this MR - once it is merged.

Should be flags be rewritten to follow general cli standards? from my understanding "-third-party-licenses" would be a bit odd since the single dash is mostly only used for shorthands ( in this case something like -tpl)

I would say yes, except when you encounter something that could potentially break servers or their automation. In that case we should look at that again in particular.

But, let's also have input from @Krzmbrzl for that

WOLFI3654 · 2023-10-20T18:46:59Z

I would say yes, except when you encounter something that could potentially break servers or their automation. In that case we should look at that again in particular.

I'd say most of these are breaking since single dash is forbidden for multicharacter options. And on server side, most of the options are.

src/murmur/main.cpp

Krzmbrzl · 2023-10-25T07:23:08Z

How are translations handled for the flags itself and the flag descriptions?

Flags should definitely not be translated. Their description don't need to be translated either imo - though this is open for discussion. Maybe omitting translation for now and then looking into that once requested would be an OK approach?

Should be flags be rewritten to follow general cli standards? from my understanding "-third-party-licenses" would be a bit odd since the single dash is mostly only used for shorthands ( in this case something like -tpl)

Yes - I think CLI11 doesn't even support non standard flags 👀

Also I am having difficulties getting --help to work under windows. It looks like this never really worked in the first place.

That might very well be the case - on Windows Mumble essentially never gets started from a command line.

It looks like neither flags nor descriptions are currently translated.

The description is currently translated - which is a massive PITA if you ask me as the help text is translated in its entirety.

WOLFI3654 · 2023-10-26T09:57:26Z

Yes - I think CLI11 doesn't even support non standard flags 👀

Thank you, yeah, this confirms what I figured. So this whole MR will basically be a breaking change for anyone using the CLI in automation. Unless we add some kind of mapping layer that will sort of “up cast” the old parameters to the new system. Is that something we should go for to smooth out the transition or do we just roll with the break, since the jump from 1.4 to 1.5 would be a somewhat good opportunity to refactor / re-think the CLI.

Krzmbrzl · 2023-10-26T10:01:13Z

I would roll with the break as I don't think maintaining support for non-standard flags makes sense for the future, so eventually we'll have to make a break (or keep the mess of manual option handling that we currently have) 🤔

since the jump from 1.4 to 1.5 would be a somewhat good opportunity to refactor / re-think the CLI.

This PR won't be part of 1.5 anymore, but 1.6 will do equally fine :)

Krzmbrzl · 2023-11-04T10:09:36Z

3rdparty/CLI11

I think we should stick to tagged releases of CLI11 - the latest of these would be https://github.com/CLIUtils/CLI11/releases/tag/v2.3.2 as of this writing.

Submodule should be put at head of tag now

Krzmbrzl · 2023-11-04T10:11:23Z

src/CMakeLists.txt

@@ -234,6 +234,8 @@ else()
 	endif()
 endif()

+target_link_libraries(shared PUBLIC CLI11::CLI11)


Please add the dependency to the client and the server explicitly - the shared target really should only have dependencies for the shared code and not dependencies that both components happen to create in their own code respectively.

Krzmbrzl · 2023-11-04T10:12:28Z

src/murmur/main.cpp

+#include <boost/logic/tribool.hpp>
+#include <boost/optional/optional_io.hpp>
+#include <boost/tuple/tuple_io.hpp>


If possible, I would like to stick to std-types. If that would make things a lot more complicated, stick to using Boost though.

I tried using std types and experienced some issues since they are not always available depending on which target system is built. Yeah boost is quite expensive to compile, I agree - not using them would be better. I'll try to see if I can remove them after things work

std-types should always present. You only have to check to stick to std types included in up to cpp14 as that's the current standard we are using.

Sadly, std::optional is not avalable in cpp14 afaik. I'll try to find another solution.

Yes, but an empty string will do equally fine for indicating that the INI file has not been specified :)

src/murmur/main.cpp

Hartmnt · 2024-05-31T12:20:12Z

What is the state on this?

WOLFI3654 · 2024-05-31T13:41:47Z

Hi — sorry, I kinda forgot about this.
From the looks of it I believe it was functional and working. If I remember correctly only thing "left" todo is to remove the need for boost

Hartmnt · 2024-05-31T13:51:25Z

Hi — sorry, I kinda forgot about this. From the looks of it I believe it was functional and working. If I remember correctly only thing "left" todo is to remove the need for boost

Nice :)
Do you have the time and motivation to finalize this MR by any chance? (no pressure)

WOLFI3654 · 2024-05-31T14:54:02Z

My time is sadly quite limited at the moment, and I don't know for sure when I could get around to it. I can try to look at it again in a few weeks (motivation is there), but no idea when you want this to be done :D

Hartmnt · 2024-05-31T14:56:56Z

My time is sadly quite limited at the moment, and I don't know for sure when I could get around to it. I can try to look at it again in a few weeks (motivation is there), but no idea when you want this to be done :D

Since this will not make it as a backport in 1.5.x, there is a lot of time left until we need this :D So you can take your time. I just wanted to make sure this was not entirely abandoned

WOLFI3654 commented Oct 21, 2023

View reviewed changes

src/murmur/main.cpp Show resolved Hide resolved

WOLFI3654 commented Oct 21, 2023

View reviewed changes

src/murmur/main.cpp Show resolved Hide resolved

WOLFI3654 force-pushed the feat/server/cli-11 branch 5 times, most recently from b085481 to 275f275 Compare October 21, 2023 18:50

WOLFI3654 force-pushed the feat/server/cli-11 branch 3 times, most recently from 19349a6 to 12dd794 Compare October 27, 2023 16:31

Krzmbrzl reviewed Nov 4, 2023

View reviewed changes

WOLFI3654 force-pushed the feat/server/cli-11 branch 8 times, most recently from 225e3f4 to 74a1803 Compare November 11, 2023 18:10

Krzmbrzl added client server feature-request This issue or PR deals with a new feature labels Feb 18, 2024

Hartmnt mentioned this pull request May 31, 2024

Warning/info msg about migration after initial install #6193

Open

WOLFI3654 added 2 commits May 31, 2024 15:46

BUILD(shared): build CLI11 into shared target

ae787df

FEAT(server,cli): parse options via CLI11

5b718a0

WOLFI3654 force-pushed the feat/server/cli-11 branch from 74a1803 to 5b718a0 Compare May 31, 2024 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUILD(shared): build CLI11 into shared target #6243

BUILD(shared): build CLI11 into shared target #6243

WOLFI3654 commented Oct 20, 2023

WOLFI3654 commented Oct 20, 2023

WOLFI3654 commented Oct 20, 2023

WOLFI3654 commented Oct 20, 2023

Hartmnt commented Oct 20, 2023

WOLFI3654 commented Oct 20, 2023

Krzmbrzl commented Oct 25, 2023

WOLFI3654 commented Oct 26, 2023

Krzmbrzl commented Oct 26, 2023

Krzmbrzl Nov 4, 2023

WOLFI3654 Nov 11, 2023

Krzmbrzl Nov 4, 2023

Krzmbrzl Nov 4, 2023

WOLFI3654 Nov 4, 2023

Krzmbrzl Nov 5, 2023

WOLFI3654 Nov 5, 2023

Krzmbrzl Nov 12, 2023

Hartmnt commented May 31, 2024

WOLFI3654 commented May 31, 2024

Hartmnt commented May 31, 2024

WOLFI3654 commented May 31, 2024

Hartmnt commented May 31, 2024

BUILD(shared): build CLI11 into shared target #6243

Are you sure you want to change the base?

BUILD(shared): build CLI11 into shared target #6243

Conversation

WOLFI3654 commented Oct 20, 2023

Checks

WOLFI3654 commented Oct 20, 2023

WOLFI3654 commented Oct 20, 2023

WOLFI3654 commented Oct 20, 2023

Hartmnt commented Oct 20, 2023

WOLFI3654 commented Oct 20, 2023

Krzmbrzl commented Oct 25, 2023

WOLFI3654 commented Oct 26, 2023

Krzmbrzl commented Oct 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hartmnt commented May 31, 2024

WOLFI3654 commented May 31, 2024

Hartmnt commented May 31, 2024

WOLFI3654 commented May 31, 2024

Hartmnt commented May 31, 2024