Fix proto2ros package vendoring (#120)

Signed-off-by: Michel Hidalgo <[email protected]>

proto2ros/cmake/proto2ros_generate.cmake

@@ -149,7 +149,7 @@ function(proto2ros_generate target)
   set(python_sources ${output_files})
   list(FILTER python_sources INCLUDE REGEX ".*\.py$")
   set(cpp_sources ${output_files})
-  list(FILTER cpp_sources INCLUDE REGEX ".*\.cpp$")
+  list(FILTER cpp_sources INCLUDE REGEX ".*\.[ch]pp$")
 
   if(BUILD_TESTING AND NOT ARG_NO_LINT AND ament_cmake_mypy_FOUND)
     set(MYPY_PATH "${APPEND_PYTHONPATH}:$ENV{PYTHONPATH}")

proto2ros/cmake/proto2ros_vendor_package.cmake

@@ -15,13 +15,30 @@
 macro(proto2ros_vendor_package target)
   set(options NO_LINT)
   set(one_value_keywords PACKAGE_NAME)
-  set(multi_value_keywords PROTOS IMPORT_DIRS CONFIG_OVERLAYS ROS_DEPENDENCIES PYTHON_MODULES PYTHON_PACKAGES)
+  set(multi_value_keywords PROTOS IMPORT_DIRS CONFIG_OVERLAYS ROS_DEPENDENCIES CPP_DEPENDENCIES CPP_INCLUDES CPP_SOURCES PYTHON_MODULES PYTHON_PACKAGES)
   cmake_parse_arguments(ARG "${options}" "${one_value_keywords}" "${multi_value_keywords}" ${ARGN})
 
   if(NOT ARG_PACKAGE_NAME)
     set(ARG_PACKAGE_NAME ${PROJECT_NAME})
   endif()
 
+  get_filename_component(package_path "${ARG_PACKAGE_NAME}" ABSOLUTE)
+  if(EXISTS "${package_path}/__init__.py")
+    list(APPEND ARG_PYTHON_PACKAGES ${ARG_PACKAGE_NAME})
+  endif()
+
+  if(NOT ARG_CPP_INCLUDES)
+    if(IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/include/${ARG_PACKAGE_NAME}")
+      list(APPEND ARG_CPP_INCLUDES "${CMAKE_CURRENT_SOURCE_DIR}/include")
+    endif()
+  endif()
+
+  if(NOT ARG_CPP_SOURCES)
+    if(IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/include/${ARG_PACKAGE_NAME}" AND IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/src")
+      file(GLOB ARG_CPP_SOURCES "${CMAKE_CURRENT_SOURCE_DIR}/src/*.cpp" "${CMAKE_CURRENT_SOURCE_DIR}/src/*.cc")
+    endif()
+  endif()
+
   set(proto2ros_generate_OPTIONS)
   if(ARG_NO_LINT)
     list(APPEND proto2ros_generate_OPTIONS NO_LINT)
@@ -35,7 +52,8 @@ macro(proto2ros_vendor_package target)
     CONFIG_OVERLAYS ${ARG_CONFIG_OVERLAYS}
     INTERFACES_OUT_VAR ros_messages
     PYTHON_OUT_VAR py_sources
-    CPP_OUT_VAR cpp_sources
+    CPP_OUT_VAR generated_cpp_sources
+    INCLUDE_OUT_VAR generated_cpp_include_dir
     ${proto2ros_generate_OPTIONS}
   )
 
@@ -45,39 +63,62 @@ macro(proto2ros_vendor_package target)
   )
   add_dependencies(${target} ${target}_messages_gen)
 
-  get_filename_component(package_path "${ARG_PACKAGE_NAME}" ABSOLUTE)
-  if(EXISTS "${package_path}/__init__.py")
-    list(APPEND ARG_PYTHON_PACKAGES ${ARG_PACKAGE_NAME})
-  endif()
-
   rosidl_generated_python_package_add(
     ${target}_additional_modules
     MODULES ${ARG_PYTHON_MODULES} ${py_sources}
     PACKAGES ${ARG_PYTHON_PACKAGES}
     DESTINATION ${target}
   )
 
-  find_package(rclcpp REQUIRED)
-  add_library(${target}_conversions ${cpp_sources})
+  add_library(${target}_conversions SHARED ${generated_cpp_sources} ${ARG_CPP_SOURCES})
+  target_compile_features(${target}_conversions PRIVATE cxx_std_17)
+  # NOTE: conversion APIs cannot ignore deprecated fields, so deprecation warnings must be disabled
+  target_compile_options(${target}_conversions PRIVATE -Wno-deprecated -Wno-deprecated-declarations)
+  list(APPEND build_include_directories "$<BUILD_INTERFACE:${generated_cpp_include_dir}>")
+  foreach(cpp_include_dir ${ARG_CPP_INCLUDES})
+    list(APPEND build_include_directories "$<BUILD_INTERFACE:${cpp_include_dir}>")
+  endforeach()
+  target_include_directories(${target}_conversions PUBLIC
+    ${build_include_directories} "$<INSTALL_INTERFACE:include/${PROJECT_NAME}>"
+  )
   rosidl_get_typesupport_target(cpp_interfaces ${target} "rosidl_typesupport_cpp")
-  target_link_libraries(${target}_conversions ${cpp_interfaces})
+  target_link_libraries(${target}_conversions ${cpp_interfaces} ${ARG_CPP_DEPENDENCIES})
   ament_target_dependencies(${target}_conversions
     ${ARG_ROS_DEPENDENCIES} builtin_interfaces proto2ros rclcpp)
 
-  set(header_files ${cpp_sources})
-  list(FILTER header_files INCLUDE REGEX ".*\.hpp$")
+  find_program(CLANG_TIDY_EXECUTABLE NAMES "clang-tidy")
+  if(BUILD_TESTING AND NOT ARG_NO_LINT AND CLANG_TIDY_EXECUTABLE)
+    list(APPEND clang_tidy_header_regexes "${generated_cpp_include_dir}/.*hpp")
+    foreach(cpp_include_dir ARG_CPP_INCLUDES)
+      list(APPEND clang_tidy_header_regexes "${cpp_include_dir}/.*hpp")
+    endforeach()
+    list(JOIN clang_tidy_header_regexes "|" clang_tidy_header_filter)
+    set(CXX_CLANG_TIDY "${CLANG_TIDY_EXECUTABLE}"
+      "-header-filter='^(${clang_tidy_header_filter})$'"
+      "-checks=-clang-diagnostic-ignored-optimization-argument")
+    set_target_properties(${target}_conversions PROPERTIES
+      CXX_STANDARD 17 CXX_STANDARD_REQUIRED ON CXX_CLANG_TIDY "${CXX_CLANG_TIDY}")
+  endif()
+
+  set(generated_header_files ${generated_cpp_sources})
+  list(FILTER generated_header_files INCLUDE REGEX ".*\.hpp$")
   install(
-    FILES ${header_files}
-    DESTINATION include/${PROJECT_NAME}
+    FILES ${generated_header_files}
+    DESTINATION include/${PROJECT_NAME}/${ARG_PACKAGE_NAME}/
   )
-  ament_export_include_directories("include/${PROJECT_NAME}")
+  foreach(cpp_include_dir ${ARG_CPP_INCLUDES})
+    install(
+      DIRECTORY ${cpp_include_dir}/
+      DESTINATION include/${PROJECT_NAME}/
+    )
+  endforeach()
 
   install(
     TARGETS ${target}_conversions
-    EXPORT export_${target}_conversions
+    EXPORT ${PROJECT_NAME}
     ARCHIVE DESTINATION lib
     LIBRARY DESTINATION lib
     RUNTIME DESTINATION bin
   )
-  ament_export_targets(export_${target}_conversions)
+  ament_export_targets(${PROJECT_NAME})
 endmacro()

proto2ros/package.xml

@@ -16,7 +16,7 @@ Copyright (c) 2023 Boston Dynamics AI Institute LLC. All rights reserved.
   <buildtool_export_depend>rosidl_default_generators</buildtool_export_depend>
   <buildtool_export_depend>ament_cmake_mypy</buildtool_export_depend>
 
-  <depend>clang-tidy</depend>
+  <build_depend>clang-tidy</build_depend>
 
   <depend>protobuf</depend>
   <depend>protobuf-dev</depend>

proto2ros/proto2ros/equivalences.py

@@ -295,7 +295,8 @@ def translate_field(
         field.annotations["optional"] = descriptor.label != FieldDescriptorProto.LABEL_REPEATED
     else:
         raise ValueError(f"unknown proto syntax: {source.syntax}")
-    field.annotations["proto-name"] = descriptor.name
+    field.annotations["proto-cpp-name"] = descriptor.name.lower()
+    field.annotations["proto-py-name"] = descriptor.name
     ros_type_name = to_ros_base_type(field_type)
     if type_name != ".google.protobuf.Any" and ros_type_name == "proto2ros/AnyProto":
         type_name = "some"
@@ -404,6 +405,8 @@ def compute_equivalence_for_message(
             field = Field(oneof_type, oneof_name)
             # oneof wrapper field cannot itself be optional
             field.annotations["optional"] = False
+            field.annotations["proto-cpp-name"] = oneof_decl.name
+            field.annotations["proto-py-name"] = oneof_decl.name
             oneof_location = resolve(source, oneof_path, location)
             leading_comments = extract_leading_comments(oneof_location)
             if leading_comments:

proto2ros/proto2ros/output/cpp.py

@@ -3,14 +3,15 @@
 """This module provides APIs to write C++ conversion code."""
 
 import os
+import re
 from typing import List, Optional, Tuple, Union
 
 import inflection
 import jinja2
 from rosidl_adapter.parser import BaseType, MessageSpecification
 
 from proto2ros.configuration import Configuration
-from proto2ros.utilities import rreplace, to_ros_base_type
+from proto2ros.utilities import rreplace, to_protobuf_field_name, to_ros_base_type
 
 
 def to_pb2_cpp_header(source_path: os.PathLike) -> str:
@@ -62,7 +63,7 @@ def to_hungarian_notation(name: str) -> str:
 
     E.g. some_name_of_mine translates to kSomeNameOfMine.
     """
-    return "k" + inflection.camelize(name)
+    return "k" + inflection.camelize(re.sub(r"([0-9])([a-z])", r"\1_\2", name))
 
 
 def dump_conversions_cpp_sources(
@@ -85,12 +86,14 @@ def dump_conversions_cpp_sources(
         the conversion C++ header and source files' content, in that order.
     """
     env = jinja2.Environment(loader=jinja2.PackageLoader("proto2ros.output"))
+    env.globals["to_pb2_cpp_name"] = to_protobuf_field_name
     env.globals["itemize_cpp_identifier"] = itemize_cpp_identifier
     env.globals["to_ros_base_type"] = to_ros_base_type
     env.globals["rreplace"] = rreplace
     env.filters["as_ros_base_type"] = to_ros_base_type
     env.filters["as_ros_cpp_type"] = to_ros_cpp_type
     env.filters["as_pb2_cpp_type"] = to_pb2_cpp_type
+    env.filters["as_pb2_cpp_name"] = to_protobuf_field_name
     env.filters["to_hungarian_notation"] = to_hungarian_notation
     env.filters["rreplace"] = rreplace
     cpp_conversions_header_template = env.get_template("conversions.hpp.jinja")

proto2ros/proto2ros/output/templates/conversions.cpp.jinja

@@ -47,9 +47,9 @@ for (const auto& {{ input_item }} : {{ source }}) {
     auto* {{ output_item }} = &(*{{ destination }})[{{ input_item }}.key];
     {% for field_spec in type_spec.fields if field_spec.name == "value" %}
         {%- if not field_spec.type.is_primitive_type() -%}
-            {%- set field_type_spec = known_message_specifications.get(to_ros_base_type(spec.type)) -%}
+            {%- set field_type_spec = known_message_specifications.get(to_ros_base_type(field_spec.type)) -%}
             {%- if field_type_spec and field_type_spec.annotations.get("proto-class") == "enum" -%}
-    *{{ output_item }} = static_cast<{{ field_type_spec.annotations["proto-type"] | as_pb2_cpp_type }}>({{ input_item }}.value);
+    *{{ output_item }} = static_cast<{{ field_type_spec.annotations["proto-type"] | as_pb2_cpp_type }}>({{ input_item }}.value.value);
             {%- else -%}{#- Handle message case. -#}
     {{ ros_to_proto_composite_field_code_block(input_item + ".value", output_item, field_spec) | indent(4) }}
             {%- endif -%}
@@ -169,7 +169,7 @@ if ({{ source }}.type_name == "{{ ros_type_name }}") {
 switch ({{ source }}.{{ tag_field_spec.name }} != 0 ? {{ source }}.{{ tag_field_spec.name }} : {{ source }}.{{ tag_field_spec.annotations["alias"] }}) {
         {%- for tag_spec, member_spec in type_spec.annotations["tagged"] -%}
             {%- set source_member = source + "." + member_spec.name -%}
-            {%- set destination_member = destination.rpartition("->")[0] + "->mutable_" + member_spec.annotations.get("proto-name", member_spec.name) + "()" %}
+            {%- set destination_member = destination.rpartition("->")[0] + "->mutable_" + to_pb2_cpp_name(member_spec.annotations.get("proto-cpp-name", member_spec.name)) + "()" %}
     case {{ type_spec.base_type | string | as_ros_cpp_type }}::{{ tag_spec.name }}:
         {{ ros_to_proto_field_code(source_member, destination_member, member_spec) | indent(8) }}
         break;
@@ -324,9 +324,9 @@ for (const auto& blob : {{ source }}) {
 switch ({{ source_case }}) {
         {%- for tag_spec, member_spec in type_spec.annotations["tagged"] %}
             {%- set parent_type_spec = type_spec.annotations["parent-spec"] -%}
-            {%- set source_member = source_parent + "." + member_spec.annotations.get("proto-name", member_spec.name) + "()" -%}
+            {%- set source_member = source_parent + "." + to_pb2_cpp_name(member_spec.annotations.get("proto-cpp-name", member_spec.name)) + "()" -%}
             {%- set destination_member = destination + "." + member_spec.name %}
-    case {{ parent_type_spec.annotations["proto-type"] | as_pb2_cpp_type }}::{{ member_spec.name | to_hungarian_notation }}:
+    case {{ parent_type_spec.annotations["proto-type"] | as_pb2_cpp_type }}::{{ member_spec.annotations.get("proto-cpp-name", member_spec.name) | to_hungarian_notation }}:
         {{ proto_to_ros_field_code(source_member, destination_member, member_spec) | indent(8) }}
         {{ destination }}.{{ tag_field_spec.name }} = {{ type_spec.base_type | string | as_ros_cpp_type }}::{{ tag_spec.name }};
         {{ destination }}.{{ tag_field_spec.annotations["alias"] }} = {{ destination }}.{{ tag_field_spec.name }};
@@ -356,7 +356,7 @@ void Convert(const {{ spec.base_type | string | as_ros_cpp_type }}& ros_msg, {{
     proto_msg->Clear();
         {%- for field_spec in spec.fields if field_spec.name != "has_field" %}
             {%- set source = "ros_msg." + field_spec.name -%}
-            {%- set destination = "proto_msg->mutable_" + field_spec.annotations.get("proto-name", field_spec.name).lower() + "()" -%}
+            {%- set destination = "proto_msg->mutable_" + to_pb2_cpp_name(field_spec.annotations.get("proto-cpp-name", field_spec.name)) + "()" -%}
             {%- if field_spec.annotations["optional"] %}{#- Check for field presence before use. #}
     if ((ros_msg.has_field & {{ spec.base_type | string | as_ros_cpp_type }}::{{ field_spec.name | upper }}_FIELD_SET) != 0) {
         {{ ros_to_proto_field_code(source, destination, field_spec) | indent(8) }}
@@ -378,10 +378,10 @@ void Convert(const {{ spec.annotations["proto-type"] | as_pb2_cpp_type }}& proto
     ros_msg->has_field = 0U;
         {%- endif -%}
         {%- for field_spec in spec.fields if field_spec.name != "has_field" -%}
-           {%- set source = "proto_msg." + field_spec.annotations.get("proto-name", field_spec.name).lower() + "()" -%}
+           {%- set source = "proto_msg." + to_pb2_cpp_name(field_spec.annotations.get("proto-cpp-name", field_spec.name)) + "()" -%}
            {%- set destination = "ros_msg->" + field_spec.name -%}
            {%- if field_spec.annotations["optional"] -%}{#- Check for field presence before use. #}
-    if (proto_msg.has_{{ field_spec.annotations.get("proto-name", field_spec.name).lower() }}()) {
+    if (proto_msg.has_{{ field_spec.annotations.get("proto-cpp-name", field_spec.name) | as_pb2_cpp_name }}()) {
         {{ proto_to_ros_field_code(source, destination, field_spec) | indent(8) }}
         ros_msg->has_field |= {{ spec.base_type | string | as_ros_cpp_type }}::{{ field_spec.name | upper }}_FIELD_SET;
     }

proto2ros/proto2ros/output/templates/conversions.hpp.jinja

@@ -2,8 +2,7 @@
 // See conversions.hpp.jinja template in the proto2ros.output.templates Python module.
 
 #pragma once
-
-{%- for header in config.cpp_headers %}
+{% for header in config.cpp_headers|sort %}
 #include <{{ header }}>
 {%- endfor %}
 
@@ -28,6 +27,5 @@ inline {{ spec.annotations["proto-type"] | as_pb2_cpp_type }} Convert(const {{ s
     Convert(ros_msg, &proto_msg);
     return proto_msg;
 }
-
-{% endfor -%}
+{% endfor %}
 }  // namespace {{ package_name }}::conversions

proto2ros/proto2ros/output/templates/conversions.py.jinja

@@ -115,7 +115,7 @@ if which == {{ type_spec.base_type | string | as_ros_python_type }}.{{ tag_spec.
 elif which == {{ type_spec.base_type | string | as_ros_python_type }}.{{ tag_spec.name }}:
             {%- endif %}
             {%- set source_member = source + "." + member_spec.name -%}
-            {%- set destination_member = destination.rpartition(".")[0] + "." + member_spec.annotations.get("proto-name", member_spec.name) %}
+            {%- set destination_member = destination.rpartition(".")[0] + "." + member_spec.annotations.get("proto-py-name", member_spec.name) %}
     {{ ros_to_proto_field_code(source_member, destination_member, member_spec) | indent(4) }}
         {%- endfor %}
 else:
@@ -221,15 +221,15 @@ else:
     raise ValueError("unknown protobuf message type in {{ spec.name }} member: %s" % {{ source }}.type_url)
     {%- elif type_spec and type_spec.annotations.get("tagged") -%}
         {#- Handle one-of field case i.e. determine and convert the Protobuf message member that is set. -#}
-which = {{ source.rpartition(".")[0] }}.WhichOneof("{{ spec.name }}")
+which = {{ source.rpartition(".")[0] }}.WhichOneof("{{ spec.annotations.get("proto-py-name", spec.name) }}")
         {%- set tag_field_spec = type_spec.annotations["tag"] %}
         {%- for tag_spec, member_spec in type_spec.annotations["tagged"] %}
             {%- if loop.first %}
 if which == "{{ member_spec.name }}":
             {%- else %}
 elif which == "{{ member_spec.name }}":
             {%- endif %}
-            {%- set source_member = source.rpartition(".")[0] + "." + member_spec.annotations.get("proto-name", member_spec.name) -%}
+            {%- set source_member = source.rpartition(".")[0] + "." + member_spec.annotations.get("proto-py-name", member_spec.name) -%}
             {%- set destination_member = destination + "." + member_spec.name %}
     {{ proto_to_ros_field_code(source_member, destination_member, member_spec) | indent(4) }}
     {{ destination }}.{{ tag_field_spec.name }} = {{ type_spec.base_type | string | as_ros_python_type }}.{{ tag_spec.name }}
@@ -253,7 +253,7 @@ def convert_{{ spec.base_type | string | as_python_identifier }}_message_to_{{ s
     proto_msg.Clear()
         {%- for field_spec in spec.fields if field_spec.name != "has_field" %}
             {%- set source = "ros_msg." + field_spec.name -%}
-            {%- set destination = "proto_msg." + field_spec.annotations.get("proto-name", field_spec.name) -%}
+            {%- set destination = "proto_msg." + field_spec.annotations.get("proto-py-name", field_spec.name) -%}
             {%- if field_spec.annotations["optional"] %}{#- Check for field presence before use. #}
     if ros_msg.has_field & {{ spec.base_type | string | as_ros_python_type }}.{{ field_spec.name | upper }}_FIELD_SET:
         {{ ros_to_proto_field_code(source, destination, field_spec) | indent(8) }}
@@ -280,10 +280,10 @@ def convert_{{ spec.annotations["proto-type"] | as_python_identifier  }}_proto_t
     ros_msg.has_field = 0
         {%- endif -%}
         {%- for field_spec in spec.fields if field_spec.name != "has_field" -%}
-           {%- set source = "proto_msg." + field_spec.annotations.get("proto-name", field_spec.name) -%}
+           {%- set source = "proto_msg." + field_spec.annotations.get("proto-py-name", field_spec.name) -%}
            {%- set destination = "ros_msg." + field_spec.name -%}
            {%- if field_spec.annotations["optional"] -%}{#- Check for field presence before use. #}
-    if proto_msg.HasField("{{ field_spec.annotations.get("proto-name", field_spec.name) }}"):
+    if proto_msg.HasField("{{ field_spec.annotations.get("proto-py-name", field_spec.name) }}"):
         {{ proto_to_ros_field_code(source, destination, field_spec) | indent(8) }}
         ros_msg.has_field |= {{ spec.base_type | string | as_ros_python_type }}.{{ field_spec.name | upper }}_FIELD_SET
             {%- else %}

proto2ros/proto2ros/utilities.py

@@ -160,6 +160,13 @@ def to_ros_base_type(type_: Union[str, BaseType]) -> str:
 RESERVED_KEYWORD_SET = PYTHON_RESERVED_KEYWORD_SET | CPP_RESERVED_KEYWORD_SET
 
 
+def to_protobuf_field_name(name: str) -> str:
+    """Transform a given name to be a valid Protobuf message field name."""
+    if name in RESERVED_KEYWORD_SET:
+        return name + "_"
+    return name
+
+
 def to_ros_field_name(name: str) -> str:
     """Transform a given name to be a valid ROS message field name."""
     name = name.lower()

proto2ros_tests/CMakeLists.txt

@@ -28,6 +28,8 @@ find_package(ament_cmake REQUIRED)
 find_package(builtin_interfaces REQUIRED)
 find_package(geometry_msgs REQUIRED)
 find_package(sensor_msgs REQUIRED)
+find_package(rclcpp REQUIRED)
+
 find_package(proto2ros REQUIRED)
 
 find_package(rosidl_default_generators REQUIRED)
@@ -70,8 +72,9 @@ target_include_directories(${PROJECT_NAME}_conversions PUBLIC
 )
 rosidl_get_typesupport_target(${PROJECT_NAME}_cpp_msgs ${PROJECT_NAME} "rosidl_typesupport_cpp")
 target_link_libraries(${PROJECT_NAME}_conversions
-  ${${PROJECT_NAME}_cpp_msgs} ${PROJECT_NAME}_proto
-  proto2ros::proto2ros_conversions protobuf::libprotobuf)
+  ${${PROJECT_NAME}_cpp_msgs} ${PROJECT_NAME}_proto protobuf::libprotobuf)
+ament_target_dependencies(${PROJECT_NAME}_conversions
+  builtin_interfaces geometry_msgs sensor_msgs proto2ros rclcpp)
 
 rosidl_generated_python_package_add(
   ${PROJECT_NAME}_additional_modules

proto2ros_tests/package.xml

@@ -19,6 +19,7 @@ Copyright (c) 2023 Boston Dynamics AI Institute LLC. All rights reserved.
   <depend>protobuf-dev</depend>
   <depend>proto2ros</depend>
 
+  <depend>rclcpp</depend>
   <depend>builtin_interfaces</depend>
   <depend>geometry_msgs</depend>
   <depend>sensor_msgs</depend>