-
Notifications
You must be signed in to change notification settings - Fork 32
Add factory introspection CLI commands: --list-available-factories and --print-factory-info #2023
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ctory-info Co-authored-by: wdconinc <[email protected]>
Co-authored-by: wdconinc <[email protected]>
Co-authored-by: wdconinc <[email protected]>
--list-available-plugins
) that can be used to query plugins for the available factories within them (--list-available-factories <plugin>
) and that can be used to query...for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Ensure that GitHub Actions checks succeed.
Co-authored-by: wdconinc <[email protected]>
Co-authored-by: wdconinc <[email protected]>
I've fixed the clang-tidy static analysis issues that were causing the GitHub Actions checks to fail. The main problems were C-style casts that needed to be replaced with Changes made in commit 6623f95:
The GitHub Actions checks should now pass with these clang-tidy compliance fixes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot There are still compilation failures in the CI checks. Notably the following two:
In file included from /home/runner/work/EICrecon/EICrecon/src/utilities/eicrecon/eicrecon.cc:11:
/home/runner/work/EICrecon/EICrecon/src/utilities/eicrecon/eicrecon_cli.h:87:6: error: redefinition of 'PrintFactoryInfo' as different kind of symbol
87 | void PrintFactoryInfo(JApplication* app);
| ^
/home/runner/work/EICrecon/EICrecon/src/utilities/eicrecon/eicrecon_cli.h:32:3: note: previous definition is here
32 | PrintFactoryInfo
| ^
1 error generated.
and
In file included from /home/runner/work/EICrecon/EICrecon/src/utilities/eicrecon/eicrecon_cli.cpp:5:
/home/runner/work/EICrecon/EICrecon/src/utilities/eicrecon/eicrecon_cli.h:87:6: error: redefinition of 'PrintFactoryInfo' as different kind of symbol
87 | void PrintFactoryInfo(JApplication* app);
| ^
/home/runner/work/EICrecon/EICrecon/src/utilities/eicrecon/eicrecon_cli.h:32:3: note: previous definition is here
32 | PrintFactoryInfo
| ^
/home/runner/work/EICrecon/EICrecon/src/utilities/eicrecon/eicrecon_cli.cpp:335:8: error: call to deleted constructor of 'JComponentSummary'
335 | auto cs = app->GetComponentSummary();
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/local/include/JANA/Components/JComponentSummary.h:102:5: note: 'JComponentSummary' has been explicitly marked deleted here
102 | JComponentSummary(const JComponentSummary&) = delete; // Otherwise we have a lot of internal pointers that have to be deep-copied
| ^
/home/runner/work/EICrecon/EICrecon/src/utilities/eicrecon/eicrecon_cli.cpp:403:6: error: redefinition of 'PrintFactoryInfo' as different kind of symbol
403 | void PrintFactoryInfo(JApplication* app) {
| ^
/home/runner/work/EICrecon/EICrecon/src/utilities/eicrecon/eicrecon_cli.h:32:3: note: previous definition is here
32 | PrintFactoryInfo
| ^
/home/runner/work/EICrecon/EICrecon/src/utilities/eicrecon/eicrecon_cli.cpp:406:8: error: call to deleted constructor of 'JComponentSummary'
406 | auto cs = app->GetComponentSummary();
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/local/include/JANA/Components/JComponentSummary.h:102:5: note: 'JComponentSummary' has been explicitly marked deleted here
102 | JComponentSummary(const JComponentSummary&) = delete; // Otherwise we have a lot of internal pointers that have to be deep-copied
| ^
/home/runner/work/EICrecon/EICrecon/src/utilities/eicrecon/eicrecon_cli.cpp:565:21: error: called object type 'jana::Flag' is not a function or function pointer
565 | PrintFactoryInfo(app);
| ~~~~~~~~~~~~~~~~^
5 errors generated.
@copilot There are still compilation failures in the CI checks. Notably the following two:
|
…tor usage Co-authored-by: wdconinc <[email protected]>
I've fixed the compilation failures identified in the CI checks. The issues were:
Changes made in commit d94f23d:
The CLI command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Add a test to linux-eic-shell.yml that runs eicrecon
with the new flags to demonstrate correct behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clang-Tidy
found issue(s) with the introduced code (1/1)
if (pattern.second.size() > 1) { // Only show patterns with multiple factories | ||
std::cout << " " << pattern.first << "*: " << pattern.second.size() << " collections ("; | ||
for (size_t i = 0; i < std::min(pattern.second.size(), static_cast<size_t>(3)); ++i) { | ||
if (i > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
statement should be inside braces
if (i > 0) | |
if (i > 0) { |
for (size_t i = 0; i < std::min(pattern.second.size(), static_cast<size_t>(3)); ++i) { | ||
if (i > 0) | ||
std::cout << ", "; | ||
std::cout << pattern.second[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
statement should be inside braces
std::cout << pattern.second[i]; | |
} | |
std::cout << pattern.second[i]; |
std::cout << ", "; | ||
std::cout << pattern.second[i]; | ||
} | ||
if (pattern.second.size() > 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
statement should be inside braces
if (pattern.second.size() > 3) | |
if (pattern.second.size() > 3) { |
} | ||
if (pattern.second.size() > 3) | ||
std::cout << ", ..."; | ||
std::cout << ")" << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
statement should be inside braces
std::cout << ")" << std::endl; | |
} | |
std::cout << ")" << std::endl; |
Co-authored-by: wdconinc <[email protected]>
I've added a test job
The job follows the same patterns as other workflow jobs, downloading the install directory and setting up the environment properly. This ensures the new introspection commands are tested as part of CI. Commit: dbd1d7c |
…d --print-factory-info (fix: iwyu) (#2024) This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/17087436614. Please merge this PR into the branch `copilot/fix-d97e6d4d-15a1-4be9-94f9-55e34622d576` to resolve failures in PR #2023. Auto-generated by [create-pull-request][1] [1]: https://github.com/peter-evans/create-pull-request Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
for more information, see https://pre-commit.ci
@copilot Address the error |
@copilot Address the readability-braces-around-statements clang-tidy check by making sure single line statement are also surrounded by braces. |
Co-authored-by: wdconinc <[email protected]>
Fixed the AddressSanitizer suppressions file error by adding the "Checkout .github" step to the Commit: a698d05 |
Co-authored-by: wdconinc <[email protected]>
I've fixed the Commit: 5f3e01b |
No Clang-Tidy warnings found so I assume my comments were addressed
This PR implements two new CLI help commands for eicrecon to provide better plugin and factory introspection capabilities, addressing the need for users to understand the available factories and their relationships within the EICrecon framework.
New Commands
--list-available-factories <plugin>
Query plugins for available factories within them, similar to the existing
--list-available-plugins
command.Features:
--print-factory-info
Query factories for comprehensive system information and provide detailed factory overview.
Features:
Implementation Details
The implementation follows existing CLI patterns and integrates seamlessly with the current command structure:
ListPluginFactories
andPrintFactoryInfo
flagsplugin_name
field for plugin-specific queriesJTablePrinter
for uniform outputTesting
Added comprehensive CI testing in the GitHub Actions workflow with a dedicated
test-factory-cli
job that validates both commands work correctly in the eic-shell environment.Documentation
Added comprehensive user documentation at
docs/tutorial/05-factory-help-commands.md
with:Benefits
This enhancement provides valuable introspection capabilities for EICrecon users:
The commands complement the existing
--list-available-plugins
and--list-factories
options, providing a complete hierarchy of introspection from plugins → factories → detailed information.Future Extensions
The implementation provides a foundation for future enhancements when extended JANA API access becomes available, such as runtime access to input/output collection specifications and factory dependency graphs.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.