Skip to content

[core] Use optparse in TApplication's option parsing#22689

Open
silverweed wants to merge 4 commits into
root-project:masterfrom
silverweed:tapplication_optparse
Open

[core] Use optparse in TApplication's option parsing#22689
silverweed wants to merge 4 commits into
root-project:masterfrom
silverweed:tapplication_optparse

Conversation

@silverweed

Copy link
Copy Markdown
Contributor

This Pull request:

replaces TApplication::GetOptions's implementation with one using the new optparse.hxx functionality. All existing flags are preserved for backward compatibility, but:

  • -a is not working as intended, so it's now ignored
  • -splash seemed unused, so it's ignored as well

Due to this change, we don't need anymore to generate the command line header file via root-argparse.py. In a future PR I intend to do the same changes to rootx and hadd and eventually get rid of the whole argparse2help.py functionality.

1. ability to ignore unknown flags and query all unprocessed arguments.
   This is useful for stuff like TApplication that needs to "pass through"
   unknown flags;
2. ability to query which arguments came after `--`. This is almost
   never needed, but it is used as a feature by, again, TApplication.
This allows accepting args coming from a `char **` and without
const-casting it to `const char **`.

@vepadulano vepadulano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this effort! I left a couple of minor comments.

-t, --enable-threading Enable thread-safety and implicit multi-threading (IMT)
-q, --quit-after-processing Exit after processing command line macro files
-l, --no-banner Do not show the ROOT banner
-config print ./configure options

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR, but just wanted to note that I didn't even know this existed, I tried it and I still don't understand what is the intended use case for it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really clean given we also have root-config as a separate CLI

[file1_C.so...fileN_C.so] Load and execute file1_C.so ... fileN_C.so (or .dll if on Windows)
They should be already-compiled ROOT macros (shared libraries) or:
regular user shared libraries e.g. userlib.so with a function userlib(args)
[anyfile1..anyfileN] All other arguments pointing to existing files will be checked to see if they are ROOT Files (checking the MIME type inside the file) and if they are not they will be handled as a ROOT macro file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[anyfile1..anyfileN] All other arguments pointing to existing files will be checked to see if they are ROOT Files (checking the MIME type inside the file) and if they are not they will be handled as a ROOT macro file
[anyfile1..anyfileN] All other arguments pointing to existing files will be checked to see if they are ROOT Files (checking the MIME type inside the file) and if they are not they will be handled as a ROOT macro file

Comment on lines +484 to +487
if (!fFiles) fFiles = new TObjArray;
TObjString *expr = new TObjString(std::string(cmd).c_str());
expr->SetBit(kExpression);
fFiles->Add(expr);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just making sure that fFiles is owning, so that we are sure the expr will be properly deleted in the destructor.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't really change the code here, but I guess the TObjArray will properly delete its elements. Of course one could cleanup that code as well, but I didn't want to introduce too many changes at once

}

// Process positional arguments after `--` as arguments for the macro.
// This is only valid if we passed at least one macro and will be considered arguments for the last one passed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have missed it, is this bit of information documented in the help message?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No; it's not there in the original message and I didn't add anything to it. I can do it in another commit if you want

@github-actions

Copy link
Copy Markdown

Test Results

    20 files      20 suites   3d 7h 18m 7s ⏱️
 3 873 tests  3 837 ✅   0 💤 36 ❌
70 123 runs  69 980 ✅ 107 💤 36 ❌

For more details on these failures, see this check.

Results for commit 10eb727.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants