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

rosidl_adapter crashes if .msg file begins with lower case letter #414

Closed
TimCraig opened this issue Sep 15, 2019 · 4 comments
Closed

rosidl_adapter crashes if .msg file begins with lower case letter #414

TimCraig opened this issue Sep 15, 2019 · 4 comments
Labels
enhancement New feature or request

Comments

@TimCraig
Copy link

TimCraig commented Sep 15, 2019

Bug report

Required Info:

  • Operating System:
    Ubuntu MATE 18.04
  • Installation type:
    binaries
  • Version or commit hash:
    0.7.5-1bionic.20190823.205203
  • DDS implementation:
    Fast-RTPS
  • Client library (if applicable):
    N/A

Steps to reproduce issue

Simply start the ,msg filename with a lower case letter, eg, druai.msg rather than Druai.msg


Expected behavior

If the filename capitalization is an actual requirement, I would expect the parser to generate a reasonable error message and possibly point to the document defining the requirement.

Actual behavior

The parser Python program crashes with error messages that are not helpful the an end user.

Starting >>> druai_msg
--- stderr: druai_msg
CMake Error at /opt/ros/dashing/share/rosidl_adapter/cmake/rosidl_adapt_interfaces.cmake:60 (message):
execute_process(/usr/bin/python3 -m rosidl_adapter --package-name druai_msg
--arguments-file
/home/brutusdev/ros2/test_ws/build/druai_msg/rosidl_adapter__arguments__druai_msg.json
--output-dir
/home/brutusdev/ros2/test_ws/build/druai_msg/rosidl_adapter/druai_msg
--output-file
/home/brutusdev/ros2/test_ws/build/druai_msg/rosidl_adapter/druai_msg.idls)
returned error code 1:

Traceback (most recent call last):

File "/usr/lib/python3.6/runpy.py", line 193, in _run_module_as_main
  "__main__", mod_spec)
File "/usr/lib/python3.6/runpy.py", line 85, in _run_code
  exec(code, run_globals)
File "/opt/ros/dashing/lib/python3.6/site-packages/rosidl_adapter/__main__.py", line 19, in <module>
  sys.exit(main())
File "/opt/ros/dashing/lib/python3.6/site-packages/rosidl_adapter/main.py", line 55, in main
  pathlib.Path(relative_path), output_dir)
File "/opt/ros/dashing/lib/python3.6/site-packages/rosidl_adapter/__init__.py", line 20, in convert_to_idl
  package_dir, package_name, interface_file, output_dir / 'msg')
File "/opt/ros/dashing/lib/python3.6/site-packages/rosidl_adapter/msg/__init__.py", line 28, in convert_msg_to_idl
  msg = parse_message_string(package_name, input_file.stem, content)
File "/opt/ros/dashing/lib/python3.6/site-packages/rosidl_adapter/parser.py", line 522, in parse_message_string
  msg = MessageSpecification(pkg_name, msg_name, fields, constants)
File "/opt/ros/dashing/lib/python3.6/site-packages/rosidl_adapter/parser.py", line 373, in __init__
  pkg_name + PACKAGE_NAME_MESSAGE_TYPE_SEPARATOR + msg_name)
File "/opt/ros/dashing/lib/python3.6/site-packages/rosidl_adapter/parser.py", line 196, in __init__
  raise InvalidResourceName(self.type)

rosidl_adapter.parser.InvalidResourceName: druai

Call Stack (most recent call first):
/opt/ros/dashing/share/rosidl_cmake/cmake/rosidl_generate_interfaces.cmake:130 (rosidl_adapt_interfaces)
CMakeLists.txt:24 (rosidl_generate_interfaces)

Additional information

@jacobperron
Copy link
Member

jacobperron commented Sep 16, 2019

In this design article, it is described that interface definitions have upper-case CamelCase, by convention. Perhaps we should update the article so that it is a requirement. Though, I'm not sure why it is a requirement (maybe for IDL compatibility). We could uncomment the following line to follow ROS 1 naming patterns:

# relaxed patterns used for compatibility with ROS 1 messages
# VALID_MESSAGE_NAME_PATTERN = re.compile('^[A-Za-z][A-Za-z0-9]*$')

But, for now, I've opened a PR to improve the error message (#415)

@mabelzhang mabelzhang added the enhancement New feature or request label Sep 19, 2019
@jacobperron
Copy link
Member

@ros2/team What is the ultimate goal? Do we want to maintain compatibility with ROS 1 interfaces that can have camelCase, or strictly only allow CamelCase?

@dirk-thomas
Copy link
Member

In this design article, it is described that interface definitions have upper-case CamelCase, by convention. Perhaps we should update the article so that it is a requirement.

The current design article already uses must: Both file names must use an upper camel case name and only consist of alphanumeric characters.

The rational why that was chosen is that all names (the msg filename, the generated source filename, as well as the class name) are the same.

If we want to drop that requirement we will have to accept potential collisions since two entities with different case can exist.

@jacobperron
Copy link
Member

The current design article already uses must: Both file names must use an upper camel case name and only consist of alphanumeric characters.

I was confused because the title of the section is "Conventions". Thanks for the response!

I'll close this issue then since the error is by design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants