adding clang-tidy as a linter tool #2269

Suyashd999 · 2024-07-16T16:21:11Z

The Ceph project has a huge codebase, and it faces the risk of containing suboptimal code that could jeopardize storage reliability or induce performance bottlenecks or cause resource inefficiencies. Identifying and rectifying such code issues is important to maintain the integrity and efficiency of the Ceph storage system. clang-tidy, a powerful static analysis tool, offers a systematic approach to uncover critical issues within the codebase and generate comprehensive reports highlighting potential vulnerabilities.

Integrating clang-tidy into the Jenkins CI pipeline of Ceph would be very beneficial for the development, as any new possible vulnerabilities would be identified early on. The goal is to enhance code quality, maintain performance, and ensure the reliability of the storage system by continuously monitoring and addressing potential issues through automated static analysis.

Signed-off-by: Suyash Dongre <[email protected]>

ceph-jenkins · 2024-07-16T16:21:14Z

Can one of the admins verify this patch?

dmick · 2024-07-16T18:55:38Z

ceph-pr-clang-tidy/build/build

@@ -0,0 +1 @@
+sudo apt-get install -y clang-tidy


of course this only works if the build machine is an apt-based system, which it may not be using your current node labels. It might be more appropriate to install the package inside the pbuilder image in that case, too, although it probably doesn't matter too much; there are ways of adding extra packages to the pbuilder image

We still need an answer for this; does clang-tidy exist for RHEL-like systems? or do we want to restrict this job to only apt systems?

It is available but for now lets keep the job to apt systems only

How is this accomplished? won't the command be executed (and fail) on all systems?

The job specifies the labels for the jenkins builder it wants; we'll need to add something like 'jammy' as a label, that should suffice

Where should I add this 'jammy' label?

job.node is the list of labels the builder must satisfy. You probably want jammy && small

job.node is the list of labels the builder must satisfy. You probably want jammy && small

@dmick I have added node: jammy && small

dmick · 2024-07-16T18:57:42Z

ceph-pr-clang-tidy/config/ceph-pr-clang-tidy.yml

+
+  parameters:
+  - string:
+      name: ghprbPullId


I suspect this is probably provided by the ghprb plugin, and doesn't need to/shouldn't be a settable parameter

What's the response to this?

I removed ghprbPullId as a settable parameter. However in others Jenkins jobs it is set as a settable parameter.

Reference:
1.

ceph-build/ceph-pull-requests-arm64/config/definitions/ceph-pull-requests-arm64.yml

Lines 13 to 16 in e8d18c7

parameters:

- string:

name: ghprbPullId

description: "the GitHub pull id, like '72' in 'ceph/pull/72'"

ceph-build/ceph-perf-pull-requests/config/definitions/ceph-perf-pull-requests.yml

Lines 137 to 140 in e8d18c7

parameters:

- string:

name: ghprbPullId

description: "the GitHub pull id, like '72' in 'ceph/pull/72'"

ceph-build/ceph-pull-requests/config/definitions/ceph-pull-requests.yml

Lines 27 to 30 in e8d18c7

parameters:

- string:

name: ghprbPullId

description: "the GitHub pull id, like '72' in 'ceph/pull/72'"

@dmick and what about this?

We can leave it in for now; I don't think it's necessary but it might be useful for testing

Signed-off-by: Suyash Dongre <[email protected]>

ceph-pr-clang-tidy/build/build

Signed-off-by: Suyash Dongre <[email protected]>

ceph-pr-clang-tidy/build/build

dmick

Let's resolve the outstanding comments

Signed-off-by: Suyash Dongre <[email protected]>

dmick · 2024-07-25T19:32:47Z

Let's change the name of LICENSE to LICENSE.clang-tidy-to-junit.py to clarify what the license applies to

dmick

address the couple more comments and then we'll set up a test job

Signed-off-by: Suyash Dongre <[email protected]>

Suyashd999 · 2024-07-26T19:42:15Z

address the couple more comments and then we'll set up a test job

Hi @dmick! I think we are ready for a test job.

However there is one problem. Clang-tidy requires compile_commands.json file for analysis for which we have to pass
-DCMAKE_EXPORT_COMPILE_COMMANDS:BOOL=ON to CMAKE. I have create a PR for that: #2270

This PR will also be need to be merged.

After this we are good to go.

Suyashd999 · 2024-07-30T16:34:44Z

address the couple more comments and then we'll set up a test job

Hi @dmick! I think we are ready for a test job.

However there is one problem. Clang-tidy requires compile_commands.json file for analysis for which we have to pass -DCMAKE_EXPORT_COMPILE_COMMANDS:BOOL=ON to CMAKE. I have create a PR for that: #2270

This PR will also be need to be merged.

After this we are good to go.

Hey @dmick can you please check this? Thank you

Suyashd999 · 2024-08-21T13:51:30Z

Maybe it's best to incorporate this as an optional step in the make-check job? make check already builds ceph, and will have a built ceph tree in the workspace (unlike a standalone jenkins job, which would have to redo the build process)

Hi @dmick !
After researching a bit I found out that we can use the Jenkins plugin 'Copy Artifact' which allows us to copy files and directories from different Jenkins jobs.

What we can do is allow make check to finish its execution and start our clang-tidy job. In our job it'll copy all the files from the job make check and then start clang-tidy.
I was able to make the copy artifact work in my local system. The build job after its successful completion would trigger our clang-tidy job. The clang-tidy job copies all the files from the build job in the form of a tar file and extracts it.

I think this is the best approach.

Please let me know what you think.

dmick · 2024-08-21T18:33:00Z

That sounds like a good way to address the concerns. IIRC this involves a change to the make check job to add permissions to allow clang-tidy to use the artifacts.

Suyashd999 · 2024-08-21T18:37:17Z

That sounds like a good way to address the concerns. IIRC this involves a change to the make check job to add permissions to allow clang-tidy to use the artifacts.

Yes you are right. You will also need to install the plugin 'Copy Artifact' Please do so

dmick · 2024-08-21T18:44:45Z

Copy artifact is installed. It's used in the ceph-build jobs (search for "copyartifact")

Suyashd999 · 2024-08-21T18:53:00Z

Copy artifact is installed. It's used in the ceph-build jobs (search for "copyartifact")

@dmick
Okay. I'll need to change the make-check job to archive the files right? And where would I need to add the code to automatically trigger the clang-tidy job after make-check job?

dmick · 2024-08-21T20:38:29Z

Yes, the make check job will both archive the files and trigger the clang-tidy job

Instead of building Ceph, existing `build` would be copied from the job `ceph-pull-requests` (make-check) to save resources on building Ceph. Signed-off-by: Suyash Dongre <[email protected]>

Suyashd999 · 2024-08-24T12:17:39Z

Yes, the make check job will both archive the files and trigger the clang-tidy job

Hi @dmick. I have created a PR for archiving Ceph and triggering the job ceph-pr-clang-tidy. Can you please take a look at this? #2276
Thanks!

Suyashd999 · 2024-08-25T21:14:18Z

Yes, the make check job will both archive the files and trigger the clang-tidy job

Hi @dmick. I have created a PR for archiving Ceph and triggering the job ceph-pr-clang-tidy. Can you please take a look at this? #2276 Thanks!

Hey @dmick! could you please look into this? Thank you!

Suyashd999 · 2024-08-27T16:58:12Z

Hi! @dmick can you check this please?

Yes, the make check job will both archive the files and trigger the clang-tidy job

Hi @dmick. I have created a PR for archiving Ceph and triggering the job ceph-pr-clang-tidy. Can you please take a look at this? #2276 Thanks!

yuvalif · 2024-12-30T13:55:19Z

ceph-pr-clang-tidy/build/build

+set -ex
+
+# Clone the Ceph repository
+git clone https://github.com/ceph/ceph.git "$WORKSPACE/ceph"


can you do a shallow clone of only the relevant branch?
this would save considerable amount of time

yuvalif · 2024-12-30T13:56:09Z

ceph-pr-clang-tidy/build/build

+
+# Install dependencies
+sudo apt-get install -y curl
+./install-deps.sh


if we don't plan on building, why do we need "install-deps.sh" this also takes considerable amount of time

Signed-off-by: Suyash Dongre <[email protected]>

Suyashd999 · 2024-12-31T17:06:50Z

@yuvalif done can you please take a look now, Thank you

yuvalif · 2025-01-01T09:47:56Z

ceph-pr-clang-tidy/build/build

+cd build
+
+# Replace the boost and include directories from the tar file
+tar -xzf "$WORKSPACE/ceph_build.tar.gz" -C "$WORKSPACE"


where is the tar file being generated?
do we control the content of the file and make sure we only add boost and include dirs are there?

The tar file is generated in the Jenkins job make check. Once the job finishes its execution it will zip the boost and include directories.

Yes we control the content of tar file, it is being controlled in the pull request: #2276

yuvalif · 2025-01-01T09:50:17Z

@yuvalif done can you please take a look now, Thank you

do you have new time measurements after all of the latest improvements?

adding clang-tidy as linter tool

1d93369

Signed-off-by: Suyash Dongre <[email protected]>

dmick reviewed Jul 16, 2024

View reviewed changes

Added build steps

f724d88

Signed-off-by: Suyash Dongre <[email protected]>

Suyashd999 commented Jul 21, 2024

View reviewed changes

ceph-pr-clang-tidy/build/build Outdated Show resolved Hide resolved

added the script clang-tidy-to-junit for report generation

