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

Jump to bottom

Remove comments from yaml file before applying substitutions #322

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

Remove comments from yaml file before applying substitutions #322

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion launch_ros/launch_ros/parameter_descriptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
Copy link

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

Copy link

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 •
edited
Loading

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)

except Exception:
raise SubstitutionFailure(
'The parameter file is not a valid yaml file, '
'before applying substitutions')
parsed = perform_substitutions(context, parse_substitution(file_without_comments))
try:
yaml.safe_load(parsed)
except Exception:
Expand Down