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.

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

Remove comments from yaml file before applying substitutions #322

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

ivanpauno
Copy link
Member

Fixes ros2/ros2#1317.

I'm not sure if it's the best solution, but it's one.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the bug Something isn't working label Aug 26, 2022
@ivanpauno ivanpauno requested a review from hidmic August 26, 2022 15:59
@ivanpauno ivanpauno self-assigned this Aug 26, 2022
@@ -240,7 +240,13 @@ def evaluate(self, context: LaunchContext) -> Path:
with open(param_file_path, 'r') as f, NamedTemporaryFile(
mode='w', prefix='launch_params_', delete=False
) as h:
parsed = perform_substitutions(context, parse_substitution(f.read()))
try:
file_without_comments = yaml.safe_dump(yaml.safe_load(f.read()))

Choose a reason for hiding this comment

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

It seems to break a case, which I am not sure if it's a correct case, that could be parsed successfully before.

/**:
  ros__parameters:
    mix_subs: "$(command pwd) $(dirname)"

before:

$ ros2 param get /parameter_blackboard mix_subs
String value is: /home/chenlh/Projects/ROS2/ros2-master /home/chenlh/Projects/ROS2/ros2-master/install/share/demo_nodes_cpp/launch/topics

after using 95d4699:

$ ros2 launch demo_nodes_cpp parameter_blackboard.xml
[INFO] [launch]: All log files can be found below /home/chenlh/.ros/log/2022-08-30-11-39-18-674265-OptiPlex-7080-460434
[INFO] [launch]: Default logging verbosity is set to INFO
tree Tree(template, [Tree(fragment, [Token(UNQUOTED_STRING, 'demo_nodes_cpp')])])
tree Tree(template, [Tree(fragment, [Token(UNQUOTED_STRING, 'parameter_blackboard')])])
tree Tree(template, [Tree(fragment, [Token(UNQUOTED_STRING, 'parameter_file_with_substitutions.yaml')])])
tree Tree(template, [Tree(fragment, [Token(UNQUOTED_STRING, '/**:\n  ros__parameters:\n    mix_subs: ')]), Tree(fragment, [Tree(substitution, [Token(IDENTIFIER, 'command'), Tree(arguments, [Tree(value, [Tree(part, [Token(UNQUOTED_RSTRING, 'pwd')])])])])]), Tree(fragment, [Token(UNQUOTED_STRING, ' ')]), Tree(fragment, [Tree(substitution, [Token(IDENTIFIER, 'dirname')])]), Tree(fragment, [Token(UNQUOTED_STRING, '\n')])])
[ERROR] [launch]: Caught exception in launch (see debug for traceback): The substituted parameter file is not a valid yaml file

Choose a reason for hiding this comment

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

It seems that we need to invoke yaml.safe_dump with default_style='"', and then to support the yaml tag for !!int, !!float in the rcl which will be similar to ros2/rcl#999.

the data is dumped with default_style='"', and then parsed by substitutions.
"/**":
  "ros__parameters":
    "array":
    - !!int "1"
    - !!int "2"
    - !!int "3"
    "double": !!float "3.141592653"
    "filename": "run /home/chenlh/Projects/ROS2/ros2-master/install/share/demo_nodes_cpp/launch/topics/parameter_blackboard.xml on 2022年 08月 30日 星期二 13:50:26 CST
"
    "filename2": "2022年 08月 30日 星期二 13:50:26 CST
 /home/chenlh/Projects/ROS2/ros2-master/install/share/demo_nodes_cpp/launch/topics/parameter_blackboard.xml"
    "float": !!float "3.1"
    "int": !!int "3"

Copy link
Member Author

Choose a reason for hiding this comment

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

@iuhilnehc-ynos thanks for showing an example when the PR doesn't work correctly.

It seems that we need to invoke yaml.safe_dump with default_style='"'

This will cause issues in yaml files like:

/**:
  ros__parameters:
    my_integer: $(env MY_INTEGER)

Copy link

@iuhilnehc-ynos iuhilnehc-ynos Aug 31, 2022

Choose a reason for hiding this comment

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

A better way to fix this issue might need to update the grammar.lark to ignore the comment. I am not familiar with lark.

After referring to https://github.com/ros2/rosidl/blob/51811962c832745f531968d351af3d1af45cd238/rosidl_parser/rosidl_parser/grammar.lark#L30, I updated the `grammar.lark` with ```patch diff --git a/launch/share/launch/frontend/grammar.lark b/launch/share/launch/frontend/grammar.lark index 740c2c4..e2d45f5 100644 --- a/launch/share/launch/frontend/grammar.lark +++ b/launch/share/launch/frontend/grammar.lark @@ -4,6 +4,9 @@ %import common.LETTER %import common.WS

+COMMENT: "#" /[^\n]/*
+%ignore COMMENT
+
IDENTIFIER: LETTER (LETTER | DIGIT | "_" | "-")*


I am not sure if it covers all comment cases. I have tested the following cases, and it seems good with the result.
yaml file:

```yaml
# int: $(env MY_INT)
/**:
  ros__parameters:
    # int: $(env MY_INT)
    filename: "run $(filename) on $(command date)"   # int: $(env MY_INT)
    filename2: "$(command date) $(filename) #fdas"
    filename3: $(env MY_INT) $(filename) #fdas
    filename4: $(env MY_INT) # $(filename) #fdas
    int2: $(env MY_INT)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

YAML config: Substitution happens for comments
2 participants