Proposal to enable cppcheck as a required CI for Autoware Universe

[kminoda]

Introduction

To improve the code quality of Autoware Universe, we have introduced cppcheck, a static analysis tool, into our CI workflow. I would like to propose making cppcheck-differential a required check.

cppcheck Workflows

We have set up two workflows:

• cppcheck-differential: This workflow checks only the minimal set of items and is intended to be a MUST-pass check. If there are no objections, we plan to make this workflow required.

• cppcheck-all: This workflow checks all items in cppcheck and is intended to gradually reduce the number of issues to zero. This workflow is for informational purposes.

How to Access the Results

You can download the results of both workflows from the Summary section at the bottom of the GitHub Actions execution results.
See https://github.com/autowarefoundation/autoware.universe/actions/runs/9383750313

Proposal

We propose making the cppcheck-differential workflow a required check in our CI pipeline to ensure the quality of our codebase. Below are the details:

• Objective: To raise the quality of Autoware's code by ensuring minimal set of cppcheck items are always passed.
• Scope: Initially, make the cppcheck-differential workflow required. We will continue to monitor and aim to address all issues found by the cppcheck-all workflow over time.
• Suppression List: A list of suppressed items can be found here.

Request for Feedback

If you have any comments or suggestions regarding this proposal, especially if you wish to increase or decrease the suppression list, please provide your feedback by 10th of June (next Monday).

[xmfcx]

https://github.com/autowarefoundation/autoware.universe/actions/workflows/cppcheck-differential.yaml?query=is%3Asuccess

This is a very slow task (takes about 1.5h to 2h).

Click here to expand (You can ignore this)

Github provides free action runners. But they are also limited in numbers.

For free plan, per organization, they provide 20 concurrent jobs.

And autoware.universe is just one of the many repositories within the organization.

• https://github.com/organizations/autowarefoundation/settings/actions/hosted-runners (not sure if non-admins can see this)

But when we have so many heavy tasks like:

• build-and-test (runs per merged pr)
• build-and-test-differential (runs for each pr update)
• and now cppcheck-differential (runs for each pr update)

These add up and clog up the resources, preventing even small jobs from running.

I understand it would be great if we could run this job per PR but if it is slow like this, it won't be possible.

We should either:

• look into speeding it up by reducing the number of files it checks or another method
• or run the daily version and someone continuously tries to keep it green
• or speed up the other heavy tasks

Never mind, I was looking at total workflow time, the cppcheck-differential takes about 2 mins only. We can make it required.

[mitsudome-r]

I modified the rulesets to make it required.
If there are any issues, please let me know.

[kminoda]

Memo:
I asked @mitsudome-r to make it un-required.

There was an issue in comparing the current branch and the main branch, especially when the PR branch is not up-to-date with the main branch. We should modify here to address this issue: https://github.com/autowarefoundation/autoware.universe/blob/afc129e98554c1499a80862f6f27235154059c23/.github/workflows/cppcheck-differential.yaml#L32-L37

[veqcc]

@kminoda
I suggest suppressing warnings on test files by adding *:*/test/* to .cppcheck_suppresions.txt.
I will make a PR later.

[veqcc]

@mitsudome-r @xmfcx

We have fixed the issue discussed above in this PR !!

There was an issue in comparing the current branch and the main branch, especially when the PR branch is not up-to-date with the main branch. We should modify here to address this issue: https://github.com/autowarefoundation/autoware.universe/blob/afc129e98554c1499a80862f6f27235154059c23/.github/workflows/cppcheck-differential.yaml#L32-L37

Could you make the cppcheck-differential required again? Thank you.

[mitsudome-r]

@veqcc I have checked some PRs and checked that cppcheck-differential is working fine. (example)
It takes about a minute to install cppcheck, but I think it is acceptable.

I have made it required for Autoware.Universe

[veqcc]

Thenk you for your quick response 👍
I will check the cppcheck-differential CI works well for 1~2 weeks, and if there is no problem, I will close this Discussion.

[veqcc]

@mitsudome-r @xmfcx @kminoda
Hi,

Thanks to a lot of help from Autoware developers, now Autoware has become cppcheck-warning free!!
See cppcheck-daily CI for more detail: https://github.com/autowarefoundation/autoware.universe/actions/runs/10592983966

Though there are some remaining warnings, I think they are inevitable in ROS 2 applications or are unnecessary to be removed.

Future plans are:

• keep cppcheck-differential workflow required for each Pull Request, as it is now
• stop (or reduce the frequency of) cppcheck-daily workflow, since any cppcheck warning can be added to Autoware thanks to cppcheck-differential
  • I will make another Pull Request to change cppcheck-daily workflow

[veqcc]

The frequency of cppcheck-daily will be reduced to weekly in autowarefoundation/autoware.universe#8713

[kminoda]

Huge thanks to you and community for executing this task 🙏 I will close this discussion with your comment.

(Feel free to re-open this discussion or maybe another issue if we should continue tracking the cppcheck-related tasks)

[veqcc]

LGTM 👍