ament_cpplint gives test errors on generated message files

xmfcx commented 1 year ago

Checklist

[X] I've read the contribution guidelines.
[X] I've searched other issues and no duplicate issues were found.
[X] I'm convinced that this is not my fault but a bug.

Description

The ros2 message unit tests related to ament_cpplint fail with Category 'whitespace/line_length' errors.

Expected behavior

ament_cpplint shouldn't give line length errors for generated message files.

Actual behavior

I was trying to generate a new messages package named autoware_control_center_msgs.

https://github.com/autowarefoundation/autoware.core/actions/runs/3913282956/jobs/6688988149#step:7:926 fails.

To replicate it on my machine I ran: colcon test --event-handlers console_cohesion+ --packages-select autoware_control_center_msgs --ctest-args -L ".*" --executor sequential --return-code-on-test-failure And got the same result.

The following tests FAILED:
          2 - cpplint_rosidl_generated_c (Failed)
          5 - cpplint_rosidl_typesupport_fastrtps_c (Failed)
          8 - cpplint_rosidl_generated_cpp (Failed)
         11 - cpplint_rosidl_typesupport_fastrtps_cpp (Failed)
         14 - cpplint_rosidl_typesupport_introspection_c (Failed)
         17 - cpplint_rosidl_typesupport_c (Failed)
         20 - cpplint_rosidl_typesupport_introspection_cpp (Failed)
         23 - cpplint_rosidl_typesupport_cpp (Failed)
         26 - cpplint_rosidl_generated_py (Failed)

To test other packages, I tried it for already existing messages like autoware_planning_msgs or autoware_sensing_msgs and got the same test errors.

Then created a test PR on autoware_msgs to debug, they were successful.

In following PR, I tested with exactly same package, it passed the test.

https://github.com/autowarefoundation/autoware_msgs/actions/runs/3931108667/jobs/6721941355#step:7:870 is successful.

Steps to reproduce

Update your system?
colcon build --symlink-install --cmake-args -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_EXPORT_COMPILE_COMMANDS=1 --packages-select autoware_sensing_msgs
colcon test --event-handlers console_cohesion+ --packages-select autoware_sensing_msgs --ctest-args -L ".*" --executor sequential --return-code-on-test-failure

It should fail the test.

Versions

OS: Ubuntu 22.04
ROS2: humble
Autoware: Core/Universe

Possible causes

Maybe some update was made in humble message generation or ament_cpplint.

Additional context

No response

beyzanurkaya commented 1 year ago

I have reproduced the bug following step 2 and step 3.

72% tests passed, 9 tests failed out of 32

Label Time Summary:
copyright     =   0.17 sec*proc (1 test)
cppcheck      =   1.55 sec*proc (9 tests)
cpplint       =   2.47 sec*proc (9 tests)
flake8        =   0.47 sec*proc (1 test)
lint_cmake    =   0.16 sec*proc (1 test)
linter        =   7.55 sec*proc (32 tests)
pep257        =   0.21 sec*proc (1 test)
uncrustify    =   1.61 sec*proc (9 tests)
xmllint       =   0.90 sec*proc (1 test)

Total Test time (real) =   7.56 sec

The following tests FAILED:
      2 - cpplint_rosidl_generated_c (Failed)
      5 - cpplint_rosidl_typesupport_fastrtps_c (Failed)
      8 - cpplint_rosidl_generated_cpp (Failed)
     11 - cpplint_rosidl_typesupport_fastrtps_cpp (Failed)
     14 - cpplint_rosidl_typesupport_introspection_c (Failed)
     17 - cpplint_rosidl_typesupport_c (Failed)
     20 - cpplint_rosidl_typesupport_introspection_cpp (Failed)
     23 - cpplint_rosidl_typesupport_cpp (Failed)
     26 - cpplint_rosidl_generated_py (Failed)
Errors while running CTest
Output from these tests are in: /home/beyza/projects/autoware/build/autoware_sensing_msgs/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.
---
--- stderr: autoware_sensing_msgs
Errors while running CTest
Output from these tests are in: /home/beyza/projects/autoware/build/autoware_sensing_msgs/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.
---
Finished <<< autoware_sensing_msgs [7.72s]  [ with test failures ]

Summary: 1 package finished [8.17s]
  1 package had stderr output: autoware_sensing_msgs
  1 package had test failures: autoware_sensing_msgs

kenji-miyake commented 1 year ago

Why do you have to use ament_cpplint? If there is no specific reason, how about replacing it with pre-commit?

xmfcx commented 1 year ago

@kenji-miyake I don't have a specific reason, we generally use ament_lint_auto like in autoware_planning_msgs/package.xml and it's default in ros2.

  <test_depend>ament_lint_auto</test_depend>
  <test_depend>ament_lint_common</test_depend>

and

for example autoware_planning_msgs cmakelists:

find_package(ament_cmake_auto REQUIRED)
ament_auto_find_build_dependencies()

rosidl_generate_interfaces(${PROJECT_NAME}
  "msg/LaneletPrimitive.msg"
  "msg/LaneletRoute.msg"
  "msg/LaneletSegment.msg"
  "msg/PoseStampedWithUuid.msg"
  "srv/SetRoute.srv"
  DEPENDENCIES
    autoware_common_msgs
    geometry_msgs
    std_msgs
    unique_identifier_msgs
  ADD_LINTER_TESTS
)

if(BUILD_TESTING)
  find_package(ament_lint_auto REQUIRED)
  ament_lint_auto_find_test_dependencies()
endif()

ament_auto_package()

It's as easy as typing ADD_LINTER_TESTS in default rosidl_generate_interfaces cmake function.

I'm ok with replacing it too.

kenji-miyake commented 1 year ago

@xmfcx Then, let's use autoware_lint_common instead! https://github.com/autowarefoundation/autoware_common/tree/main/autoware_lint_common

xmfcx commented 1 year ago

@xmfcx Then, let's use autoware_lint_common instead! https://github.com/autowarefoundation/autoware_common/tree/main/autoware_lint_common

I will try and report back today. Also I will create a PR to update existing autoware_messages too.

xmfcx commented 1 year ago

@kenji-miyake I've tried and it gives the same errors: https://github.com/autowarefoundation/autoware.core/actions/runs/4084787104/jobs/7041989474

My commit: https://github.com/autowarefoundation/autoware.core/pull/57/commits/3c473bb31ac67438b2d860c4b25be1bbe9414bcb

I've followed the readme of https://github.com/autowarefoundation/autoware_common/tree/main/autoware_lint_common and results are the same.

xmfcx commented 1 year ago

@kenji-miyake Now I looked into https://github.com/tier4/tier4_ad_api_adaptor/blob/tier4/universe/autoware_external_api_msgs/CMakeLists.txt as an example and removed the ADD_LINTER_TESTS from rosidl_generate_interfaces function with this commit: https://github.com/autowarefoundation/autoware.core/pull/57/commits/04fa65a07505e831409aff16692e9b94d6ed8ee9 . Now it passes the tests.

I think we should add this to the existing messages and also as a recommendation in the autoware_lint_common readme file.

kenji-miyake commented 1 year ago

FYI @mitsudome-r @yukkysaito @isamu-takagi.

autowarefoundation / autoware.core