[quality] consolidate lint rule implementations

This consolidates the lint rule implementations for clang_format, gofmt, and protolint, so a common rule implementation and template script is used for all lint rules, which operate in a similar fashion. Additionally, this updates the clang_format lint rule to use the clang_format binary included in the lowRISC RV32 toolchain, for improved build reproducibility. Signed-off-by: Tim Trippel <[email protected]>
lowRISC · Sep 10, 2024 · b525b88 · b525b88
1 parent d26b688
commit b525b88
Show file tree

Hide file tree

Showing 12 changed files with 147 additions and 179 deletions.
diff --git a/quality/BUILD.bazel b/quality/BUILD.bazel
@@ -60,6 +60,7 @@ gofmt_check(
  name = "gofmt_check",
  mode = "diff",
  tags = ["lint"],
+ workspace = "//:WORKSPACE",
 )
 
 gofmt_fix(

diff --git a/rules/quality.bzl b/rules/quality.bzl
@@ -12,33 +12,39 @@ def _ensure_tag(tags, *tag):
  tags.append(t)
  return tags
 
-################################################################################
-# gofmt
-################################################################################
-def _gofmt_impl(ctx):
+def _general_lint_impl(ctx):
  out_file = ctx.actions.declare_file(ctx.label.name + ".bash")
  exclude_patterns = ["\\! -path {}".format(shell.quote(p)) for p in ctx.attr.exclude_patterns]
  include_patterns = ["-name {}".format(shell.quote(p)) for p in ctx.attr.patterns]
+ workspace = ctx.file.workspace.path if ctx.file.workspace else ""
  substitutions = {
  "@@EXCLUDE_PATTERNS@@": " ".join(exclude_patterns),
  "@@INCLUDE_PATTERNS@@": " -o ".join(include_patterns),
- "@@GOFMT@@": shell.quote(ctx.executable.gofmt.short_path),
- "@@DIFF_COMMAND@@": shell.quote(ctx.attr.diff_command),
+ "@@LINT_TOOL@@": shell.quote(ctx.executable.lint_tool.short_path),
  "@@MODE@@": shell.quote(ctx.attr.mode),
+ "@@WORKSPACE@@": workspace,
+ "@@RUNNER_SH@@": ctx.file._runner.path,
  }
  ctx.actions.expand_template(
- template = ctx.file._runner,
+ template = ctx.file._runner_general,
  output = out_file,
  substitutions = substitutions,
  is_executable = True,
  )
 
+ runfiles = [ctx.executable.lint_tool, ctx.file._runner]
+ if ctx.file.workspace:
+ runfiles.append(ctx.file.workspace)
+
  return DefaultInfo(
  files = depset([out_file]),
- runfiles = ctx.runfiles(files = [ctx.executable.gofmt]),
+ runfiles = ctx.runfiles(files = runfiles),
  executable = out_file,
  )
 
+################################################################################
+# gofmt
+################################################################################
 gofmt_attrs = {
  "patterns": attr.string_list(
  default = ["*.go"],
@@ -52,31 +58,35 @@ gofmt_attrs = {
  values = ["diff", "fix"],
  doc = "Execution mode: display diffs or fix formatting",
  ),
- "diff_command": attr.string(
- default = "diff -u",
- doc = "Command to execute to display diffs",
- ),
- "gofmt": attr.label(
+ "lint_tool": attr.label(
  default = "@go_sdk//:bin/gofmt",
  allow_single_file = True,
  cfg = "host",
  executable = True,
  doc = "The gofmt executable",
  ),
+ "workspace": attr.label(
+ allow_single_file = True,
+ doc = "Label of the WORKSPACE file",
+ ),
  "_runner": attr.label(
- default = "//rules/scripts:gofmt.template.sh",
+ default = "//rules/scripts:gofmt.sh",
+ allow_single_file = True,
+ ),
+ "_runner_general": attr.label(
+ default = "//rules/scripts:general_lint.template.sh",
  allow_single_file = True,
  ),
 }
 
 gofmt_fix = rule(
- implementation = _gofmt_impl,
+ implementation = _general_lint_impl,
  attrs = gofmt_attrs,
  executable = True,
 )
 
 _gofmt_test = rule(
- implementation = _gofmt_impl,
+ implementation = _general_lint_impl,
  attrs = gofmt_attrs,
  test = True,
 )
@@ -91,31 +101,6 @@ def gofmt_check(**kwargs):
 ################################################################################
 # clang-format
 ################################################################################
-def _clang_format_impl(ctx):
- out_file = ctx.actions.declare_file(ctx.label.name + ".bash")
- exclude_patterns = ["\\! -path {}".format(shell.quote(p)) for p in ctx.attr.exclude_patterns]
- include_patterns = ["-name {}".format(shell.quote(p)) for p in ctx.attr.patterns]
- workspace = ctx.file.workspace.path if ctx.file.workspace else ""
- substitutions = {
- "@@EXCLUDE_PATTERNS@@": " ".join(exclude_patterns),
- "@@INCLUDE_PATTERNS@@": " -o ".join(include_patterns),
- "@@CLANG_FORMAT@@": shell.quote(ctx.attr.clang_format_command),
- "@@DIFF_COMMAND@@": shell.quote(ctx.attr.diff_command),
- "@@MODE@@": shell.quote(ctx.attr.mode),
- "@@WORKSPACE@@": workspace,
- }
- ctx.actions.expand_template(
- template = ctx.file._runner,
- output = out_file,
- substitutions = substitutions,
- is_executable = True,
- )
-
- return DefaultInfo(
- files = depset([out_file]),
- executable = out_file,
- )
-
 clang_format_attrs = {
  "patterns": attr.string_list(
  default = ["*.c", "*.h", "*.cc", "*.cpp"],
@@ -129,32 +114,35 @@ clang_format_attrs = {
  values = ["diff", "fix"],
  doc = "Execution mode: display diffs or fix formatting",
  ),
- "diff_command": attr.string(
- default = "diff -u",
- doc = "Command to execute to display diffs",
- ),
- "clang_format_command": attr.string(
- default = "clang-format",
+ "lint_tool": attr.label(
+ default = "@clang-format//:clang-format",
+ allow_single_file = True,
+ cfg = "host",
+ executable = True,
  doc = "The clang-format executable",
  ),
  "workspace": attr.label(
  allow_single_file = True,
  doc = "Label of the WORKSPACE file",
  ),
  "_runner": attr.label(
- default = "//rules/scripts:clang_format.template.sh",
+ default = "//rules/scripts:clang_format.sh",
+ allow_single_file = True,
+ ),
+ "_runner_general": attr.label(
+ default = "//rules/scripts:general_lint.template.sh",
  allow_single_file = True,
  ),
 }
 
 clang_format_fix = rule(
- implementation = _clang_format_impl,
+ implementation = _general_lint_impl,
  attrs = clang_format_attrs,
  executable = True,
 )
 
 _clang_format_test = rule(
- implementation = _clang_format_impl,
+ implementation = _general_lint_impl,
  attrs = clang_format_attrs,
  test = True,
 )
@@ -169,36 +157,6 @@ def clang_format_check(**kwargs):
 ################################################################################
 # protolint
 ################################################################################
-def _protolint_impl(ctx):
- out_file = ctx.actions.declare_file(ctx.label.name + ".bash")
- exclude_patterns = ["\\! -path {}".format(shell.quote(p)) for p in ctx.attr.exclude_patterns]
- include_patterns = ["-name {}".format(shell.quote(p)) for p in ctx.attr.patterns]
- workspace = ctx.file.workspace.path if ctx.file.workspace else ""
- substitutions = {
- "@@EXCLUDE_PATTERNS@@": " ".join(exclude_patterns),
- "@@INCLUDE_PATTERNS@@": " -o ".join(include_patterns),
- "@@PROTOLINT@@": shell.quote(ctx.executable.protolint.short_path),
- "@@MODE@@": shell.quote(ctx.attr.mode),
- "@@WORKSPACE@@": workspace,
- }
- ctx.actions.expand_template(
- template = ctx.file._runner,
- output = out_file,
- substitutions = substitutions,
- is_executable = True,
- )
-
- runfiles = [ctx.executable.protolint]
- if ctx.file.workspace:
- runfiles.append(ctx.file.workspace)
-
- return DefaultInfo(
- files = depset([out_file]),
- runfiles = ctx.runfiles(files = runfiles),
- executable = out_file,
- )
- return
-
 protolint_attrs = {
  "patterns": attr.string_list(
  default = ["*.proto"],
@@ -212,7 +170,7 @@ protolint_attrs = {
  values = ["diff", "fix"],
  doc = "Execution mode: display diffs or fix formatting.",
  ),
- "protolint": attr.label(
+ "lint_tool": attr.label(
  default = "@protolint//:protolint",
  allow_single_file = True,
  cfg = "host",
@@ -224,19 +182,23 @@ protolint_attrs = {
  doc = "Label of the WORKSPACE file",
  ),
  "_runner": attr.label(
- default = "//rules/scripts:protolint.template.sh",
+ default = "//rules/scripts:protolint.sh",
+ allow_single_file = True,
+ ),
+ "_runner_general": attr.label(
+ default = "//rules/scripts:general_lint.template.sh",
  allow_single_file = True,
  ),
 }
 
 protolint_fix = rule(
- implementation = _protolint_impl,
+ implementation = _general_lint_impl,
  attrs = protolint_attrs,
  executable = True,
 )
 
 _protolint_test = rule(
- implementation = _protolint_impl,
+ implementation = _general_lint_impl,
  attrs = protolint_attrs,
  test = True,
 )

diff --git a/rules/scripts/clang_format.sh b/rules/scripts/clang_format.sh
@@ -0,0 +1,21 @@
+#!/usr/bin/env bash
+# Copyright lowRISC contributors (OpenTitan project).
+# Licensed under the Apache License, Version 2.0, see LICENSE for details.
+# SPDX-License-Identifier: Apache-2.0
+
+case "$MODE" in
+ diff)
+ RESULT=0
+ for f in $FILES; do
+ diff -Naur "$f" <(${lint_tool} ${f})
+ RESULT=$(($RESULT | $?))
+ done
+ exit $RESULT
+ ;;
+ fix)
+ echo "$FILES" | xargs ${lint_tool} -i
+ ;;
+ *)
+ echo "Unknown mode: $MODE"
+ exit 2
+esac
diff --git a/rules/scripts/clang_format.template.sh b/rules/scripts/clang_format.template.sh
diff --git a/rules/scripts/protolint.template.sh → rules/scripts/general_lint.template.sh b/rules/scripts/protolint.template.sh → rules/scripts/general_lint.template.sh
@@ -3,11 +3,14 @@
 # Licensed under the Apache License, Version 2.0, see LICENSE for details.
 # SPDX-License-Identifier: Apache-2.0
 
-PROTOLINT=@@PROTOLINT@@
+# Set mode and lint tool.
+LINT_TOOL=@@LINT_TOOL@@
 MODE=@@MODE@@
 WORKSPACE="@@WORKSPACE@@"
+RUNNER_SH=@@RUNNER_SH@@
 
-protolint=$(readlink "$PROTOLINT")
+lint_tool=$(readlink "$LINT_TOOL")
+runner_sh=$(readlink "$RUNNER_SH")
 
 # Change directories based on whether the mode is to "fix" or to "diff".
 if [[ -n "${WORKSPACE}" ]]; then
@@ -32,16 +35,10 @@ else
  -print)
 fi
 
-# Perfom the "diff" or "fix" operation.
-case "$MODE" in
- diff)
- echo "$FILES" | xargs "${protolint}"
- exit $?
- ;;
- fix)
- echo "$FILES" | xargs "${protolint}" -fix
- ;;
- *)
- echo "Unknown mode: $MODE"
- exit 2
-esac
+if [[ -z "$FILES" ]]; then
+ echo "Error no files found to lint for pattern: \"$INCLUDE_PATTERNS\"."
+ exit 1
+fi
+
+# Execute the runner script.
+source "$runner_sh"
diff --git a/rules/scripts/gofmt.sh b/rules/scripts/gofmt.sh
@@ -0,0 +1,17 @@
+#!/usr/bin/env bash
+# Copyright lowRISC contributors (OpenTitan project).
+# Licensed under the Apache License, Version 2.0, see LICENSE for details.
+# SPDX-License-Identifier: Apache-2.0
+
+case "$MODE" in
+ diff)
+ echo "$FILES" | xargs ${lint_tool} -s -d
+ exit $?
+ ;;
+ fix)
+ echo "$FILES" | xargs ${lint_tool} -s -w
+ ;;
+ *)
+ echo "Unknown mode: $MODE"
+ exit 2
+esac