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

Set maybe_placeholders to False for lark 1.+ compatibility #664

Merged
merged 1 commit into from Feb 18, 2022

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Feb 18, 2022

Fixes #663

This set's maybe_placeholders to False. It's default value changed to True in lark-parser 1.0.

In the grammar annotation applications are given as

annotation_appl: "@" scoped_name [ "(" annotation_appl_params ")" ]

The square brackets [ ... ] say "maybe there will be parentheses with parameters for the annotation application". If maybe_placeholders is set to True, then the AST will contain the value None when there aren't any parentheses or parameters. This sets it to False so that the AST does not contain anything in that case.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz added the bug Something isn't working label Feb 18, 2022
@sloretz sloretz self-assigned this Feb 18, 2022
@sloretz
Copy link
Contributor Author

sloretz commented Feb 18, 2022

CI (build: --packages-up-to rosidl_adapter rosidl_cli rosidl_cmake rosidl_default_generators rosidl_default_runtime rosidl_generator_c rosidl_generator_cpp rosidl_generator_dds_idl rosidl_generator_py rosidl_parser rosidl_runtime_c rosidl_runtime_cpp rosidl_runtime_py rosidl_typesupport_c rosidl_typesupport_connext_c rosidl_typesupport_connext_cpp rosidl_typesupport_cpp rosidl_typesupport_fastrtps_c rosidl_typesupport_fastrtps_cpp rosidl_typesupport_interface rosidl_typesupport_introspection_c rosidl_typesupport_introspection_cpp rosidl_typesupport_introspection_tests test: --packages-select-regex "rosidl.*") Edit: long build args because I didn't realize there's also --packages-up-to-regex 🙃

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

So looking at https://ci.ros2.org/view/nightly/job/nightly_linux_debug/2210/, for instance, I don't see a failing test. Do we need to add one to trigger this?

(otherwise, the fix itself looks good to me)

@sloretz
Copy link
Contributor Author

sloretz commented Feb 18, 2022

So looking at https://ci.ros2.org/view/nightly/job/nightly_linux_debug/2210/, for instance, I don't see a failing test. Do we need to add one to trigger this?

No. The rosidl_parser tests fail on Jammy. I don't know why it's not failing in that job.

@sloretz
Copy link
Contributor Author

sloretz commented Feb 18, 2022

CI attempt 2 (build: --packages-up-to-regex "rosidl.*" test: --packages-select-regex "rosidl.*")

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@sloretz
Copy link
Contributor Author

sloretz commented Feb 18, 2022

Ah, I figured it out. That job seems to be installing lark from pip instead of apt.

Collecting lark-parser
  Downloading lark_parser-0.12.0-py2.py3-none-any.whl

<exec_depend>python3-lark-parser</exec_depend>

https://github.com/ros/rosdistro/blob/028e5351fc191b3c25555607776a6f33c2215c1d/rosdep/python.yaml#L6882-L6896

@clalancette
Copy link
Contributor

OK, I'm able to reproduce the problem locally as well.

Oh, I think I know what the problem is. CI is installing lark-parser from pip, while I locally just have lark (installed from the Ubuntu python3-lark package). So we aren't testing the same things. I'll do an update to the CI scripts so we stop doing this, and always use lark from the system if possible.

@sloretz
Copy link
Contributor Author

sloretz commented Feb 18, 2022

Oh, I think I know what the problem is. CI is installing lark-parser from pip, while I locally just have lark (installed from the Ubuntu python3-lark package). So we aren't testing the same things. I'll do an update to the CI scripts so we stop doing this, and always use lark from the system if possible.

One more thing, it looks like the PyPI package is now lark: https://pypi.org/project/lark/ instead of lark-parser. That's mentioned on the project readme too https://github.com/lark-parser/lark#install-lark edit: for installing it on windows.

@sloretz
Copy link
Contributor Author

sloretz commented Feb 18, 2022

PR job test failure seems unrelated - maybe a bug in osrf_testing_tools_cpp?

9: Test command: /usr/bin/python3.9 "-u" "/opt/ros/rolling/share/ament_cmake_test/cmake/run_test.py" "/tmp/ws/test_results/rosidl_runtime_c/cppcheck.xunit.xml" "--package-name" "rosidl_runtime_c" "--output-file" "/tmp/ws/build_isolated/rosidl_runtime_c/ament_cppcheck/cppcheck.txt" "--command" "/opt/ros/rolling/bin/ament_cppcheck" "--xunit-file" "/tmp/ws/test_results/rosidl_runtime_c/cppcheck.xunit.xml" "--include_dirs" "/opt/ros/rolling/include" "/tmp/ws/src/rosidl/rosidl_runtime_c/include"
9: Test timeout computed to be: 300
9: -- run_test.py: invoking following command in '/tmp/ws/src/rosidl/rosidl_runtime_c':
9:  - /opt/ros/rolling/bin/ament_cppcheck --xunit-file /tmp/ws/test_results/rosidl_runtime_c/cppcheck.xunit.xml --include_dirs /opt/ros/rolling/include /tmp/ws/src/rosidl/rosidl_runtime_c/include
9: [/opt/ros/rolling/include/osrf_testing_tools_cpp/variant_helper.hpp:20]: (error: preprocessorErrorDirective) failed to evaluate #if condition
9: 1 errors
9: -- run_test.py: return code 1
9: -- run_test.py: verify result file '/tmp/ws/test_results/rosidl_runtime_c/cppcheck.xunit.xml'
 9/13 Test  #9: cppcheck .............................***Failed    1.65 sec

@clalancette
Copy link
Contributor

PR job test failure seems unrelated - maybe a bug in osrf_testing_tools_cpp?

Ah, no. cppcheck 2.x on Jammy is broken. I have a patch into ament_lint that fixes it, but we haven't done a release of that yet. I'll do that soon.

@sloretz
Copy link
Contributor Author

sloretz commented Feb 18, 2022

CI LGTM 🎉

@sloretz sloretz merged commit 90a74c3 into master Feb 18, 2022
@delete-merged-branch delete-merged-branch bot deleted the sloretz__lark_1_maybe_placeholders branch February 18, 2022 19:46
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.

StopIteration raised in rosidl_parser tests on Ubuntu Jammy
2 participants