feat(lint): plumb exit codes as outputs rather than dropping them (#225)

* feat(lint): plumb exit codes as outputs rather than dropping on the floor This allows downstream tools to return a correct exit code based on 'errors were reported by at least one linter' * refactor: rewrite lint_test to be based on exit code when available * fix: shellcheck must declare another output When running it to produce patches we need to ignore its exit code. * refactor: more specific naming of stdout rather than report * chore: rename reports to collect outputs * chore: remove term 'diagnostics'
aspect-build · May 9, 2024 · 65b2e4d · 65b2e4d
1 parent 035953e
commit 65b2e4d
Show file tree

Hide file tree

Showing 25 changed files with 245 additions and 198 deletions.
diff --git a/docs/buf.md b/docs/buf.md
diff --git a/docs/eslint.md b/docs/eslint.md
diff --git a/docs/flake8.md b/docs/flake8.md
diff --git a/docs/ktlint.md b/docs/ktlint.md
diff --git a/docs/lint_test.md b/docs/lint_test.md
diff --git a/docs/pmd.md b/docs/pmd.md
diff --git a/docs/ruff.md b/docs/ruff.md
diff --git a/docs/shellcheck.md b/docs/shellcheck.md
diff --git a/docs/vale.md b/docs/vale.md
diff --git a/example/WORKSPACE.bazel b/example/WORKSPACE.bazel
@@ -256,8 +256,7 @@ fetch_pmd()
 
 load("@aspect_rules_lint//lint:ruff.bzl", "fetch_ruff")
 
-# https://github.com/astral-sh/ruff/pull/8631#issuecomment-2022746290
-fetch_ruff("v0.3.2")
+fetch_ruff()
 
 load("@aspect_rules_lint//lint:vale.bzl", "fetch_vale")
 

diff --git a/example/WORKSPACE.bzlmod b/example/WORKSPACE.bzlmod
@@ -17,8 +17,7 @@ fetch_ktfmt()
 
 fetch_swiftformat()
 
-# https://github.com/astral-sh/ruff/pull/8631#issuecomment-2022746290
-fetch_ruff("v0.3.2")
+fetch_ruff()
 
 load("@aspect_rules_lint//lint:pmd.bzl", "fetch_pmd")
 

diff --git a/example/src/unused_import.py b/example/src/unused_import.py
@@ -5,7 +5,7 @@
 # Error: bazel exited with exit code: 1
 
 # Demo with just running ruff:
-# $ bazel run --run_under="cd $PWD &&" -- :ruff --config=.ruff.toml --exit-zero src/*.py
+# $ bazel run --run_under="cd $PWD &&" -- //tools/lint:ruff check --config=.ruff.toml src/*.py
 # INFO: Build completed successfully, 1 total action
 # src/unused_import.py:12:8: F401 [*] `os` imported but unused
 # Found 1 error.

diff --git a/example/test/lint_test.bats b/example/test/lint_test.bats
@@ -41,6 +41,8 @@ EOF
 	assert_lints
 
 	run $BATS_TEST_DIRNAME/../lint.sh --fix --dry-run //src:all
+	assert_success
+
 	# Check that we created a 'patch -p1' format file that fixes the ESLint violation
 	run cat bazel-bin/src/ESLint.ts.aspect_rules_lint.patch
 	assert_success

diff --git a/lint/README.md b/lint/README.md
@@ -51,7 +51,7 @@ Add these three things:
    This is responsible for selecting which rule types in the graph it "knows how to lint".
    It should call the `my_linter_action` function.
    It must always return a `rules_lint_report` output group, which is easiest by using the
-   `report_file` helper in `//lint/private:lint_aspect.bzl`.
+   `report_files` helper in `//lint/private:lint_aspect.bzl`.
    The simple lint.sh also relies on the report output file being named following the convention
    `*.aspect_rules_lint.report`, though this is a design smell.
 

diff --git a/lint/buf.bzl b/lint/buf.bzl
@@ -12,23 +12,24 @@ buf = buf_lint_aspect(
 """
 
 load("@rules_proto//proto:defs.bzl", "ProtoInfo")
-load("//lint/private:lint_aspect.bzl", "LintOptionsInfo", "report_file")
+load("//lint/private:lint_aspect.bzl", "LintOptionsInfo", "report_files")
 
 _MNEMONIC = "buf"
 
 def _short_path(file, _):
     return file.path
 
-def buf_lint_action(ctx, buf, protoc, target, report, use_exit_code = False):
+def buf_lint_action(ctx, buf, protoc, target, stderr, exit_code = None):
     """Runs the buf lint tool as a Bazel action.
 
     Args:
         ctx: Rule OR Aspect context
         buf: the buf-lint executable
         protoc: the protoc executable
         target: the proto_library target to run on
-        report: output file to generate
-        use_exit_code: whether the protoc process exiting non-zero will be a build failure
+        stderr: output file containing the stderr of protoc
+        exit_code: output file to write the exit code.
+            If None, then fail the build when protoc exits non-zero.
     """
     config = json.encode({
         "input_config": "" if ctx.file._config == None else ctx.file._config.short_path,
@@ -57,22 +58,25 @@ def buf_lint_action(ctx, buf, protoc, target, report, use_exit_code = False):
     args.add_joined("--descriptor_set_in", deps, join_with = ":", map_each = _short_path)
     args.add_joined(["--buf-plugin_out", "."], join_with = "=")
     args.add_all(sources)
+    outputs = [stderr]
 
-    if use_exit_code:
-        command = "{protoc} $@ && touch {report}"
+    if exit_code:
+        command = "{protoc} $@ 2>{stderr}; echo $? > " + exit_code.path
+        outputs.append(exit_code)
     else:
-        command = "{protoc} $@ 2>{report} || true"
+        # Create empty file on success, as Bazel expects one
+        command = "{protoc} $@ && touch {stderr}"
 
     ctx.actions.run_shell(
         inputs = depset([
             ctx.file._config,
             protoc,
             buf,
         ], transitive = [deps]),
-        outputs = [report],
+        outputs = outputs,
         command = command.format(
             protoc = protoc.path,
-            report = report.path,
+            stderr = stderr.path,
         ),
         arguments = [args],
         mnemonic = _MNEMONIC,
@@ -82,14 +86,14 @@ def _buf_lint_aspect_impl(target, ctx):
     if ctx.rule.kind not in ["proto_library"]:
         return []
 
-    report, info = report_file(_MNEMONIC, target, ctx)
+    report, exit_code, info = report_files(_MNEMONIC, target, ctx)
     buf_lint_action(
         ctx,
         ctx.toolchains[ctx.attr._buf_toolchain].cli,
         ctx.toolchains["@rules_proto//proto:toolchain_type"].proto.proto_compiler.executable,
         target,
         report,
-        ctx.attr._options[LintOptionsInfo].fail_on_violation,
+        exit_code,
     )
     return [info]