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.

Sign up for GitHub

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

bazel/clang-tidy: Cleanups and optimizations #2496

Merged
merged 2 commits into from
Dec 2, 2024
Merged

bazel/clang-tidy: Cleanups and optimizations #2496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f55e394
bazel/clang-tidy: Cleanups and optimizations
phlax Nov 27, 2024
821544a
use file extension for sources
phlax Dec 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 22 additions & 53 deletions bazel/format/clang_tidy/clang_tidy.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
load("@bazel_tools//tools/build_defs/cc:action_names.bzl", "ACTION_NAMES")
load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain")

TARGET_EXTENSIONS = [
"c", "cc", "cpp", "cxx", "c++", "C", "h", "hh", "hpp", "hxx", "inc", "inl", "H",
]

def _run_tidy(
ctx,
wrapper,
Expand All @@ -11,62 +15,43 @@ def _run_tidy(
compilation_context,
infile,
discriminator):
direct_deps = [infile, config]
if exe.files_to_run.executable:
direct_deps.append(exe.files_to_run.executable)
if additional_deps.files:
direct_deps.append(additional_deps.files)
inputs = depset(
direct = (
[infile, config] +
additional_deps.files.to_list() +
([exe.files_to_run.executable] if exe.files_to_run.executable else [])
),
direct = direct_deps,
transitive = [compilation_context.headers],
)

args = ctx.actions.args()

# specify the output file - twice
outfile = ctx.actions.declare_file(
"bazel_clang_tidy_" + infile.path + "." + discriminator + ".clang-tidy.yaml",
"bazel_clang_tidy_%s.%s.clang-tidy.yaml" % (infile.path, discriminator)
)

# this is consumed by the wrapper script
if len(exe.files.to_list()) == 0:
args.add("clang-tidy")
else:
args.add(exe.files_to_run.executable)

args.add(outfile.path) # this is consumed by the wrapper script

args.add(exe.files_to_run.executable if exe.files else "clang-tidy")
args.add(outfile.path)
args.add(config.path)

args.add("--export-fixes", outfile.path)

args.add("--quiet")

# add source to check
args.add(infile.path)

# start args passed to the compiler
args.add("--")

# add args specified by the toolchain, on the command line and rule copts
args.add_all(flags)

# add defines
for define in compilation_context.defines.to_list():
args.add("-D" + define)

for define in compilation_context.local_defines.to_list():
args.add("-D" + define)

args.add_all(compilation_context.defines, before_each = "-D")
args.add_all(compilation_context.local_defines, before_each = "-D")
# add includes
for i in compilation_context.framework_includes.to_list():
args.add("-F" + i)

for i in compilation_context.includes.to_list():
args.add("-I" + i)

args.add_all(compilation_context.quote_includes.to_list(), before_each = "-iquote")

args.add_all(compilation_context.system_includes.to_list(), before_each = "-isystem")
args.add_all(compilation_context.framework_includes, before_each = "-F")
args.add_all(compilation_context.includes, before_each = "-I")
args.add_all(compilation_context.quote_includes, before_each = "-iquote")
args.add_all(compilation_context.system_includes, before_each = "-isystem")

ctx.actions.run(
inputs = inputs,
Expand All @@ -80,22 +65,10 @@ def _run_tidy(
return outfile

def _rule_sources(ctx):
def check_valid_file_type(src):
"""
Returns True if the file type matches one of the permitted srcs file types for C and C++ header/source files.
"""
permitted_file_types = [
".c", ".cc", ".cpp", ".cxx", ".c++", ".C", ".h", ".hh", ".hpp", ".hxx", ".inc", ".inl", ".H",
]
for file_type in permitted_file_types:
if src.basename.endswith(file_type):
return True
return False

srcs = []
if hasattr(ctx.rule.attr, "srcs"):
for src in ctx.rule.attr.srcs:
srcs += [src for src in src.files.to_list() if src.is_source and check_valid_file_type(src)]
srcs += [_src for _src in src.files.to_list() if _src.is_source and _src.extension in TARGET_EXTENSIONS]
return srcs

def _toolchain_flags(ctx, action_name = ACTION_NAMES.cpp_compile):
Expand Down Expand Up @@ -124,7 +97,6 @@ def _safe_flags(flags):
"-fno-canonical-system-headers",
"-fstack-usage",
]

return [flag for flag in flags if flag not in unsupported_flags and not flag.startswith("--sysroot")]

def _clang_tidy_aspect_impl(target, ctx):
Expand All @@ -143,9 +115,6 @@ def _clang_tidy_aspect_impl(target, ctx):
ACTION_NAMES.cpp_compile: _safe_flags(_toolchain_flags(ctx, ACTION_NAMES.cpp_compile) + rule_flags),
ACTION_NAMES.c_compile: _safe_flags(_toolchain_flags(ctx, ACTION_NAMES.c_compile) + rule_flags),
}

srcs = _rule_sources(ctx)

outputs = [
_run_tidy(
ctx,
Expand All @@ -158,11 +127,11 @@ def _clang_tidy_aspect_impl(target, ctx):
src,
target.label.name,
)
for src in srcs
for src in _rule_sources(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

I think this outputs can be one run i guess? i don't see any reason why we cannot run clang-tidy for all source codes in the target at once vs here run each file per clang-tidy.

Copy link
Member Author

Choose a reason for hiding this comment

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

let me test ...

Copy link
Member Author

@phlax phlax Dec 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

so the one issue with this plan is that the flags are set according to whether src is .c or .cc

a quick grep of the code suggests there are not any builds that have both as a src (not sure that would make sense - but what do i know) - so probably surmountable

on the upsides it makes the code cleaner and removes the need to use to_list on the srcs

Copy link
Member

@mathetake mathetake Dec 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i believe all Envoy codes are C++ hence .cc and .c are for test C program code (not clang-tidy worthy), so we can simply ignore them

Copy link
Member Author

@phlax phlax Dec 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

so next problem is

 38     outfile = ctx.actions.declare_file(
 39         "bazel_clang_tidy_%s.%s.clang-tidy.yaml" % (infile.path, discriminator)
 40     )

i can workaround for now - i think in the original implementation this was intended for the fix file altho not sure it was actually implemented

my intention is to use this output for the output from clang-tidy and then collect those up so probs we can think of a different path scheme

Copy link
Member Author

Choose a reason for hiding this comment

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

lets deal with this separately - the benefit is marginal at best and it requires quite a lot of change to make it work

]

return [
OutputGroupInfo(report = depset(direct = outputs)),
OutputGroupInfo(report = depset(direct = outputs))
]

clang_tidy_aspect = aspect(
Expand Down
2 changes: 2 additions & 0 deletions bazel/format/clang_tidy/run_clang_tidy.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,6 @@ truncate -s 0 "$OUTPUT"
# place it in the current directory
test -e .clang-tidy || ln -s -f "$CONFIG" .clang-tidy

# echo "RUN: ${CLANG_TIDY_BIN} ${*}"

Comment on lines +24 to +25
Copy link
Member

Choose a reason for hiding this comment

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

leftover?

Copy link
Member Author

Choose a reason for hiding this comment

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

i figured keep it - its kinda useful for debugging

"${CLANG_TIDY_BIN}" "$@" |& grep -v "warnings generated" || :