efeb4fa

Signed-off-by: Suyash Dongre <[email protected]>

dmick reviewed Jul 23, 2024

View reviewed changes

ceph-pr-clang-tidy/build/build Outdated Show resolved Hide resolved

dmick requested changes Jul 23, 2024

View reviewed changes

Suyashd999 added 2 commits July 24, 2024 18:23

refactored clang-tidy-to-junit.py execution

31aead3

Signed-off-by: Suyash Dongre <[email protected]>

refactored status-context

600c035

Signed-off-by: Suyash Dongre <[email protected]>

ronen-fr requested a review from dmick July 24, 2024 13:25

dmick requested changes Jul 25, 2024

View reviewed changes

Suyashd999 added 2 commits July 27, 2024 01:04

renamed LICENSE and added node configuration

9935304

Signed-off-by: Suyash Dongre <[email protected]>

added node configuration

5e54b91

Signed-off-by: Suyash Dongre <[email protected]>

Suyashd999 requested a review from dmick July 26, 2024 19:42

dmick mentioned this pull request Aug 2, 2024

CMAKE compile_commands flag addition #2270

Open

configured to work with copy_artifacts plugin

d64fdaa

Instead of building Ceph, existing `build` would be copied from the job `ceph-pull-requests` (make-check) to save resources on building Ceph. Signed-off-by: Suyash Dongre <[email protected]>

Suyashd999 force-pushed the main branch from 5b6158d to d64fdaa Compare August 24, 2024 09:06

Suyashd999 mentioned this pull request Aug 24, 2024

archiving Ceph's directories for the job 'ceph-pr-clang-tidy' #2276

Open

Suyashd999 added 2 commits November 24, 2024 16:17

updated to run clang-tidy on all files

498e2a3

Working with new "build/boost" and "build/include" from the tar file

84e8418

yuvalif reviewed Dec 30, 2024

View reviewed changes

uses shallow clone

df5dc10

Signed-off-by: Suyash Dongre <[email protected]>

yuvalif reviewed Jan 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding clang-tidy as a linter tool #2269

adding clang-tidy as a linter tool #2269

Suyashd999 commented Jul 16, 2024

ceph-jenkins commented Jul 16, 2024

dmick Jul 16, 2024

dmick Jul 23, 2024

Suyashd999 Jul 24, 2024

ronen-fr Jul 24, 2024

dmick Jul 24, 2024

Suyashd999 Jul 25, 2024

dmick Jul 25, 2024

Suyashd999 Jul 26, 2024

dmick Jul 16, 2024

dmick Jul 23, 2024

Suyashd999 Jul 24, 2024

Suyashd999 Jul 25, 2024

dmick Jul 25, 2024

dmick left a comment

dmick commented Jul 25, 2024

dmick left a comment

Suyashd999 commented Jul 26, 2024 •

edited

Loading

Suyashd999 commented Jul 30, 2024

Suyashd999 commented Aug 21, 2024

dmick commented Aug 21, 2024

Suyashd999 commented Aug 21, 2024

dmick commented Aug 21, 2024

Suyashd999 commented Aug 21, 2024 •

edited

Loading

dmick commented Aug 21, 2024

Suyashd999 commented Aug 24, 2024

Suyashd999 commented Aug 25, 2024

Suyashd999 commented Aug 27, 2024

yuvalif Dec 30, 2024

yuvalif Dec 30, 2024

Suyashd999 commented Dec 31, 2024

yuvalif Jan 1, 2025

Suyashd999 Jan 1, 2025

yuvalif commented Jan 1, 2025

	parameters:
	- string:
	name: ghprbPullId
	description: "the GitHub pull id, like '72' in 'ceph/pull/72'"

adding clang-tidy as a linter tool #2269

Are you sure you want to change the base?

adding clang-tidy as a linter tool #2269

Conversation

Suyashd999 commented Jul 16, 2024

ceph-jenkins commented Jul 16, 2024

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

dmick left a comment

Choose a reason for hiding this comment

dmick commented Jul 25, 2024

dmick left a comment

Choose a reason for hiding this comment

Suyashd999 commented Jul 26, 2024 • edited Loading

Suyashd999 commented Jul 30, 2024

Suyashd999 commented Aug 21, 2024

dmick commented Aug 21, 2024

Suyashd999 commented Aug 21, 2024

dmick commented Aug 21, 2024

Suyashd999 commented Aug 21, 2024 • edited Loading

dmick commented Aug 21, 2024

Suyashd999 commented Aug 24, 2024

Suyashd999 commented Aug 25, 2024

Suyashd999 commented Aug 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Suyashd999 commented Dec 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuvalif commented Jan 1, 2025

Suyashd999 commented Jul 26, 2024 •

edited

Loading

Suyashd999 commented Aug 21, 2024 •

edited

Loading