Skip to content
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.

Sign up for GitHub

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

Promote special CLI rules to flags #495

Merged
merged 3 commits into from
Sep 12, 2019
Merged

Promote special CLI rules to flags #495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cefafd4
Promote special CLI rules to flags.
hidmic Sep 3, 2019
ce5b8a3
Improve argument parsing logging and error messages.
hidmic Sep 9, 2019
5f8181c
Remove some short form CLI flags.
hidmic Sep 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions rcl/include/rcl/arguments.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,16 @@ typedef struct rcl_arguments_t
#define RCL_ROS_ARGS_EXPLICIT_END_TOKEN "--"
#define RCL_PARAM_FLAG "--param"
#define RCL_SHORT_PARAM_FLAG "-p"
#define RCL_PARAM_FILE_FLAG "--params-file"
#define RCL_REMAP_FLAG "--remap"
#define RCL_SHORT_REMAP_FLAG "-r"
#define RCL_LOG_LEVEL_FLAG "--log-level"
#define RCL_EXTERNAL_LOG_CONFIG_FLAG "--log-config-file"
// To be prefixed with --enable- or --disable-
#define RCL_LOG_STDOUT_FLAG_SUFFIX "stdout-logs"
#define RCL_LOG_ROSOUT_FLAG_SUFFIX "rosout-logs"
#define RCL_LOG_EXT_LIB_FLAG_SUFFIX "external-lib-logs"

#define RCL_LOG_LEVEL_ARG_RULE "__log_level:="
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't we want to also keep the old way?
I'm fine with deleting them, but I'm not sure if we had consensus about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There're two related questions that need an answer here. Thanks for bringing it up again.

First, whether we want to drop the old rules or not. If we don't, that'd preclude supporting multiple values per flag (e.g. multiple remapping rules per -r flag). That's not supported nor in the design document, but I've seen interest expressed elsewhere.

Assuming that the answer to the first question is yes, how do we want to drop them? This is simply removing them, but we may want to go through a deprecation cycle to allow the community to settle. I'd like to hear what @dirk-thomas and @wjwwood think about this.

Copy link
Member

Choose a reason for hiding this comment

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

I had thought we'd settled on keeping the old rules so there is less disruption, but I may have imagined it.

I'm in favor of dropping them though with a deprecation cycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another detail I had not thought of is how far should we go back. This PR only or back to the start, with known limitations?

Copy link
Member

@ivanpauno ivanpauno Sep 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What start are you referring to?
So, I think that --ros-args, -r, -p should we always be enforced, without backwards compatibility. If not, it will be quite confusing.

About changing some of the __*:=* options for flags, I think we can support both for a time, first just deprecating __*:=* and the deleting them.

But as we will break API, by adding --ros-args and forcing -r -p, I think is also ok to just delete the old __*:=* options.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have to agree with @jacobperron that we should keep the old flags for now. While it is somewhat annoying, breaking everything in one release will lead to quite some fallout for downstream users (and it isn't just code; they'll also have to retrain their fingers to do something different). So I'd be for deprecating the old __*:=* in Eloquent, then removing them in F-turtle.

Which brings up a point about how we go about deprecating them. Obviously we can put it in documentation, but should we also print a console message if the old flags are used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, per offline discussion, we're going to re-instantiate support for old arguments and mark them as deprecated with a warning. I'll do so in another PR. Since it's orthogonal to this PR, we can defer merging till then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I take that back. It's much easier to do this in a follow-up PR, which I've written already. So if you don't mind, let's get this in and review the next PR right after.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jacobperron @ivanpauno ^

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay. A follow-up PR is okay with me. I'll take another look at this one.

#define RCL_EXTERNAL_LOG_CONFIG_ARG_RULE "__log_config_file:="
#define RCL_LOG_DISABLE_STDOUT_ARG_RULE "__log_disable_stdout:="
#define RCL_LOG_DISABLE_ROSOUT_ARG_RULE "__log_disable_rosout:="
#define RCL_LOG_DISABLE_EXT_LIB_ARG_RULE "__log_disable_external_lib:="
#define RCL_PARAM_FILE_ARG_RULE "__params:="

/// Return a rcl_arguments_t struct with members initialized to `NULL`.
RCL_PUBLIC
Expand Down Expand Up @@ -75,7 +76,7 @@ rcl_get_zero_initialized_arguments(void);
* Parameter override rule parsing is supported via `-p/--param` flags e.g. `--param name:=value`
* or `-p name:=value`.
*
* The default log level will be parsed as `__log_level:=level`, where `level` is a name
* The default log level will be parsed as `--log-level level`, where `level` is a name
* representing one of the log levels in the `RCUTILS_LOG_SEVERITY` enum, e.g. `info`, `debug`,
* `warn`, not case sensitive.
* If multiple of these rules are found, the last one parsed will be used.
Expand Down
2 changes: 1 addition & 1 deletion rcl/include/rcl/types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ typedef rmw_ret_t rcl_ret_t;
/// Argument is not a valid parameter rule
#define RCL_RET_INVALID_PARAM_RULE 1010
/// Argument is not a valid log level rule
#define RCL_RET_INVALID_LOG_LEVEL_RULE 1020
#define RCL_RET_INVALID_LOG_LEVEL 1020

// rcl event specific ret codes in 20XX
/// Invalid rcl_event_t given return code.
Expand Down
Loading