feat: clang-tidy emit action per file to lint#512
feat: clang-tidy emit action per file to lint#512molar wants to merge 2 commits intoaspect-build:mainfrom
Conversation
molar
changed the title
molar
changed the title
molar
changed the title
|
Friendly ping @alexeagle do you have some input or can you kindly suggest a reviewer? |
|
I'm sorry - I looked through this once and then got in my head that it was blocked on something. It's a bit complex/gross to have to merge the exit codes back together this way. like you say, it's using non-hermetic system tools, plus space delimiting is risky when filenames can have spaces, so may need some quoting work. Is it possible to move the splitting earlier? I'm imagining the aspect visits a cc_library target and produces an action per file at that point. Bazel sees all the actions and can react to the non-zero exit code(s) itself. I think there's no need to aggregate the results back to the library level, right? |
|
There's a new feature coming in Bazel that's likely useful here, action templates. I think we might want to wait and re-base on that rather than do something difficult and short-term |
|
@molar are you still interested here? Bazel 9 has |
|
@alexeagle Thank you for your time on this. I will not be working on this for the time being unfortunately. |
|
|
This is similar - with similar motivations - to the feature proposed in aspect-build#512 I worked on this around the same time and stumbled upon that PR only recently. The approach this patch follows is different however and I think in line with what @alexeagle suggested in [this comment](aspect-build#512 (comment)), i.e. one action is generated per file. The way this is introduced should make it possible to re-use this logic to implement similar per-file logic for other linters. To do so we: - add a new parameter to output_files/patch_files so that they can declare per-file outputs/patches - adapt the _lint_test rule so that it can handle multiple output/exit_code files - use the new capability in the clang-tidy aspect implementation I believe fixing works correctly with this approach although we do not use the Aspect CLI for this but a shell script similar to the lint.sh gist. We have been using this patch for a while on version 1.10.2 with no issues. I have re-adapted the patch for the version 2.0.0 of the ruleset but the changes were pretty much the same. I have also checked the unit tests of this repo and the cpp example.
This is similar - with similar motivations - to the feature proposed in aspect-build#512 I worked on this around the same time and stumbled upon that PR only recently. The approach this patch follows is different however and I think in line with what @alexeagle suggested in [this comment](aspect-build#512 (comment)), i.e. one action is generated per file. The way this is introduced should make it possible to re-use this logic to implement similar per-file logic for other linters. To do so we: - add a new parameter to output_files/patch_files so that they can declare per-file outputs/patches - adapt the _lint_test rule so that it can handle multiple output/exit_code files - use the new capability in the clang-tidy aspect implementation I believe fixing works correctly with this approach although we do not use the Aspect CLI for this but a shell script similar to the lint.sh gist. We have been using this patch for a while on version 1.10.2 with no issues. I have re-adapted the patch for the version 2.0.0 of the ruleset but the changes were pretty much the same. I have also checked the unit tests of this repo and the cpp example.
This is similar - with similar motivations - to the feature proposed in #512 I worked on this around the same time and stumbled upon that PR only recently. The approach this patch follows is different however and I think in line with what @alexeagle suggested in [this comment](#512 (comment)), i.e. one action is generated per file. The way this is introduced should make it possible to re-use this logic to implement similar per-file logic for other linters. To do so we: - add a new parameter to output_files/patch_files so that they can declare per-file outputs/patches - adapt the _lint_test rule so that it can handle multiple output/exit_code files - use the new capability in the clang-tidy aspect implementation I believe fixing works correctly with this approach although we do not use the Aspect CLI for this but a shell script similar to the lint.sh gist. ### Test plan - Covered by existing test cases: confirmed the repo unit tests and the cpp example still work - Manual testing: we have been using this patch for a while on version 1.10.2 with no issues. I have re-adapted the patch for the version 2.0.0 of the ruleset but the changes were pretty much the same.
3 participants
Some cc_library/binary targets may contain 10+ files listed in sources and as clang-tidy runs single threaded, this can lead
to long running linting actions. This PR emits a clang-tidy action per source file, so they can be parallelised locally/remotely. The stdout/stderror is merged in unspecified order according to the order of the srcs using a simple
concatenation. The exit_code of the individual action, is merged by sorting them numerically and selecting the highest number.
I did also consider making the splitting optional, in case we are unsure of the impact of this change. Let me know if i should do that.
I also considered updated the progress message to use the source file short path instead, but decided to leave it out here. Currently the progress message will be the same for each individual action and makes it harder to see which source file is being linted.
Test plan
sortandheadon macOS.fixpart as we do not use the aspect wrapper in-house