Iox #404 Fix verbose output and introduce CmdLineArgs_t struct #523

mossmaurice · 2021-01-27T13:34:07Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

Fix output when calling ./iox-roudi --help
Print command line args in RouDiApp instead of in main
Change interface from CmdLineParser& to CmdLineArgs_t struct to make RouDi more modular
Add CmdLineParser* unit tests

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Code according to our coding style and naming conventions
Unit tests have been written for new behavior
Public API changes are documented via doxygen
Copyright owner are updated in the changed files
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

Post-review Checklist for the Eclipse Committer

All checkboxes in the PR checklist are checked or crossed out
Merge

References

Partly closes Wrong handling of applications that are started multiple times in parallel #404

…neParser Signed-off-by: Simon Hoinkis <[email protected]>

Signed-off-by: Simon Hoinkis <[email protected]>

iceoryx_posh/include/iceoryx_posh/roudi/cmd_line_args.hpp

iceoryx_posh/source/roudi/roudi_config_toml_file_provider.cpp

Signed-off-by: Simon Hoinkis <[email protected]>

iceoryx_posh/include/iceoryx_posh/roudi/cmd_line_args.hpp

Signed-off-by: Simon Hoinkis <[email protected]>

…#404-fix-verbose-output

elBoberido

Do we have tests for the CmdLineParser?

iceoryx_posh/source/roudi/roudi_cmd_line_parser.cpp

elBoberido · 2021-02-01T16:51:34Z

iceoryx_posh/source/roudi/application/roudi_main.cpp

-    cmdLineParser.printParameters();
-
-    IceOryxRouDiApp roudi(cmdLineParser, roudiConfig);
+    IceOryxRouDiApp roudi(cmdLineArgs, roudiConfig);


It would be nice to only have the roudiConfig here. Ideally such things like the LogLevel could also set by TOML but the cmd line would have a higher precedence. Maybe something for another PR.

Yeah, good point. For me this is related to the RouDi refactorings for safety certificiation. This epic should be started with #230

Signed-off-by: Simon Hoinkis <[email protected]>

…st cases Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice · 2021-02-03T11:41:35Z

iceoryx_posh/source/roudi/roudi_cmd_line_parser.cpp

                                      {"kill-delay", required_argument, nullptr, 'k'},
                                      {nullptr, 0, nullptr, 0}};

    // colon after shortOption means it requires an argument, two colons mean optional argument
-    constexpr const char* shortOptions = "hvm:l:u:c:k:";
+    constexpr const char* shortOptions = "hvm:l:u:x:k:";


Hope this change won't lead to misfortune 😆

oh no, the luck is gone 😆

Okay, there was a clash with the --config-file option. It's a bit unfortunate that the long option doesn't start with the same letter as the short option. Maybe we can think of something that also fits and doesn't yet have a short option.

Signed-off-by: Simon Hoinkis <[email protected]>

iceoryx_examples/iceperf/roudi_main_static_config.cpp

iceoryx_posh/include/iceoryx_posh/roudi/roudi_cmd_line_parser.hpp

iceoryx_posh/source/roudi/application/roudi_main.cpp

elBoberido · 2021-02-03T13:37:35Z

iceoryx_posh/source/roudi/roudi_cmd_line_parser.cpp

                                      {"kill-delay", required_argument, nullptr, 'k'},
                                      {nullptr, 0, nullptr, 0}};

    // colon after shortOption means it requires an argument, two colons mean optional argument
-    constexpr const char* shortOptions = "hvm:l:u:c:k:";
+    constexpr const char* shortOptions = "hvm:l:u:x:k:";


Okay, there was a clash with the --config-file option. It's a bit unfortunate that the long option doesn't start with the same letter as the short option. Maybe we can think of something that also fits and doesn't yet have a short option.

iceoryx_posh/test/moduletests/test_roudi_cmd_line_parser.cpp

Signed-off-by: Simon Hoinkis <[email protected]>

iceoryx_examples/iceperf/roudi_main_static_config.cpp

iceoryx_posh/source/roudi/application/roudi_main.cpp

orecham

Only one minor point to consider otherwise looks good.

iceoryx_examples/iceperf/roudi_main_static_config.cpp

Signed-off-by: Simon Hoinkis <[email protected]>

orecham · 2021-02-04T13:36:22Z

iceoryx_examples/iceperf/roudi_main_static_config.cpp

@@ -25,7 +25,12 @@ int main(int argc, char* argv[])
    static constexpr uint32_t ONE_MEGABYTE = 1024U * 1024;

    iox::config::CmdLineParserConfigFileOption cmdLineParser;
-    cmdLineParser.parse(argc, argv);
+    auto cmdLineArgs = cmdLineParser.parse(argc, argv);
+    if (cmdLineArgs.has_error() && (cmdLineArgs.get_error() != iox::config::CmdLineParserResult::INFO_OUTPUT_ONLY))


Is INFO_OUTPUT_ONLY an error or success result ?

Neither an error nor a success, but something inbetween. This is why I called it CmdLineParserResult and not CmdLineParserError. We did the same for removing the optional inside a expected for the new v1.0 API. However, the methods are still called get_error(). We might consider renaming them to get_result() but that is somehow ambiguous.

Yeah... but then get_error can be a lie !

Rust uses the same terminology for Result as we currently do for the expected.

@elBoberido What's your take on this?

both of you are right ... and I have to leave 😁
get_unexpected might be a better name is this case, but I'm not quite sure what's in the expected proposal.
As mossmaurice pointed out, Rust does it the same way, although it would be more idiomatic to use a Result<Option<CmdLineArgs>, E> in this case.
I'm slightly leaning towards being pragmatic and keep it as it is, since we are doing the same with takeResult in the Subscriber. We could think of renaming the method, though.

mossmaurice added 3 commits January 26, 2021 15:46

iox-eclipse-iceoryx#404 Pass cmdLineArgs to RouDiApp instead of cmdLi…

0e798da

…neParser Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#404 Remove unecessary namespace

ab6db2e

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#404 Remove out-dated method in RouDi app

afd04d6

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice added bugfix Solves a bug refactoring Refactor code without adding features labels Jan 27, 2021

mossmaurice requested review from marthtz and elBoberido January 27, 2021 13:34

mossmaurice self-assigned this Jan 27, 2021

iox-eclipse-iceoryx#404 Add missing copyright info

28f5715

Signed-off-by: Simon Hoinkis <[email protected]>

elfenpiff reviewed Jan 28, 2021

View reviewed changes

iceoryx_posh/include/iceoryx_posh/roudi/cmd_line_args.hpp Outdated Show resolved Hide resolved

iceoryx_posh/source/roudi/roudi_config_toml_file_provider.cpp Outdated Show resolved Hide resolved

iox-eclipse-iceoryx#404 Address review findings from Christian Eltzschig

026f92e

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice requested a review from elfenpiff January 28, 2021 18:44

FerdinandSpitzschnueffler reviewed Jan 29, 2021

View reviewed changes

iceoryx_posh/include/iceoryx_posh/roudi/cmd_line_args.hpp Outdated Show resolved Hide resolved

mossmaurice added 2 commits January 29, 2021 18:18

iox-eclipse-iceoryx#404 Fix copyright header

f116f71

Signed-off-by: Simon Hoinkis <[email protected]>

Merge remote-tracking branch 'origin/master' into iox-eclipse-iceoryx…

d7d7b9f

…#404-fix-verbose-output

elBoberido previously approved these changes Feb 1, 2021

View reviewed changes

mossmaurice dismissed elBoberido’s stale review via e316481 February 3, 2021 10:42

iox-eclipse-iceoryx#404 Add unit test for CmdLineParser*

c582303

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice force-pushed the iox-#404-fix-verbose-output branch from e316481 to c582303 Compare February 3, 2021 11:02

mossmaurice added 2 commits February 3, 2021 12:07

iox-eclipse-iceoryx#404 Fix the fix f116f71

6499b52

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#404 Fix doxygen and add two more CmdLineParser te…

bd873da

…st cases Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice requested a review from elBoberido February 3, 2021 11:27

mossmaurice commented Feb 3, 2021

View reviewed changes

iox-eclipse-iceoryx#404 Disable CmdLineParser* tests on Windows

e3c26ec

Signed-off-by: Simon Hoinkis <[email protected]>

elBoberido reviewed Feb 3, 2021

View reviewed changes

iox-eclipse-iceoryx#404 Address review findings from Mathias Kraus

a0c9f40

Signed-off-by: Simon Hoinkis <[email protected]>

orecham reviewed Feb 3, 2021

View reviewed changes

iceoryx_examples/iceperf/roudi_main_static_config.cpp Outdated Show resolved Hide resolved

mossmaurice removed request for marthtz and elfenpiff February 3, 2021 15:00

orecham reviewed Feb 3, 2021

View reviewed changes

iceoryx_posh/source/roudi/application/roudi_main.cpp Outdated Show resolved Hide resolved

orecham previously approved these changes Feb 3, 2021

View reviewed changes

mossmaurice mentioned this pull request Feb 3, 2021

RouDi modularisation #539

Open

9 tasks

elBoberido reviewed Feb 3, 2021

View reviewed changes

iceoryx_examples/iceperf/roudi_main_static_config.cpp Outdated Show resolved Hide resolved

iox-eclipse-iceoryx#404 Address review findings from Mathias Kraus

deff5a6

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice dismissed orecham’s stale review via deff5a6 February 4, 2021 09:01

mossmaurice requested review from orecham and elBoberido February 4, 2021 09:02

elBoberido approved these changes Feb 4, 2021

View reviewed changes

orecham reviewed Feb 4, 2021

View reviewed changes

orecham approved these changes Feb 5, 2021

View reviewed changes

mossmaurice merged commit 2c0c504 into eclipse-iceoryx:master Feb 5, 2021

enkeyz mentioned this pull request Feb 5, 2021

iox-#536 refactored creation/getter method names in units::Duration #546

Merged

19 tasks

marthtz mentioned this pull request Mar 5, 2021

Iox #454 enhance cmd line parser tests #564

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iox #404 Fix verbose output and introduce CmdLineArgs_t struct #523

Iox #404 Fix verbose output and introduce CmdLineArgs_t struct #523

mossmaurice commented Jan 27, 2021 •

edited

Loading

elBoberido left a comment

elBoberido Feb 1, 2021

mossmaurice Feb 2, 2021

mossmaurice Feb 3, 2021

elBoberido Feb 3, 2021

elBoberido Feb 3, 2021

elBoberido Feb 3, 2021

orecham left a comment

orecham Feb 4, 2021

mossmaurice Feb 4, 2021 •

edited

Loading

orecham Feb 4, 2021

mossmaurice Feb 4, 2021 •

edited

Loading

elBoberido Feb 4, 2021

Iox #404 Fix verbose output and introduce CmdLineArgs_t struct #523

Iox #404 Fix verbose output and introduce CmdLineArgs_t struct #523

Conversation

mossmaurice commented Jan 27, 2021 • edited Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

Post-review Checklist for the Eclipse Committer

References

elBoberido left a 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

orecham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossmaurice Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossmaurice Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossmaurice commented Jan 27, 2021 •

edited

Loading

mossmaurice Feb 4, 2021 •

edited

Loading

mossmaurice Feb 4, 2021 •

edited

Loading