test(clang-tidy): test if workflow fails only when it reports errors

[xmfcx]

Description

⚠️This is not made to be merged.

Related links

Related PR:

• fix(clang-tidy): make clang-tidy workflow fail only when it reports errors autoware-github-actions#306
• ci(clang-tidy-differential): point to the new config #7729 (comment)

How was this PR tested?

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

[github-actions]

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

• You've checked our contribution guidelines.
• Your PR follows our pull request guidelines.
• All required CI checks pass before marking the PR ready for review.

[xmfcx]

We need to validate that

• It fails on error link
• It passes on warnings without errors: Link

[xmfcx]

Also we need a clang-tidy-all workflow too, not just differential. To check for all the files in the repository, before making it required.

[xmfcx]

Running it on this file:

• https://github.com/autowarefoundation/autoware.universe/blob/da0df3e93f9f79248b9777de60455721acfa10c9/map/map_loader/test/test_pointcloud_map_loader_module.cpp

On CI

This command:
> clang-tidy -p build/ -export-fixes /tmp/clang-tidy-result/fixes.yaml map/map_loader/test/test_pointcloud_map_loader_module.cpp

On the CI machine, generated these errors:

/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:31:29: error: using decl 'operator""ms' is unused [misc-unused-using-decls,-warnings-as-errors]
using std::chrono_literals::operator""ms;
                            ^
/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:31:29: note: remove the using
using std::chrono_literals::operator""ms;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:52:23: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
      cloud.points[i].x = static_cast<float>(i);
                      ^
/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:53:23: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
      cloud.points[i].y = static_cast<float>(i * 2);
                      ^
/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:54:23: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
      cloud.points[i].z = static_cast<float>(i * 3);
                      ^
/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:106:46: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
    EXPECT_FLOAT_EQ(received_cloud.points[i].x, static_cast<float>(i));
                                             ^
/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:107:46: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
    EXPECT_FLOAT_EQ(received_cloud.points[i].y, static_cast<float>(i * 2));
                                             ^
/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:108:46: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
    EXPECT_FLOAT_EQ(received_cloud.points[i].z, static_cast<float>(i * 3));
                                             ^

These are wrong results, at least the first warning is wrong:

31:29: error: using decl 'operator""ms' is unused

autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp

Line 89 in da0df3e

executor.spin_some(50ms);

But when I run it on my machine:

$ clang-tidy -p=build/ -export-fixes fixes.yaml src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp
22593 warnings and 1 error generated.
Error while processing /home/mfc/projects/autoware/src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp.
/home/mfc/projects/autoware/src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:33:7: warning: constructor does not initialize these fields: node, temp_pcd_path [cppcoreguidelines-pro-type-member-init]
class TestPointcloudMapLoaderModule : public ::testing::Test
      ^
/opt/ros/humble/include/rclcpp/rclcpp/rclcpp.hpp:152:10: error: 'csignal' file not found [clang-diagnostic-error]
#include <csignal>
         ^~~~~~~~~
Suppressed 22609 warnings (22592 in non-user code, 17 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
Found compiler error(s).

I can't figure out why these are different. clang-tidy versions are same, they both use the old .clang-tidy file.

[veqcc]

@xmfcx

clang-tidy differential will fail due to clang-diagnostic-error (redefinition of 'utils')
commit: fa2be66

Added:
It successfully failed!!
clang-tidy-differential: https://github.com/autowarefoundation/autoware.universe/actions/runs/9736792984/job/26868255713?pr=7749

Could clone this test PR and move on to review autowarefoundation/autoware-github-actions#306 ?

[veqcc]

@xmfcx
About the clang-tidy false positive you mention here

My thought is that false positives in static analysis tools are inevitable.
We could ignore those false positives which have lower severity.
In this case, the severity of misc-unused-using-decls is LOW and not required for PR merging, so let's ignore this time.

When we face a false positive with higher severity in the future, then let's find a way to avoid it. My idea is forcing developers to avoid those false positives even if the program is correct, because such kind of programs tend to be complicated and have low readability/maintainability.
What do you think?

[xmfcx]

My thought is that false positives in static analysis tools are inevitable.
We could ignore those false positives which have lower severity.
In this case, the severity of misc-unused-using-decls is LOW and not required for PR merging, so let's ignore this time.

I'm not talking about the error specifically.

I have pointed out that it behaves differently on CI than on my machine.

When you run clang-tidy -p build/ -export-fixes /tmp/clang-tidy-result/fixes.yaml map/map_loader/test/test_pointcloud_map_loader_module.cpp what result do you get?

[veqcc]

@xmfcx

I'm not talking about the error specifically.
I have pointed out that it behaves differently on CI than on my machine.
When you run clang-tidy -p build/ -export-fixes /tmp/clang-tidy-result/fixes.yaml map/map_loader/test/test_pointcloud_map_loader_module.cpp what result do you get?

Sorry, I was misunderstaing your point.

I have tested on my local machine:

$ clang-tidy --version
Ubuntu LLVM version 14.0.0
  
  Optimized build.
  Default target: x86_64-pc-linux-gnu
  Host CPU: alderlake

clang-tidy is the default version with apt install clang-tidy

$ clang-tidy -p build/ -export-fixes /tmp/clang-tidy-result/fixes.yaml src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp
32576 warnings and 1 error generated.
Error while processing /home/veqcc/work/awf_autoware/src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp.
/home/veqcc/work/awf_autoware/src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:65:28: warning: variable 'pcd_paths' is not initialized [cppcoreguidelines-init-variables]
  std::vector<std::string> pcd_paths = {temp_pcd_path};
                           ^
                                     = 0
/usr/include/eigen3/Eigen/Core:70:10: error: 'omp.h' file not found [clang-diagnostic-error]
#include <omp.h>
         ^~~~~~~

[xmfcx]

The version is same for me:

clang-tidy --version
Ubuntu LLVM version 14.0.0
  
  Optimized build.
  Default target: x86_64-pc-linux-gnu
  Host CPU: znver3

/usr/include/eigen3/Eigen/Core:70:10: error: 'omp.h' file not found [clang-diagnostic-error]

I've never got that one before 😕

[veqcc]

Sorry, I have forgotten I changed the compile_commands.json based on https://github.com/orgs/autowarefoundation/discussions/4876#discussioncomment-9921675

I will test with the original compile_commands.json again 🙇

Added:
I got the same results.

[veqcc]

After I have build map_loader with the following command,

colcon build --symlink-install  --cmake-args -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_EXPORT_COMPILE_COMMANDS=1  --packages-up-to  map_loader

I still got the same warnings

$ clang-tidy -p build/map_loader/compile_commands.json -export-fixes ./fixes.yaml src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp 
32576 warnings and 1 error generated.
Error while processing /home/veqcc/work/awf_autoware/src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp.
/home/veqcc/work/awf_autoware/src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:65:28: warning: variable 'pcd_paths' is not initialized [cppcoreguidelines-init-variables]
  std::vector<std::string> pcd_paths = {temp_pcd_path};
                           ^
                                     = 0
/usr/include/eigen3/Eigen/Core:70:10: error: 'omp.h' file not found [clang-diagnostic-error]
#include <omp.h>
         ^~~~~~~
Suppressed 32606 warnings (32575 in non-user code, 31 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
Found compiler error(s).