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

Run Clang-Tidy on taskcluster #9244

Merged
merged 28 commits into from
Apr 2, 2024
Merged

Run Clang-Tidy on taskcluster #9244

merged 28 commits into from
Apr 2, 2024

Conversation

strseb
Copy link
Collaborator

@strseb strseb commented Mar 19, 2024

This PR adds taskcluster jobs to run clang-tidy. Given we have diffrent c++ source files for each plattform i did create Tasks covering Android,Windows,Macos which should cover most users.

iOS and Linux should be easy to add, but given this pr is already 27 commit's this will be a Followup.

I also touched the clang-tidy file, adding a few checks, however only clang-analyzer-security.* will produce errors, therefore can block merging, for now :)

@strseb strseb requested a review from brizental March 20, 2024 19:08
@strseb strseb marked this pull request as ready for review March 20, 2024 19:08
@strseb strseb requested a review from a team as a code owner March 20, 2024 19:08
@strseb strseb requested review from bhearsum and removed request for a team March 20, 2024 19:08
@strseb strseb changed the title WIP: Clang-Tidy on taskcluster Run Clang-Tidy on taskcluster Mar 20, 2024
@brizental
Copy link
Contributor

brizental commented Mar 21, 2024

This PR adds taskcluster jobs to run clang-tidy. Given we have diffrent c++ source files for each plattform i did create Tasks covering Android,Windows,Macos which should cover most users.

Does clang-tidy need to build the files or is it just a static analyzer?

@strseb
Copy link
Collaborator Author

strseb commented Mar 21, 2024

Does clang-tidy need to build the files or is it just a static analyzer?

Yes,ish? It is invoking clang to get the AST so it's only skipping generating bitcode from that AST, so Compilation would need to work to get a proper ast, so we can't just drop it any file.

@brizental
Copy link
Contributor

Yes,ish? It is invoking clang to get the AST so it's only skipping generating bitcode from that AST, so Compilation would need to work to get a proper ast, so we can't just drop it any file.

I know I am being annoying about this but... It seems excessive to run a static analyzer multiple times in different platforms just to analyze all files when it can be done by any of them only once. For local development I am still not a big fan, but I get that attempting to figure out what is built for each platform is a pain and something locally we might need more.

Idea: what about running cland-tidy only filed touched since commit X? Or only on staged files if no commit is provided?

@strseb
Copy link
Collaborator Author

strseb commented Mar 25, 2024

analyze all files when it can be done by any of them only once.

for a few files that might be true but consider i.e controller.cpp, it's used in all builds

#if defined(MZ_FLATPAK)
#  include "platforms/linux/networkmanagercontroller.h"
#elif defined(MZ_LINUX)
#  include "platforms/linux/linuxcontroller.h"
#elif defined(MZ_MACOS) || defined(MZ_WINDOWS)
#  include "localsocketcontroller.h"
#elif defined(MZ_IOS)
#  include "platforms/ios/ioscontroller.h"
#elif defined(MZ_ANDROID)
#  include "platforms/android/androidcontroller.h"
#else
#  include "platforms/dummy/dummycontroller.h"
#endif

On each platform we have completely different shapes of the input object (even 2 different for linux), depending on the compiler arguments. Even on "cross-plattform" sources.
On excessiveness, i really do not think that ~10 minutes per target is excessive when it covers the whole codebase, that's roughly 1,5 functional test suites :)

Idea: what about running cland-tidy only filed touched since commit X? Or only on staged files if no commit is provided?

Sure i like that, for local development, on ci this would hide existing errors.

@brizental
Copy link
Contributor

brizental commented Mar 25, 2024

Ok, I think I misunderstood your first answer i.e. clang-tidy is not just a static analyzer 🤦‍♀️

Copy link
Contributor

@brizental brizental left a comment

Choose a reason for hiding this comment

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

r+wc

mz_add_clang_tidy(shared-sources)
if(TARGET shared-sources_clang_tidy_report)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this condition required here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do not generate that target i.e if there is no clang-tidy binary found, so the target may or may not exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure but the dependencies are being added into this target, so if we don't build this the add_depencies also doesn't do anything.

Copy link
Contributor

@brizental brizental Mar 27, 2024

Choose a reason for hiding this comment

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

AH nvm, we don't even create the target.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, if you don't even create it the TARGET keyword is prob unnecessary. It's a nit though.

@strseb strseb merged commit 8360943 into main Apr 2, 2024
132 checks passed
@strseb strseb deleted the tidy_ci branch April 2, 2024 15:57
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.

2 participants