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

Support clang-tidy (early PR for review) #284

Closed
wants to merge 24 commits into from

Conversation

peakschris
Copy link
Contributor

@peakschris peakschris commented Jun 14, 2024

Support clang-tidy for linting of C++ code.

Inspired by and containing some code from bazel_clang_tidy repo. This integration has been discussed by maintainers of both repos in erenon/bazel_clang_tidy#35.


Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: yes
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: todo

Test plan

Needs discussion

  • Covered by existing test cases
  • New test cases added
  • Manual testing; please provide instructions so we can reproduce:

Code used from bazel_clang_tidy repo

Two helper functions toolchain_flags and safe_flags have been copied from bazel_clang_tidy and used in clang_tidy.bzl. Also, some lines in get_args are copied from bazel_clang_tidy. bazel_clang_tidy was also a very helpful learning tool in how to integrate clang-tidy into a bazel aspect. The rest of the code is implemented by following existing rules_lint code such as ruff.bzl and eslint.bzl.

Release notes (early input, will reformat)

  • warning that git bash does not handle arguments correctly and --fix may have issues on Windows. msys64 bash (already recommended by Bazel is ok).

@CLAassistant
Copy link

CLAassistant commented Jun 14, 2024

CLA assistant check
All committers have signed the CLA.

@peakschris
Copy link
Contributor Author

peakschris commented Jun 14, 2024

Known issues with this MR:

  1. ./lint.sh --fix //src/cpp/main/... fails on linux with:
ERROR: /apps/browchri/rules_lint/example/src/cpp/main/BUILD:3:11: output 'src/cpp/main/cpp/main/hello-greet.cc.AspectRulesLintClangT                                                idy.report' was not created
ERROR: /apps/browchri/rules_lint/example/src/cpp/main/BUILD:3:11: output 'src/cpp/main/cpp/main/hello-greet.cc.AspectRulesLintClangT                                                idy.exit_code' was not created
ERROR: /apps/browchri/rules_lint/example/src/cpp/main/BUILD:3:11: Linting //src/cpp/main:hello-greet with clang-tidy failed: not all                                                 outputs were created or valid
  1. Not sure if my approach to handle multiple srcs whilst clang-tidy only handles one is valid or optimal.
  2. Would like guidance on refactoring/adding APIs to replace my copies of report_files and patch_file that allow per-file reports/patches
  3. Could use guidance on how to construct a relative filename for these report files for files that are in subdirectories.

Known issues with repo on windows, not related to this MR:

  1. bash lint.sh --fix //src/cpp/main/... fails on windows with missing node.bat wrapper error from rules_js
ERROR: D:/workdir/rules_lint3/example/src/cpp/main/BUILD:10:10: Linting //src/cpp/main:hello-world with clang-tidy failed: (Exit 1): patcher.bat failed: error executing AspectRulesLintClangTidy command (from target //src/cpp/main:hello-world) bazel-out\x64_windows-opt-exec-ST-13d3ddad9198\bin\external\aspect_rules_lint~\lint\private\patcher_\patcher.bat bazel-out/x64_windows-fastbuild/bin/src/cpp/main/_hello-world.patch_cfg
FATAL: aspect_rules_js[js_binary]: node wrapper '/d/udu/b/l2x2bhay/execroot/_main/./external/aspect_rules_lint~/lint/private/patcher_node_bin/node.bat' not found
  1. Various issues with support for windows in the examples repo; workarounds applied by workaround_windows.bat but would guidance on correcting these properly.

Copy link
Member

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Wow, this looks like a great start. I'll find some more time to comb through the details soon.

###############################################
# START COPY CODE FROM private/lint_aspect.bzl
#
# this code needed to be changed to support generation of a report file
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, why is clang-tidy the only tool that needs to produce different number of reports? I think this will break the API this ruleset exposes to surrounding tools that want to present the results - they assume there is a report for each action. Maybe we can merge them?

@peakschris
Copy link
Contributor Author

I'm replacing this PR with #287, addressing the comment about report files and cleaning up the commit history.

@peakschris peakschris closed this Jun 15, 2024
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.

3 participants