[PoC] Integrate SonarCloud

aureleoules commented at 3:09 pm on January 10, 2023: member

This is a proof of concept that integrates SonarCloud in the CI. SonarCloud is a static code analyzer that could help us catch bugs and improve code quality, so I think this would be a great addition to the CI. It can provide PR diff analysis. The code in this PR doesn’t currently work because it requires a Sonarcloud account along with a token.

I’ve tested it on master and some pull requests if you want to take a look: https://sonarcloud.io/summary/overall?id=aureleoules_bitcoin.

Let me know your thoughts!

[PoC] Integrate SonarCloud f3ee4282de

DrahtBot commented at 3:09 pm on January 10, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

maflcko commented at 3:18 pm on January 10, 2023: member

How is this different from cppcheck, clang-analyser, clang-tidy, etc … ? We tried them, but they didn’t really produce useful output on the code.

fanquake commented at 3:19 pm on January 10, 2023: member

untested proof of concept

Looks very noisy, with 1000’s of false positives etc. Maybe wait until it finds something actionable?

To properly make it work,

What do you mean by properly? Couldn’t you just run this yourself and open an issue if anything interesting is reported? Same as we do for OSS-Fuzz / other tools.

aureleoules commented at 5:20 pm on January 10, 2023: member

How is this different from cppcheck, clang-analyser, clang-tidy, etc … ? We tried them, but they didn’t really produce useful output on the code.

I haven’t played with them long enough to really answer but one useful feature of SonarCloud is that you can get a PR diff analysis.

Looks very noisy, with 1000’s of false positives etc. Maybe wait until it finds something actionable?

Yeah, I don’t recommend looking at the master branch, there is a lot of data. But I think it still outputs good results for PRs as it only shows the diff between the master analysis and the PR analysis.

I wouldn’t want to make the CI fail if SonarCloud reports bad code quality as there is a high chance that there will be false positives, but only provide a code quality report so that it can quickly be looked at to see how the code can be improved. Any rule that gives too many false positives can be disabled also.

What do you mean by properly? Couldn’t you just run this yourself and open an issue if anything interesting is reported? Same as we do for OSS-Fuzz / other tools.

By properly I mean integrating it in the Bitcoin Core CI because to upload the report to sonarcloud it needs a (free) token generated by a repo admin. Yes, I could run it myself but on a limited amount of PRs. I also think it’s more practical to generate the report from the CI and provide it after every push event and display it to everyone.

ghost commented at 1:00 pm on January 11, 2023: none

Couldn’t you just run this yourself and open an issue if anything interesting is reported? Same as we do for OSS-Fuzz / other tools.

Agree

By properly I mean integrating it in the Bitcoin Core CI because to upload the report to sonarcloud it needs a (free) token generated by a repo admin.

Have you tried https://deepsource.io/ ? They recently added support for C++ and python was already available. Fork repos are also supported and works for pull requests as well. No CI required and free for personal accounts with unlimited repos.

maflcko commented at 1:41 pm on January 11, 2023: member

No objection to trying this out temporarily, as long as it is not too spammy and doesn’t fail CI

maflcko commented at 1:49 pm on January 11, 2023: member

Sonarcloud account along with a token

It would be good to know what this requires. Write access to the repo would be a blocker.

I wonder if it is better to just take their rules and enable (or implement) them in clang-tidy or a linter?

aureleoules commented at 2:13 pm on January 11, 2023: member

These are the permissions that the SonarCloud GitHub application requires :

An admin of the organization would need to:

Setup this GitHub app on the repo
Login on SonarCloud with GitHub
Create the bitcoin/bitcoin project on SonarCloud and retrieve the associated SonarCloud token
Encrypt the token in Cirrus’s dashboard and store it the .cirrus.yml or store it raw in the GitHub repo secrets?

The token is only needed to send the analysis report back to SonarCloud.

SonarCloud can post a comment on the PR when the report is generated but that may spam, so a link to the report in DrahtBot’s summary comment could do it.

I wonder if it is better to just take their rules and enable (or implement) them in clang-tidy or a linter?

I think it would take too much time, so not worth it. There are a lot of rules and SonarCloud occasionally adds and maintains them also.

maflcko commented at 2:17 pm on January 11, 2023: member

Login on SonarCloud with GitHub

This requires “Act on your behalf”, which seems sketchy

aureleoules commented at 2:23 pm on January 11, 2023: member

Good catch, I looked into it and found this:

https://community.sonarsource.com/t/what-are-the-security-implications-of-letting-sonarcloud-act-on-my-behalf-after-signing-up/75233/10 https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/authorizing-github-apps

This ability to act on your behalf is limited to the GitHub resources where both you and the GitHub App have access. In some cases, however, the application may never make any changes on your behalf.

So would it still be an issue?

aureleoules commented at 2:25 pm on January 11, 2023: member

As an alternative, SonarCloud provides a self-hosted solution “SonarQube”. It is much more cumbersome to setup though.

fanquake commented at 2:34 pm on January 11, 2023: member

I haven’t played with them long enough to really answer but one useful feature of SonarCloud is that you can get a PR diff analysis. But I think it still outputs good results for PRs

Can you provide some examples of these good results, where it’s found bugs or things that are actionable?

maflcko commented at 2:40 pm on January 11, 2023: member

Maybe open a few pulls from this repo against yours, so that it can be checked?

aureleoules commented at 3:58 pm on January 11, 2023: member

Can you provide some examples of these good results, where it’s found bugs or things that are actionable?

Ran the tool on a few pulls and picked a few code smells for each:

fanquake commented at 4:08 pm on January 11, 2023: member

Ran the tool on a few pulls and picked a few code smells for each:

Remove this redundant cast. Define a constant instead of duplicating this literal Pass large object “hw” by reference to const. Use “empty()” to check whether the container is empty or not. Make the type of this parameter a reference-to-const. Replace the redundant type with “auto”. Remove the commented out code. Replace this use of “push_back” with “emplace_back”.

Looks like theres a lot of cross-over with clang-tidy, and all of it’s checks. Some are basically identical, i.e: https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-emplace.html https://clang.llvm.org/extra/clang-tidy/checks/readability/container-size-empty.html https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables.html https://clang.llvm.org/extra/clang-tidy/checks/misc/const-correctness.html https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-auto.html

Given we are currently in the process of potentially utilising more clang-tidy checks in our CI, it’s not clear how this is any better.

aureleoules closed this on Jan 16, 2023

bitcoin locked this on Jan 16, 2024

[PoC] Integrate SonarCloud #26865

Reviews