add yaml dumper representator for str type to keep quotes always.

[fujitatomoya]

closes #384

[fujitatomoya]

this make sure that yaml dumper uses quotes to str object during dumping, so that rcl_yaml_param_parser (libyaml) can recognize that as str always. e.g) "123E5".

[danielcranston]

@fujitatomoya Thanks, this indeed resolves the issue! FWIW, I also went with the yaml.add_representer approach in the test I mentioned in the linked issue:

I did a quick (rclcpp) test of solution 1 above, and indeed with explicit quotes in the temporary YAML the issue disappears.

So I approve of the approach 🙂

I think we can reduce the diff though, see the comment and let me know what you think.

Also, if a maintainer reads this and wants an easy way to reproduce, you can clone and build this branch, then run

ros2 launch parameter_test test.launch.py param:=123E5

Before applying this PR:

[parameter_test-1] terminate called after throwing an instance of 'rclcpp::exceptions::InvalidParameterTypeException'
[parameter_test-1]   what():  parameter 'param' has invalid type: Wrong parameter type, parameter {param} is of type {string}, setting it to {double} is not allowed.
[ERROR] [parameter_test-1]: process has died [pid 10097, exit code -6, cmd 'path/to/ws/install/parameter_test/lib/parameter_test/parameter_test --ros-args -r __node:=param_test --params-file /tmp/launch_params_xzypvhlu'].

Contents of "/tmp/launch_params_xzypvhlu" (The lack of quotes is the issue)

/**:
  ros__parameters:
    param: 123E5

After applying this PR:

[parameter_test-1] [INFO] [1738674287.523757293] [param_test]: param is 123E5

Contents of the associated YAML params file in /tmp (all strings have explicit quotes)

"/**":
  "ros__parameters":
    "param": "123E5"

[fujitatomoya]

Pulls: #436
Gist: https://gist.githubusercontent.com/fujitatomoya/bf4e720e5b67bb7c6b64d63fbd37cca8/raw/42b5ade1f8d08380258f484fed9a59674a399e48/ros2.repos
BUILD args: --packages-above-and-dependencies launch_ros
TEST args: --packages-above launch_ros
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15116

• Linux
• Linux (retry)
• Linux-aarch64
• Linux-rhel
• Windows

[christophebedard]

The test_security test failures seem suspicious.

[mjcarroll]

The test_security test failures seem suspicious.

They all seem to be this, maybe the governance filename isn't making it through the YAML (str vs bytes or something like that?)

[test_publisher-1] 1738694078.825545 [232] test_secur: Error occurred while validating local permissions: Can not parse governance file(code: 137)
[test_subscriber-2] 1738694078.825559 [232] test_secur: Error occurred while validating local permissions: Can not parse governance file(code: 137)

[fujitatomoya]

it does not ring a bell for me, need to look into those. @danielcranston do you have any thoughts?

[danielcranston]

I'm new to the ros2 org CI, I don't even know what "governance file" might refer to. I'm just a guy whose hardware serial number unluckily happened to have an E in the second to last digit 😄

Some org-wide github search yielded these:
ros2/system_tests#408 (comment)
ros2/ros2#926 (comment)

Maybe there's some flakiness going on? The "Linux build (retry)" one seems to have passed.

[clalancette]

Maybe there's some flakiness going on? The "Linux build (retry)" one seems to have passed.

Yeah, there is a long-time flake there, where every once in a long while, the security tests fail. A "governance" file has to do with how our security works. Regardless, since the rebuild worked, you can safely ignore it.

[christophebedard]

Yeah, there is a long-time flake there, where every once in a long while, the security tests fail. A "governance" file has to do with how our security works. Regardless, since the rebuild worked, you can safely ignore it.

Thanks for the info!

@fujitatomoya I think it would be good to add a test for this. Is there an easy way to test it?

[fujitatomoya]

I think it would be good to add a test for this. Is there an easy way to test it?

@christophebedard that is a good question, let's see what i can do here...

[fujitatomoya]

@christophebedard i had a quick scan, it would be hard to add the actual test case for this. for doing this, we do need the actual node implementation of specific type of parameter, and using LaunchConfiguration to override the parameter 123E5 (double type without fix, string with fix) and makes sure that parameter type matches.

ros2launch does not any actually tests, probably we want to add the tests there with another PR.

[fujitatomoya]

@christophebedard at least, @danielcranston 's reproducible test is passing and i also made sure locally, and all tests are pass. i would want to go merge this one and create another issue to add more tests for ros2launch. what do you think? if there are good approach, i am listening 👂