Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
4 changes: 4 additions & 0 deletions rosidl_generator_py/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ ament_python_install_package(${PROJECT_NAME})
if(BUILD_TESTING)
find_package(ament_cmake_pytest REQUIRED)

find_package(builtin_interfaces REQUIRED)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I will run CI to double check. But I'm pretty sure this won't work since this would cause a circular package order. We might need to make another pr before this splitting the tests into there own rosidl_generator_py_tests or something to avoid this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah CI complains about being unable to topologically order the package after adding builtin_interfaces.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good catch, fixed in 3dd9104. I removed the builtin_interfaces test dependency and moved the regression coverage to a template-level pytest so this no longer affects package ordering.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Confirmed, fixed in 3dd9104 by removing the generated builtin_interfaces test interfaces and the CMake/package.xml dependency that caused the topological cycle.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Also addressed in 3dd9104: the package-order failure is gone because the PR no longer adds builtin_interfaces to rosidl_generator_py CMake/package test dependencies.

find_package(rmw REQUIRED)
find_package(rosidl_cmake REQUIRED)

Expand Down Expand Up @@ -47,8 +48,11 @@ if(BUILD_TESTING)
${test_interface_files_MSG_FILES}
# Cases not covered by test_interface_files
msg/BuiltinTypeSequencesIdl.idl
msg/Duration.msg
msg/DurationArraySequence.msg
msg/StringArrays.msg
msg/Property.msg
DEPENDENCIES builtin_interfaces
ADD_LINTER_TESTS
SKIP_INSTALL
)
Expand Down
1 change: 1 addition & 0 deletions rosidl_generator_py/msg/Duration.msg
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
builtin_interfaces/Duration data
2 changes: 2 additions & 0 deletions rosidl_generator_py/msg/DurationArraySequence.msg
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
builtin_interfaces/Duration[2] array_data
builtin_interfaces/Duration[] sequence_data
1 change: 1 addition & 0 deletions rosidl_generator_py/package.xml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
<test_depend>ament_index_python</test_depend>
<test_depend>ament_lint_auto</test_depend>
<test_depend>ament_lint_common</test_depend>
<test_depend>builtin_interfaces</test_depend>
<test_depend>python3-numpy</test_depend>
<test_depend>python3-pytest</test_depend>
<test_depend>rmw</test_depend>
Expand Down
71 changes: 38 additions & 33 deletions rosidl_generator_py/resource/_msg.py.em
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,27 @@ from rosidl_parser.definition import NamespacedType
from rosidl_parser.definition import SIGNED_INTEGER_TYPES
from rosidl_parser.definition import UnboundedSequence
from rosidl_parser.definition import UNSIGNED_INTEGER_TYPES


def get_importable_namespaced_type(
type_: NamespacedType,
action_goal_suffix: str = ACTION_GOAL_SUFFIX,
action_result_suffix: str = ACTION_RESULT_SUFFIX,
action_feedback_suffix: str = ACTION_FEEDBACK_SUFFIX,
camel_to_snake=convert_camel_case_to_lower_case_underscore,
) -> tuple[str, str]:
joined_type_namespaces = '.'.join(type_.namespaces)
if (
type_.name.endswith(action_goal_suffix) or
type_.name.endswith(action_result_suffix) or
type_.name.endswith(action_feedback_suffix)
):
action_name, _ = type_.name.rsplit('_', 1)
lower_case_name = camel_to_snake(action_name)
module_name = f'{joined_type_namespaces}._{lower_case_name}'
else:
module_name = joined_type_namespaces
return module_name, f'{module_name}.{type_.name}'
}@
@{
import_type_checking = False
Expand Down Expand Up @@ -182,22 +203,13 @@ for member in message.structure.members:
type_.name.endswith(SERVICE_REQUEST_MESSAGE_SUFFIX)
):
continue
if (
type_.name.endswith(ACTION_GOAL_SUFFIX) or
type_.name.endswith(ACTION_RESULT_SUFFIX) or
type_.name.endswith(ACTION_FEEDBACK_SUFFIX)
):
action_name, suffix = type_.name.rsplit('_', 1)
typename = (*type_.namespaces, action_name, action_name + '.' + suffix)
else:
typename = (*type_.namespaces, type_.name, type_.name)
importable_typesupports.add(typename)
importable_typesupports.add(get_importable_namespaced_type(type_))
}@
@[for typename in sorted(importable_typesupports)]@
@[for module_name, type_name in sorted(importable_typesupports)]@

from @('.'.join(typename[:-2])) import @(typename[-2])
if @(typename[-1])._TYPE_SUPPORT is None:
@(typename[-1]).__import_type_support__()
import @(module_name)
if @(type_name)._TYPE_SUPPORT is None:
@(type_name).__import_type_support__()
@[end for]@

@@classmethod
Expand Down Expand Up @@ -370,15 +382,10 @@ if isinstance(type_, AbstractNestedType):
self.@(member.name) = @(member.name) if @(member.name) is not None else @(message.structure.namespaced_type.name).@(member.name.upper())__DEFAULT
@[ else]@
@[ if isinstance(type_, NamespacedType) and not isinstance(member.type, AbstractSequence)]@
@[ if (
type_.name.endswith(ACTION_GOAL_SUFFIX) or
type_.name.endswith(ACTION_RESULT_SUFFIX) or
type_.name.endswith(ACTION_FEEDBACK_SUFFIX)
)]@
from @('.'.join(type_.namespaces))._@(convert_camel_case_to_lower_case_underscore(type_.name.rsplit('_', 1)[0])) import @(type_.name)
@[ else]@
from @('.'.join(type_.namespaces)) import @(type_.name)
@[ end if]@
@{
module_name, type_name = get_importable_namespaced_type(type_)
}@
import @(module_name)
@[ end if]@
@[ if isinstance(member.type, Array)]@
@[ if isinstance(type_, BasicType) and type_.typename == 'octet']@
Expand All @@ -392,7 +399,8 @@ if isinstance(type_, AbstractNestedType):
else:
self.@(member.name) = @(member.name)
@[ else]@
self.@(member.name) = @(member.name) if @(member.name) is not None else [@(get_python_type(type_))() for x in range(@(member.type.size))]
@{default_type = type_name if isinstance(type_, NamespacedType) else get_python_type(type_)}@
self.@(member.name) = @(member.name) if @(member.name) is not None else [@(default_type)() for x in range(@(member.type.size))]
@[ end if]@
@[ end if]@
@[ elif isinstance(member.type, AbstractSequence)]@
Expand All @@ -405,6 +413,8 @@ if isinstance(type_, AbstractNestedType):
self.@(member.name) = @(member.name) if @(member.name) is not None else bytes([0])
@[ elif isinstance(type_, BasicType) and type_.typename in CHARACTER_TYPES]@
self.@(member.name) = @(member.name) if @(member.name) is not None else chr(0)
@[ elif isinstance(type_, NamespacedType)]@
self.@(member.name) = @(member.name) if @(member.name) is not None else @(type_name)()
@[ else]@
self.@(member.name) = @(member.name) if @(member.name) is not None else @(get_python_type(type_))()
@[ end if]@
Expand Down Expand Up @@ -504,15 +514,10 @@ if isinstance(member.type, (Array, AbstractSequence)):
@[ end if]@
@[ end if]@
@[ if isinstance(type_, NamespacedType)]@
@[ if (
type_.name.endswith(ACTION_GOAL_SUFFIX) or
type_.name.endswith(ACTION_RESULT_SUFFIX) or
type_.name.endswith(ACTION_FEEDBACK_SUFFIX)
)]@
from @('.'.join(type_.namespaces))._@(convert_camel_case_to_lower_case_underscore(type_.name.rsplit('_', 1)[0])) import @(type_.name)
@[ else]@
from @('.'.join(type_.namespaces)) import @(type_.name)
@[ end if]@
@{
module_name, type_name = get_importable_namespaced_type(type_)
}@
import @(module_name)
@[ end if]@

@{
Expand Down
36 changes: 33 additions & 3 deletions rosidl_generator_py/resource/_msg_check_fields.py.em
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ from rosidl_parser.definition import AbstractGenericString
from rosidl_parser.definition import AbstractNestedType
from rosidl_parser.definition import AbstractSequence
from rosidl_parser.definition import Array
from rosidl_parser.definition import ACTION_FEEDBACK_SUFFIX
from rosidl_parser.definition import ACTION_GOAL_SUFFIX
from rosidl_parser.definition import ACTION_RESULT_SUFFIX
from rosidl_parser.definition import BasicType
from rosidl_parser.definition import BOOLEAN_TYPE
from rosidl_parser.definition import INTEGER_TYPES
Expand All @@ -11,8 +14,35 @@ from rosidl_parser.definition import FLOATING_POINT_TYPES
from rosidl_parser.definition import SIGNED_INTEGER_TYPES
from rosidl_parser.definition import UNSIGNED_INTEGER_TYPES
from rosidl_parser.definition import NamespacedType
from rosidl_pycommon import convert_camel_case_to_lower_case_underscore
from rosidl_generator_py.generate_py_impl import get_python_type
from rosidl_generator_py.generate_py_impl import SPECIAL_NESTED_BASIC_TYPES


def get_importable_namespaced_type(
type_: NamespacedType,
action_goal_suffix: str = ACTION_GOAL_SUFFIX,
action_result_suffix: str = ACTION_RESULT_SUFFIX,
action_feedback_suffix: str = ACTION_FEEDBACK_SUFFIX,
camel_to_snake=convert_camel_case_to_lower_case_underscore,
) -> tuple[str, str]:
joined_type_namespaces = '.'.join(type_.namespaces)
if (
type_.name.endswith(action_goal_suffix) or
type_.name.endswith(action_result_suffix) or
type_.name.endswith(action_feedback_suffix)
):
action_name, _ = type_.name.rsplit('_', 1)
lower_case_name = camel_to_snake(action_name)
module_name = f'{joined_type_namespaces}._{lower_case_name}'
else:
module_name = joined_type_namespaces
return module_name, f'{module_name}.{type_.name}'
}@
@{
python_type = get_python_type(type_)
if isinstance(type_, NamespacedType):
_, python_type = get_importable_namespaced_type(type_)
}@
if self._check_fields:
@[ if isinstance(member.type, AbstractNestedType)]@
Expand Down Expand Up @@ -61,8 +91,8 @@ from rosidl_generator_py.generate_py_impl import SPECIAL_NESTED_BASIC_TYPES
@{assert_msg_suffixes.insert(1, 'with length %d' % member.type.size)}@
@[ end if]@
@[ end if]@
all(isinstance(v, @(get_python_type(type_))) for v in value) and
@{assert_msg_suffixes.append("and each value of type '%s'" % get_python_type(type_))}@
all(isinstance(v, @(python_type)) for v in value) and
@{assert_msg_suffixes.append("and each value of type '%s'" % python_type)}@
@[ if isinstance(type_, BasicType) and type_.typename in SIGNED_INTEGER_TYPES]@
@{
nbits = int(type_.typename[3:])
Expand Down Expand Up @@ -106,7 +136,7 @@ bound = 1.7976931348623157e+308
"The '@(member.name)' field must be string value " \
'not longer than @(type_.maximum_size)'
@[ elif isinstance(type_, NamespacedType)]@
isinstance(value, @(type_.name)), \
isinstance(value, @(python_type)), \
"The '@(member.name)' field must be a sub message of type '@(type_.name)'"
@[ elif isinstance(type_, BasicType) and type_.typename == 'octet']@
(isinstance(value, (bytes, bytearray, memoryview)) and
Expand Down
45 changes: 45 additions & 0 deletions rosidl_generator_py/test/test_interfaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import array
import math
import pathlib
import sys

import numpy
Expand All @@ -25,6 +26,8 @@
from rosidl_generator_py.msg import BuiltinTypeSequencesIdl
from rosidl_generator_py.msg import Constants
from rosidl_generator_py.msg import Defaults
from rosidl_generator_py.msg import Duration
from rosidl_generator_py.msg import DurationArraySequence
from rosidl_generator_py.msg import Nested
from rosidl_generator_py.msg import StringArrays
from rosidl_generator_py.msg import Strings
Expand Down Expand Up @@ -269,6 +272,48 @@ def test_constructor() -> None:
Strings(unknown_field='test', check_fields=True)


def test_namespaced_field_imports_are_absolute() -> None:
duration_module = sys.modules[Duration.__module__]
duration_file = duration_module.__file__
assert duration_file is not None
duration_source = pathlib.Path(duration_file).read_text(encoding='utf-8')

assert 'from builtin_interfaces.msg import Duration' not in duration_source
assert 'import builtin_interfaces.msg' in duration_source
assert 'builtin_interfaces.msg.Duration._TYPE_SUPPORT' in duration_source

from builtin_interfaces.msg import Duration as BuiltinDuration

Duration.__import_type_support__()

msg = Duration(check_fields=True)
assert isinstance(msg.data, BuiltinDuration)


def test_namespaced_array_sequence_fields_are_absolute() -> None:
duration_module = sys.modules[DurationArraySequence.__module__]
duration_file = duration_module.__file__
assert duration_file is not None
duration_source = pathlib.Path(duration_file).read_text(encoding='utf-8')

assert 'from builtin_interfaces.msg import Duration' not in duration_source
assert 'import builtin_interfaces.msg' in duration_source

from builtin_interfaces.msg import Duration as BuiltinDuration

msg = DurationArraySequence(check_fields=True)
assert all(isinstance(value, BuiltinDuration) for value in msg.array_data)
assert [] == msg.sequence_data

msg.array_data = [BuiltinDuration(sec=1), BuiltinDuration(sec=2)]
msg.sequence_data = [BuiltinDuration(sec=3)]

with pytest.raises(AssertionError):
msg.array_data = [BuiltinDuration(), object()]
with pytest.raises(AssertionError):
msg.sequence_data = [object()]


def test_constants() -> None:
assert Constants.BOOL_CONST is True
assert bytes([50]) == Constants.BYTE_CONST
Expand Down