Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMAKE compile_commands flag addition #2270

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Suyashd999
Copy link

The DCMAKE_EXPORT_COMPILE_COMMANDS flag generates the compile_commands.json file which contains the exact compiler calls for all translation units of the project in machine-readable form.

This file is used by many code analysis tools, IDE(s), refactoring tools, etc.

There is no harm in having the compile_commands.json file.

We want to integrate clang-tidy into the Ceph CI for which the compile_commands.json file is necessary.

@ceph-jenkins
Copy link

Can one of the admins verify this patch?

Copy link
Member

@dmick dmick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the commit message follow the standard format described at https://github.com/ceph/ceph/blob/main/SubmittingPatches.rst#describe-your-changes

@dmick
Copy link
Member

dmick commented Aug 1, 2024

How large is the .json file? where is it placed? Does this mean that the clang-tidy job must be run after the ceph build job? How is that going to fit in our job workflow?

@@ -960,7 +960,7 @@ build_debs() {

echo building debs for $DIST

CEPH_EXTRA_CMAKE_ARGS="$CEPH_EXTRA_CMAKE_ARGS $(extra_cmake_args)"
CEPH_EXTRA_CMAKE_ARGS="$CEPH_EXTRA_CMAKE_ARGS $(extra_cmake_args)" + " -DCMAKE_EXPORT_COMPILE_COMMANDS:BOOL=ON"
Copy link
Contributor

@tchaikov tchaikov Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, scripts/build_utils.sh is used by the CI for building packages. and developers always just use the packages for testing and for redistributing Ceph. if that's the case, who will be reading the output of the linter?

Copy link
Member

@dmick dmick Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2269, as mentioned above, is meant to be a check job, but if it's got to run after a ceph build, I'm not sure how it fits in either.

@Suyashd999
Copy link
Author

Suyashd999 commented Aug 2, 2024

@dmick

How large is the .json file? where is it placed? Does this mean that the clang-tidy job must be run after the ceph build job? How is that going to fit in our job workflow?

How large is the .json file?

The .json file is about 50MB.

where is it placed?

There is no specific constraint on where it needs to be placed, we can place it anywhere, as of now I am placing it in the build directory.

Does this mean that the clang-tidy job must be run after the ceph build job?

Yes

How is that going to fit in our job workflow?

After build_debs() finishes execution it would have created the compile_commands.json and stored it in the build directory (considering we are going to store it in build) then clang-tidy will be specified the directory which has the compile_commands.json file. clang-tidy will then start its execution.

Reference: After the build_debs finishes we are starting clang-tidy

@dmick
Copy link
Member

dmick commented Aug 2, 2024

but...so...that means clang-tidy isn't a separate Jenkins job anymore, or a pre-check for PRs, it's a step of the actual build. That's unlike anything else in the build structure. What do we do if the build succeeds but clang-tidy fails?

I had started out assuming that clang-tidy was going to be running on ceph PRs like other checks, but now that seems very different from the flow it may need.

The DCMAKE_EXPORT_COMPILE_COMMANDS flag generates the `compile_commands.json` file which contains  the exact compiler calls for all translation units of the project in machine-readable form.

This file is used by many code analysis tools, IDE(s), refactoring tools, etc.

There is no harm in having the `compile_commands.json` file.

We want to integrate clang-tidy into the Ceph CI for which the `compile_commands.json` file is necessary.

Signed-off-by: Suyash Dongre <[email protected]>
@Suyashd999
Copy link
Author

but...so...that means clang-tidy isn't a separate Jenkins job anymore, or a pre-check for PRs, it's a step of the actual build. That's unlike anything else in the build structure. What do we do if the build succeeds but clang-tidy fails?

I had started out assuming that clang-tidy was going to be running on ceph PRs like other checks, but now that seems very different from the flow it may need.

What do we do if the build succeeds but clang-tidy fails?

It doesn't matter if clang-tidy fails, in fact we want it to "fail". The only purpose of clang-tidy is to pop warnings/errors to users.

I had started out assuming that clang-tidy was going to be running on ceph PRs like other checks,

It still is. The only change that we are making is to get the .json file which the clang-tidy needs. That .json file can only be generated by cmake. There would be no alteration to the workflow whatsoever.

@dmick
Copy link
Member

dmick commented Aug 2, 2024

oh, but, no...the clang-tidy job is attempting to "build_debs()" itself. That's not going to work; the build is a long resource-intensive process, and there's a lot more going on than just build_debs(); that depends on being run from a jenkins job that includes a -setup stage.

We're going to need to rethink this.

@Suyashd999
Copy link
Author

Suyashd999 commented Aug 2, 2024

oh, but, no...the clang-tidy job is attempting to "build_debs()" itself. That's not going to work; the build is a long resource-intensive process, and there's a lot more going on than just build_debs(); that depends on being run from a jenkins job that includes a -setup stage.

We're going to need to rethink this.

I see.. We can just run the cmake to get the .json file. But the issue arises when clang-tidy cannot find the boost module which is installed when building. That is why I was going on with building ceph. Is there a way to install the boost module without the traditional build? If we are able to achieve this then there is no need to build ceph at all.

@dmick
Copy link
Member

dmick commented Aug 2, 2024

I don't know. Also, even "just running cmake" depends on stuff in ./install-deps.sh, which for Debian builds would need to be running inside a pbuilder environment... this is totally new ground.

@Suyashd999
Copy link
Author

I don't know. Also, even "just running cmake" depends on stuff in ./install-deps.sh, which for Debian builds would need to be running inside a pbuilder environment... this is totally new ground.

what should we do now? What do you think is an appropriate solution?

@Suyashd999
Copy link
Author

@dmick is it possible that I manually build Ceph instead of build_debs() inside the pbuilder environment? By following the steps specified in the README

@Suyashd999
Copy link
Author

Hi @dmick can you please check the above comments? Thanks!

@dmick
Copy link
Member

dmick commented Aug 5, 2024

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)

@Suyashd999
Copy link
Author

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)

@dmick
We can but make check is a required check and we want clang-tidy to be a non-blocking check. Can we add clang-tidy to another job?

@Suyashd999
Copy link
Author

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)

I have suggested a solution for this please check: #2269 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants