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

Switch from bazel_clang_tidy to aspect-build/rules_lint for clang-tidy validaton #879

Merged
merged 4 commits into from
Feb 4, 2025

Conversation

Databean
Copy link
Member

@Databean Databean commented Feb 4, 2025

This comes with some tradeoffs:

  • It is possible to have different .clang-tidy files for subdirectories, as long as they are exported to the linting rule.
  • clang_tidy invocation now happens at a build step, concurrent with other compilation operations.
  • IIUC the actual validation happens in a test step, which reports success or failure based on the build step.
  • The validation can be quite slow, even for moderate sized targets. One target in //cuttlefish/common/libs/utils took about 2 minutes. Based on CPU utilization, it seems like clang_tidy is being run serially for all source files in a build target, and only different build targets can be validated in parallel with each other.

This will allow using clang-tidy in `bazel test` targets, and different
.clang-tidy files per directory. That will help with importing files
from other repositories.
@cjreynol
Copy link
Collaborator

cjreynol commented Feb 4, 2025

on my machine:

  • bazel clean then bazel build ... jumps from ~67 to ~190-200 seconds
  • changing cuttlefish/host/commands/cvd/cli/commands/cache.cpp and triggering an incremental build jumps from ~3-4 seconds to 120 seconds

The jump in clean build times I am not so concerned about, but the incremental build jump is steep. Based on your last comment we could maybe split up some of our targets to get a speedup, but I would rather not make library structure decisions based on our tooling speeds.

A quick search shows we could use something like bazel query results piped to bazel build avoid test targets, but that also has downsides.

I did test using tags = ["avoid_me"], on the clang_tidy_test targets and running bazel build --build_tag_filters="-avoid_me" ... which brings the times back to the current numbers.

https://bazel.build/docs/user-manual#build-tag-filters)

I would prefer something like the tags option (or anything similar) to be able to keep the incremental builds speed down.

@Databean
Copy link
Member Author

Databean commented Feb 4, 2025

The incremental build time is still good for specific targets like bazel run //cuttlefish/host/commands/cvd -- ... but you're right that it is easy to walk into a slow case with something like bazel build //....

@cjreynol
Copy link
Collaborator

cjreynol commented Feb 4, 2025

From our lunch conversations, I can just build the cvd target and skip the broad wildcard target.

... as suggested by cjreynol@. This enables running
```
bazel build --build_tag_filters="-clang-tidy" "//..."
```
@Databean
Copy link
Member Author

Databean commented Feb 4, 2025

I did test using tags = ["avoid_me"], on the clang_tidy_test targets and running bazel build --build_tag_filters="-avoid_me" ... which brings the times back to the current numbers.

Added the "clang_tidy" and "clang-tidy" tags to enable this strategy.

@Databean
Copy link
Member Author

Databean commented Feb 4, 2025

Thanks for reviewing!

@Databean Databean enabled auto-merge February 4, 2025 21:50
@Databean Databean added this pull request to the merge queue Feb 4, 2025
Merged via the queue into google:main with commit f697d8c Feb 4, 2025
25 checks passed
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