PS-9040 Integrate clang-tidy checks for Percona server #5196

VarunNagaraju · 2024-01-03T11:31:05Z

https://perconadev.atlassian.net/browse/PS-9040

Creates a yaml file for clang-tidy workflow which is triggered on a pull request.

The workflow triggers a run of generating comiplation commands and uses the same to perform a clang-tidy check on the modified files and generates a report of the warnings in the changed code through a github action bot in the form of review comments on the pull request.

inikep

LGTM

percona-ysorokin · 2024-01-03T11:49:55Z

.github/workflows/main.yml

+          fetch-depth: 0
+
+      - name: Fetch base branch
+        run: |


Any chance we can re-use actions/checkout@v4 action here as well?

Fetching submodules can be added to the checkout step. But, I'm not sure if we can fetch just one more branch(base branch) without fetching all the other unnecessary branches and tags using fetch-depth option.
Also, after checking out to the PR branch, fetching the base branch takes less than a second. So, I don't think it adds any overhead.

percona-ysorokin · 2024-01-03T11:52:47Z

.github/workflows/main.yml

+      - name: Install clang-tidy
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y clang clang-tidy libldap2-dev curl libcurl4-openssl-dev bison libudev-dev libkrb5-dev


I would go with the latest clang-17 here but you will need to add a custom LLVM apt repository first
(https://apt.llvm.org/)

In this case please use a variable instead of 17 as we do at:
https://github.com/percona/percona-server/blob/8.0/azure-pipelines.yml#L396-L400
You will also need a variable with a Linux distro name (above we use e.g. UBUNTU_CODE_NAME: focal).

percona-ysorokin · 2024-01-03T12:38:17Z

.github/workflows/main.yml

+
+      - name: Prepare compile_commands.json
+        run: |
+          cmake -B build -DCMAKE_INSTALL_PREFIX=../install -DWITH_DEBUG=1 -DDOWNLOAD_BOOST=1 -DWITH_BOOST=my_boost \


Put build directory out of source tree, say to ../build.

Do not use -DWITH_DEBUG directly, use -DCMAKE_BUILD_TYPE=Debug|RelWithDebInfo.
BTW, it also makes sense to check both Debug and RelWithDebInfo configurations with clang-tidy.

Put boost directory out of source, say ../boost. It also makes sense to cache the boost tarball archive using actions/cache@v3 action.

For Boolean CMake Options please use ON|OFF instead of ''1|0'

For this particular scenario, I think you should use -DWITH_SYSTEM_LIBS=ON for every library possible. Having something bundled just extends build command JSON file with additional items that won't be used.

Something is wrong with WITH_CURL option, it points to a binary - just remove it.

Out of source build dir - Done.

Use a cache for boost - Done.

Update boolean options to ON/OFF - Done.

Use DWITH_SYSTEM_LIBS=ON - Done.

Remove curl option - Done.

Regarding checking for both Debug and RelWithDebInfo builds, wouldn't checking only for Debug build cover all the code since -DCMAKE_BUILD_TYPE=Debug is equivalent to a superset of both RelWithDebInfo and Release options from a compile time static analysis is concerned?

VarunNagaraju · 2024-01-05T10:58:15Z

After integration, the clang-tidy warnings will be shown as review comments like VarunNagaraju#4. In any case, one can also choose to ignore addressing the comments and the push/merge option will NOT be disabled.

The comments will be posted only for the snippets of the code which are modified by the PR. But, clang-tidy will be run for the whole file(on all the files modified by the PR) and the clang-tidy log for that can be found in the Checks section or by clicking on the Show all checks > Static analysis > Details > Analyze in the workflow.

percona-ysorokin · 2024-01-05T13:01:06Z

.github/workflows/main.yml

+          curl -sSL "http://apt.llvm.org/llvm-snapshot.gpg.key" | sudo -E apt-key add -
+          echo "deb http://apt.llvm.org/${UBUNTU_CODE_NAME}/ llvm-toolchain-${UBUNTU_CODE_NAME}-${COMPILER_VERSION} main" | sudo tee -a /etc/apt/sources.list > /dev/null
+          sudo apt-get update
+          sudo apt-get install -y clang-17 clang-tidy libldap2-dev curl libcurl4-openssl-dev bison libudev-dev libkrb5-dev libreadline-dev zlib1g-dev liblz4-dev \


Suggested change

sudo apt-get install -y clang-17 clang-tidy libldap2-dev curl libcurl4-openssl-dev bison libudev-dev libkrb5-dev libreadline-dev zlib1g-dev liblz4-dev \

sudo apt-get install -y clang-${COMPILER_VERSION} clang-tidy libldap2-dev curl libcurl4-openssl-dev bison libudev-dev libkrb5-dev libreadline-dev zlib1g-dev liblz4-dev \

percona-ysorokin · 2024-01-05T13:01:59Z

.github/workflows/main.yml

+      - name: Prepare compile_commands.json
+        run: |
+          cmake -B ../debug-build -DCMAKE_INSTALL_PREFIX=../install -DCMAKE_BUILD_TYPE=Debug -DDOWNLOAD_BOOST=ON -DWITH_BOOST=~/my_boost \
+          -DWITH_SSL=system -DWITH_AUTHENTICATION_LDAP=0 -WITH_KEYRING_VAULT=ON -DWITH_ROCKSDB=0 -DCMAKE_C_COMPILER=clang-17 -DCMAKE_CXX_COMPILER=clang++-17 \


Suggested change

-DWITH_SSL=system -DWITH_AUTHENTICATION_LDAP=0 -WITH_KEYRING_VAULT=ON -DWITH_ROCKSDB=0 -DCMAKE_C_COMPILER=clang-17 -DCMAKE_CXX_COMPILER=clang++-17 \

-DWITH_SSL=system -DWITH_AUTHENTICATION_LDAP=0 -WITH_KEYRING_VAULT=ON -DWITH_ROCKSDB=0 -DCMAKE_C_COMPILER=clang-${COMPILER_VERSION} -DCMAKE_CXX_COMPILER=clang++-${COMPILER_VERSION} \

Why -DWITH_AUTHENTICATION_LDAP=0 and -DWITH_ROCKSDB=0?
This is out code which, I believe, should also be covered.

Re, having clang-tidy checks for both Debug and RelWithDebInfo configurations. This comes just from personal experience - I remember warnings shown only in RelWithDebInfo configuration (mostly related to assert() or assert-like macros like ut_ad()). But this can definitely be a second iteration of this improvement.

Done.
Agree.

percona-ysorokin · 2024-01-05T13:09:48Z

.github/workflows/main.yml

+        continue-on-error: true
+        run: |
+          git diff --name-only --diff-filter=ACRM "$(git merge-base HEAD "upstream/${{ github.event.pull_request.base.ref }}")" | \
+          grep -Ee "\.([ch](pp)|(cc|hh)|[i](c|h)|(cxx)|[chi])$" | xargs clang-tidy -p ../debug-build --checks=-readability-* -export-fixes clang-tidy-result/fixes.yml


Not sure if it is suitable here but there is a standard clang-tidy-diff-17.py script coming from the clang packages. Please also check if it can be used here.

Yes. I had used clang-tidy directly since it also reported warnings in unchanged sections of the code as well. But, this script will do as well. Will update it.

percona-ysorokin · 2024-01-05T13:13:44Z

.github/workflows/main.yml

+
+      - name: Fetch base branch
+        run: |
+          git remote add upstream "https://github.com/${{ github.event.pull_request.base.repo.full_name }}"


Is it OK in git to have 2 remotes pointing to the same repo?
In case we are creating a non-crossfork PR.

Right. It is unnecessary to point 2 remotes to the same repo since origin will already be pointing to the appropriate repo. Will remove it.

Sorry, @VarunNagaraju I might have mislead you. I wanted to say that in case of a cross-fork PR we will need 2 remotes:

origin https://github.com/percona/percona-server.git fork origin https://github.com/<percona_user>/percona-server.git

But in case of a PR within the same repo (say, when we merge release-8.0.xx-yy branch into 8.0 trunk) we will basically have

origin https://github.com/percona/percona-server.git fork https://github.com/percona/percona-server.git

And my question was more about if git itself allows to have 2 remotes with different names pointing to the same repo.

inikep · 2024-01-10T08:57:15Z

.github/workflows/main.yml

+          curl -sSL "http://apt.llvm.org/llvm-snapshot.gpg.key" | sudo -E apt-key add -
+          echo "deb http://apt.llvm.org/${UBUNTU_CODE_NAME}/ llvm-toolchain-${UBUNTU_CODE_NAME}-${COMPILER_VERSION} main" | sudo tee -a /etc/apt/sources.list > /dev/null
+          sudo apt-get update
+          sudo apt-get install -y clang-${COMPILER_VERSION} clang-tidy-${COMPILER_VERSION} libldap2-dev curl libcurl4-openssl-dev bison libudev-dev libkrb5-dev libreadline-dev zlib1g-dev liblz4-dev \


We may add --allow-unauthenticated here. Without this we may sporadically have issues when packages are updated or can't be authenticated (at least we had these issues with Travis CI or Azure Pipelines).
Moreover using --no-install-recommends should make install slightly faster but it will require to manually add dependencies; probably llvm-$(COMPILER_VERSION)-dev will be needed.

VarunNagaraju · 2024-02-27T11:25:45Z

All the review comments are addressed and cross fork testing #5241 works as well. But, there is an issue with permissions to post comments which has popped up recently.
A ticket with the clang-tidy-pr-comments github actions has been reported platisd/clang-tidy-pr-comments#75.

VarunNagaraju · 2024-03-03T14:53:47Z

Cross fork testing results:

8.0 - Remove clang-tidy checks in source code venkatesh-prasad-v/percona-server#2
trunk - Remove clang-tidy checks in source code venkatesh-prasad-v/percona-server#3

percona-ysorokin

LGTM with minor mostly stylistic comments

.github/workflows/clang-tidy.yml

percona-ysorokin · 2024-03-12T19:28:39Z

.github/workflows/post-pr-comments.yml

+          const artifacts = await github.rest.actions.listWorkflowRunArtifacts({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              run_id: ${{ github.event.workflow_run.id }},


Is it OK to end the last element with a comma?

Since, the code snippet is a javascript method invocation, the trailing comma is not a problem.
In JavaScript, it's a common practice to use trailing commas in multi-line object and array literals, mainly because it allows for cleaner git diffs when adding or removing items.

percona-ysorokin · 2024-03-12T19:28:57Z

.github/workflows/post-pr-comments.yml

+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              artifact_id: matchArtifact.id,
+              archive_format: "zip",


percona-ysorokin · 2024-03-12T19:29:51Z

.github/workflows/post-pr-comments.yml

+          const artifacts = await github.rest.actions.listWorkflowRunArtifacts({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              run_id: ${{ github.event.workflow_run.id }},


percona-ysorokin · 2024-03-12T19:30:01Z

.github/workflows/post-pr-comments.yml

+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              artifact_id: matchArtifact.id,
+              archive_format: "zip",


https://perconadev.atlassian.net/browse/PS-9040 Creates a workflow to be triggered on a pull request to perform clang-tidy checks on the changed code and post the violations as comments on the pull request to be addressed. The workflow triggers a run of generating comiplation commands and uses the same to perform a clang-tidy check on the modified files and generates a report of the warnings in the changed code through a github action bot in the form of review comments on the pull request.

percona-ysorokin

LGTM

VarunNagaraju requested review from percona-ysorokin and inikep January 3, 2024 11:31

inikep approved these changes Jan 3, 2024

View reviewed changes

percona-ysorokin requested changes Jan 3, 2024

View reviewed changes

VarunNagaraju force-pushed the PS-9040-8.0 branch from d3d8eb1 to 9e69432 Compare January 5, 2024 10:55

VarunNagaraju mentioned this pull request Jan 5, 2024

PS-9040 Integrate clang-tidy checks for Percona server #5194

Closed

percona-ysorokin reviewed Jan 5, 2024

View reviewed changes

VarunNagaraju force-pushed the PS-9040-8.0 branch from 9e69432 to 8b11cea Compare January 8, 2024 11:00

inikep reviewed Jan 10, 2024

View reviewed changes

VarunNagaraju force-pushed the PS-9040-8.0 branch from 8b11cea to 4f9b494 Compare January 10, 2024 11:45

VarunNagaraju force-pushed the PS-9040-8.0 branch from 4f9b494 to 0662867 Compare February 29, 2024 10:26

VarunNagaraju requested review from percona-ysorokin and inikep February 29, 2024 10:27

percona-ysorokin approved these changes Mar 12, 2024

View reviewed changes

VarunNagaraju force-pushed the PS-9040-8.0 branch from 0662867 to 7c13381 Compare March 14, 2024 07:29

percona-ysorokin approved these changes Mar 14, 2024

View reviewed changes

VarunNagaraju merged commit b5dfcb2 into percona:8.0 Mar 14, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PS-9040 Integrate clang-tidy checks for Percona server #5196

PS-9040 Integrate clang-tidy checks for Percona server #5196

VarunNagaraju commented Jan 3, 2024

inikep left a comment

percona-ysorokin Jan 3, 2024 •

edited

Loading

VarunNagaraju Jan 5, 2024

percona-ysorokin Jan 3, 2024

inikep Jan 4, 2024 •

edited

Loading

VarunNagaraju Jan 5, 2024

percona-ysorokin Jan 3, 2024

VarunNagaraju Jan 5, 2024

VarunNagaraju commented Jan 5, 2024 •

edited

Loading

percona-ysorokin Jan 5, 2024

VarunNagaraju Jan 8, 2024

percona-ysorokin Jan 5, 2024

percona-ysorokin Jan 5, 2024

percona-ysorokin Jan 5, 2024

VarunNagaraju Jan 8, 2024

percona-ysorokin Jan 5, 2024

VarunNagaraju Jan 8, 2024

percona-ysorokin Jan 5, 2024

VarunNagaraju Jan 8, 2024

percona-ysorokin Jan 11, 2024

inikep Jan 10, 2024

VarunNagaraju Jan 10, 2024

VarunNagaraju commented Feb 27, 2024

VarunNagaraju commented Mar 3, 2024

percona-ysorokin left a comment

percona-ysorokin Mar 12, 2024

VarunNagaraju Mar 14, 2024

percona-ysorokin Mar 12, 2024

percona-ysorokin Mar 12, 2024

percona-ysorokin Mar 12, 2024

percona-ysorokin left a comment

	sudo apt-get install -y clang-17 clang-tidy libldap2-dev curl libcurl4-openssl-dev bison libudev-dev libkrb5-dev libreadline-dev zlib1g-dev liblz4-dev \
	sudo apt-get install -y clang-${COMPILER_VERSION} clang-tidy libldap2-dev curl libcurl4-openssl-dev bison libudev-dev libkrb5-dev libreadline-dev zlib1g-dev liblz4-dev \

	-DWITH_SSL=system -DWITH_AUTHENTICATION_LDAP=0 -WITH_KEYRING_VAULT=ON -DWITH_ROCKSDB=0 -DCMAKE_C_COMPILER=clang-17 -DCMAKE_CXX_COMPILER=clang++-17 \
	-DWITH_SSL=system -DWITH_AUTHENTICATION_LDAP=0 -WITH_KEYRING_VAULT=ON -DWITH_ROCKSDB=0 -DCMAKE_C_COMPILER=clang-${COMPILER_VERSION} -DCMAKE_CXX_COMPILER=clang++-${COMPILER_VERSION} \

PS-9040 Integrate clang-tidy checks for Percona server #5196

PS-9040 Integrate clang-tidy checks for Percona server #5196

Conversation

VarunNagaraju commented Jan 3, 2024

inikep left a comment

Choose a reason for hiding this comment

percona-ysorokin Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inikep Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VarunNagaraju commented Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VarunNagaraju commented Feb 27, 2024

VarunNagaraju commented Mar 3, 2024

percona-ysorokin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

percona-ysorokin left a comment

Choose a reason for hiding this comment

percona-ysorokin Jan 3, 2024 •

edited

Loading

inikep Jan 4, 2024 •

edited

Loading

VarunNagaraju commented Jan 5, 2024 •

edited

Loading