[hadd] rewrite argument parsing logic

[silverweed]

This PR refactors (or rather, rewrites) hadd's argument parsing code to achieve the following improvements:

• code is simpler to follow, more consistent among different flags and extracted from main
• adding new flags is easier
• allows the familiar syntax of -j2 in addition to -j 2. As a bonus, and for consistency with -cachesize=, all flags can now also be passed as -j=2 (except for -f which has a special logic).
• allows passing flags after the positional arguments
• hadd will abort when encountering invalid flag arguments, instead of just printing a message

In the name of code simplification, this PR doesn't attempt to replicate the same exact error-reporting behavior string-by-string as the current version, but the results should ideally be the same.

I ran the roottest suite and all tests pass but we might want to add more tests to be sure.

Checklist:

• [x] tested changes locally
• [x] updated the docs (if necessary)

[ferdymercury]

Good idea! I would even suggest further simplifying it, using this header-only parser: https://github.com/CLIUtils/CLI11

[silverweed]

@ferdymercury is that library already used in ROOT? Considering it's 10k loc, I'd not count it as "simplifying" unless we decided to use it in multiple places (ideally all our cpp executables). Also, we must make sure that the argument parsing logic remains backward-compatible, which is not very clear to me if it would be the case with that lib.

[ferdymercury]

@ferdymercury is that library already used in ROOT?

I don't think it's used yet in ROOT, but @henryiii could confirm.

Considering it's 10k loc, I'd not count it as "simplifying" unless we decided to use it in multiple places (ideally all our cpp executables).

True. My experience is that there are many places doing CLI parsing over and over again within ROOT, so if we would replace everywhere, then we would remove more than 10k from ROOT's codebase, and reduce maintenance / bugs on ROOT's side, so it would be worth it. Or at least for new interfaces, see e.g. https://github.com/root-project/root/pull/14038

Also, we must make sure that the argument parsing logic remains backward-compatible, which is not very clear to me if it would be the case with that lib.

That lib has many customization options, so a derived parser class could probably do it, but yeah, it's hard to say for sure in advance.

[ferdymercury]

Related forum post: https://root-forum.cern.ch/t/command-line-argument-parsing/35783/ and PPT: https://indico.cern.ch/event/619465/contributions/2507949/attachments/1448567/2232649/20170424-diana-2.pdf

[github-actions[bot]]

Test Results

    17 files      17 suites   4d 6h 32m 57s ⏱️  2 694 tests  2 694 ✅ 0 💤 0 ❌ 44 144 runs  44 144 ✅ 0 💤 0 ❌

Results for commit 897ce2c9.

:recycle: This comment has been updated with latest results.

[silverweed]

@ferdymercury thanks for the links. I fully agree with Axel's proposal of trying to make all ROOT binaries consistent in their argument parsing and make them POSIX (while keeping the legacy -long options valid but hidden).

Having never used it, I don't have a strong opinion on the CLI11 library yet. On one hand it seems well maintained and tested on all platforms, which is great, and being header-only certainly makes it convenient to use. On the other hand it's quite big and I wonder if we need enough features from it as to justify its size.

In any case I think we should revive the topic and understand if there were any blockers to it or if it was simply not picked up by anyone in the past 3 years.

[silverweed]

I did a relatively big change with commit 5726d5f; the commit message describes the details.

[pcanal]

allows passing flags after the positional arguments

Do we have the corresponding support for -- for those adventurous users that want their filename to start with a -?

[henryiii]

I don't think it's used yet in ROOT, but @henryiii could confirm.

CLI11 had a "vendor" option added specifically for ROOT where you can vendor the it and set a custom namespace, but I don't think ROOT ever picked it up.

[pcanal]

Did we extent the testing of the hadd parameters? If so can we link the roottest PR in the description?

[silverweed]

@pcanal Yes, I'm adding the new tests here. I'll link it also in the OP.