[hadd] rewrite argument parsing logic #16090
Conversation
silverweed
force-pushed
the
hadd_argparse
branch
from
9849c0b to
bc58e12
Compare
|
Good idea! I would even suggest further simplifying it, using this header-only parser: https://github.com/CLIUtils/CLI11 |
|
@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). |
I don't think it's used yet in ROOT, but @henryiii could confirm.
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. #14038
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. |
Test Results 18 files 18 suites 4d 0h 24m 56s ⏱️ For more details on these failures, see this check. Results for commit ba3be62. ♻️ This comment has been updated with latest results. |
|
@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 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
force-pushed
the
hadd_argparse
branch
from
c292bae to
282fd03
Compare
silverweed
force-pushed
the
hadd_argparse
branch
from
7abcf45 to
c8db834
Compare
| The first syntax is the preferred one since it's backward-compatible with previous versions of hadd. | ||
| The -f flag is an exception to this rule: it only supports the `-f[0-9]` syntax. | ||
|
|
||
| Note that merging multiple flags is NOT supported: `-jfa` will be interpreted as -j=fa, which is invalid! |
There was a problem hiding this comment.
Nice, there should be a test to double check that this situation raises an error
There was a problem hiding this comment.
|
I did a relatively big change with commit 5726d5f; the commit message describes the details. |
silverweed
force-pushed
the
hadd_argparse
branch
3 times, most recently
from
a31fb9c to
c8bf298
Compare
|
Did we extent the testing of the |
[hadd] fix off-by-one error [hadd] reintroduce RNTuple-specific options in hadd
Some flags, such as -v, accept an optional argument. To handle such cases, the Convert* functions are changed to return either a parsed value, an error value or an ignored value. If a flag accepting an optional value fails to convert the next argument via its convert function returning Ignored, it will assign the default value to the flag. The Convert functions should return Ignored when the next argument doesn't "look" like an attempt of passing a valid argument. This is different from Error, which means the next argument was probably intended to be of the correct type but failed to parse. This is a somewhat arbitrary heuristic so it should be implemented with consideration.
now they are like Error(), Warning() and Info() from TError.h
-- can be specified after the first group of flags and it causes any following argument to be considered a positional argument, including ones starting with -.
silverweed
force-pushed
the
hadd_argparse
branch
from
521b698 to
ba3be62
Compare
This PR refactors (or rather, rewrites)
hadd's argument parsing code to achieve the following improvements:-j2in addition to-j 2. As a bonus, and for consistency with-cachesize=, all flags can now also be passed as-j=2(except for-fwhich has a special logic).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.
Notes
New test suite for this change: root-project/roottest#1201
Checklist: