Support parameter overrides and remap rules flags on command line

[hidmic]

Precisely what the title says. A new rcl_arguments_get_param_overrides() API is introduced to retrieve those parameters after command line arguments parsing.

[sloretz]

The approach (generalizing the parsing of the node name prefix and left side name) looks good to me

[hidmic]

CI up to rclcpp and rclpy:

• Linux
• Linux-aarch64
• macOS
• Windows

[hidmic]

Command line parsing in rcl does no error handling on its own, beyond filling up the unparsed arguments bucket. I consciously did the same here, to keep this PR as simple as it can be, but I wonder if we should add it (and then figure out how to propagate those errors for downstream code to deal with, maybe re-using rcutils error messages?).

[ivanpauno]

I understood that there was not need of repeating --param or -p if you want to pass multiple parameters.
e.g.: EXEC_NAME --ros-args -p param1:=value1 param2:=value2 should work.
Also, I think -r and --remap aren't handled correctly, and they are being added to "not handle ros specific arguments" array, which is not correct.

[ivanpauno]

This is just a debug message because we want to accept things like EXECUTABLE_NAME --ros-args -p -r asd:=bsd, right?

[hidmic]

Not entirely sure, thus my comment above. Currently, rcl_parse_arguments has no concept of a bad argument, it simply avoids acknowledging them as such and defers error handling to upper layers (as it has to). I kept it as-is to avoid having to define how it should behave (i.e. how it notifies the caller about a bad option, what it does with the half built rcl_arguments struct, etc.). Also, it allows upper layers to give other meanings to rcl flags in their own custom ways, which I personally think is not a great idea but others may disagree.

[ivanpauno]

Now that we have the concept of --ros-args, I think we should have a concept of bad argument here. It's also ok to have an array with non parsed ros specific arguments, because we may want to have specific options for only one client library and not for the other.
But IMO, if a remapping rule was badly written we should notify the error here and not defer it .
I think we can left this discussion for a follow-up PR if you want.

[ivanpauno]

I would add a test with -r --remap arguments, and one combining both params and remapping rules.

[hidmic]

This PR does not implement --remap/-r flags nor aims to.

[hidmic]

I understood that there was not need of repeating --param or -p if you want to pass multiple parameters.

That is not what the design document says. It also makes it impossible to do this incrementally.

Also, I think -r and --remap aren't handled correctly, and they are being added to "not handle ros specific arguments" array, which is not correct.

Actually, that is correct for the moment. This PR does not implement --remap/-r flags but only parameter overrides. That'll come in a follow-up PR, after which we should also decide whether things like __log_level should remain as they are today or become flags e.g. --log-level.

Thank you for the review @ivanpauno !

[ivanpauno]

That is not what the design document says. It also makes it impossible to do this incrementally.

I think it's ok taking only one param assignment per -p/--param option.
The only thing that I don't like is that after this PR, the following example:
EXEC_NAME --ros-args --param asd:=bsd csd:=fsd
is unclear. asd:=bsd is a param assignment and csd:=fsd is a remapping rule.

I prefer going all in, and implement -r and --remap here than deferring it to another PR.

[hidmic]

The only thing that I don't like is that after this PR, the following example:
EXEC_NAME --ros-args --param asd:=bsd csd:=fsd
is unclear.

That is true, yes.

I prefer going all in, and implement -r and --remap here than deferring it to another PR.

Alright, fair enough.

[jacobperron]

Minor comments. LGTM

[hidmic]

@ivanpauno --remap/-r implemented in 6bb553c. I left the other args out on purpose, as I think that things like __log_rosout_disable should become --disable-log-rosout or alike. I'll do so in a follow-up PR if the @ros2/team agrees.

[ivanpauno]

LGTM with green CI

[ivanpauno]

Something else about error handling here (it applies to params too):
If the parsing of the remapping rule fails, we are not skipping it after that (i += 1), we're just skipping the flag and we will try to parse the "rule" as another thing later.
I don't know if that's good or not, we should probably improve error handling here (it can be done in a follow up).

[hidmic]

Yes, I know. It is as (in)correct as we deem it to be. We should probably fail entirely instead of moving forward though.

[hidmic]

Alright, all green. Going in.