feat: add static checker for cpp with cppcheck

[sakshi-1505]

Fixes #2297

Changes

Added staticcheck analysis

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

Merging #2346 (1ceb7e2) into main (0b9371d) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2346   +/-   ##
=======================================
  Coverage   87.06%   87.06%           
=======================================
  Files         199      199           
  Lines        6087     6087           
=======================================
  Hits         5299     5299           
  Misses        788      788           

[sakshi-1505]

@lalitb https://github.com/open-telemetry/opentelemetry-cpp/actions/runs/6446820822/job/17505022142?pr=2346 Looks like we will need to blacklist the third_party folder or the run will continue like this. Let me push a fix

[sakshi-1505]

@lalitb The CI worked & was quite fast all 300+ files in just 4 min! Do we want to exit 0 at the failures for cppcheck?

[sakshi-1505]

@lalitb I have made the action to return error whenever any rule is violated for staticcheck. This may cause check failure in CI, which the team might want to look at.

[lalitb]

@lalitb I have made the action to return error whenever any rule is violated for staticcheck. This may cause check failure in CI, which the team might want to look at.

Yeah the CI should fail in case of any static check violations in the code.

[lalitb]

If there are only few similar pattern of errors, e.g “ The one definition rule is violated,” error as seen in logs, can we suppress cppcheck rules for those error, and let this CI pass. This will enable merging this PR, and then track those errors separately.

[marcalff]

All the test code should be ignored.

I think we need a way to list, say in a separate file, all the directories to include/exclude from cppcheck.

[lalitb]

Should we have this CI similar to that for format :

opentelemetry-cpp/.github/workflows/ci.yml

Lines 678 to 686 in 11d5d9e

	format:
	name: Format
	runs-on: ubuntu-20.04
	steps:
	- uses: actions/checkout@v3
	- name: setup
	run: sudo ./ci/install_format_tools.sh
	- name: run tests
	run: ./ci/do_ci.sh format

• Create script to install a particular version of cppcheck passed as argument.
• And invoke the cppcheck through ./do_ci.sh cppcheck

This will also allow developers to invoke static analysis locally.

[ThomsonTan]

And should test_common also be ignored here?

[sakshi-1505]

All the test code should be ignored.

I think we need a way to list, say in a separate file, all the directories to include/exclude from cppcheck.

Acknowledged, let me see if there's a way

[sakshi-1505]

Sorry folks for delay here, I wasn't able to find any way by which I could actually use a file for giving the exclude paths. What I can do is create an exclude_path & wrap the cppcheck execution in a bash script which may use that excludePaths, will this work @lalitb @marcalff ?

[lalitb]

What I can do is create an exclude_path & wrap the cppcheck execution in a bash script which may use that excludePaths, will this work

If you plan to create the bash script as suggested here, the script can read all the paths to be ignored from a file, and create cppcheck arguments as -i<path1> -i<path2>.

[marcalff]

Hi @sakshi-1505

Please find below some guidance to resolve current issues.

The CI fails because:

Missing copyright in ./ci/exclude_cppcheck.txt
Missing license in ./ci/exclude_cppcheck.txt
Total number of failed checks: 2

To fix this, add copyright and license as comments.

And yes, it means this file must support comments: a simple grep -v '#' can be used to filter them out.

Please rename this configuration file to .cppcheck at the top level directory, for consistency with other tools (.clang-format, etc)

As @lalitb already indicated, the scripts should separate clearly:

• building cppcheck itself
• running cppcheck

For the CI workflow, it should use:

• env: CPPCHECK_VERSION = xxx
• sudo -E ./ci/install_cppcheck.sh

and then

• ./ci/do_ci.sh cppcheck

A developer locally will have cppcheck already installed, and will run only the second part.

About the cppcheck file format, I think it is simpler to provide full cppcheck options already instead of building them.

For example:

# Exclude third parties
-i ./third_party

# Exclude unit tests
-i ./api/test
-i ./sdk/test

etc

[ThomsonTan]

Use the checkout action to checkout the cppcheck repo?

[ThomsonTan]

Should cmake install already handled by ./ci/setup_cmake.sh 2 lines above?

[marcalff]

Ping @sakshi-1505

Are you still working on this ?
Do the code review comments make sense ?

Also, please update the PR branch with a recent main.

[sakshi-1505]

Yes I will get this updated by tonight.

…

On Thu, 7 Dec 2023, 10:59 pm Marc Alff, ***@***.***> wrote: Ping @sakshi-1505 <https://github.com/sakshi-1505> Are you still working on this ? Do the code review comments make sense ? Also, please update the PR branch with a recent main. — Reply to this email directly, view it on GitHub <#2346 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BDCZVU22REVU7B35742WAYDYIH4IPAVCNFSM6AAAAAA5XR4T3KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBVG44DKMJZGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[marcalff]

@sakshi-1505

This PR has been opened for 3 months now.

We (opentelemetry-cpp maintainers) have offered comments, suggestions and guidance to move forward.

The harsh reality as of time of writing (January 2024) is that not only nothing has changed (I understand this part, we all get busy), there is also (from my perspective) a bigger problem of lack of communication, with complete silence and no reply to comments, instead of engaging into a technical discussion to make progress.

I am now closing this PR.

Thanks for your interest in opentelemetry-cpp.