diff --git a/example/test/BUILD.bazel b/example/test/BUILD.bazel new file mode 100644 index 00000000..e69de29b diff --git a/example/test/machine_outputs/BUILD.bazel b/example/test/machine_outputs/BUILD.bazel new file mode 100644 index 00000000..e69de29b diff --git a/examples/rust/src/BUILD b/examples/rust/src/BUILD index 0d599655..e6f53f4c 100644 --- a/examples/rust/src/BUILD +++ b/examples/rust/src/BUILD @@ -76,3 +76,14 @@ rust_binary( ], deps = [":bad_lib_with_noclippy"], ) + +rust_binary( + name = "binary_with_warning_and_error", + srcs = ["warning_and_error.rs"], + edition = "2021", + visibility = [ + "//src/rust:__pkg__", + "//test:__pkg__", + "//test/machine_outputs:__pkg__", + ], +) diff --git a/examples/rust/src/bad_binary.rs b/examples/rust/src/bad_binary.rs index 4f84f7b2..07892179 100644 --- a/examples/rust/src/bad_binary.rs +++ b/examples/rust/src/bad_binary.rs @@ -1,5 +1,8 @@ fn main() { - // Will fail clippy because we could just write println!("Hello World"). - println!("{}", "Hello World"); + // Will fail clippy because of the missing comma + let _missing_comma = &[ + -1, -2, -3 // <= no comma here + -4, -5, -6 + ]; } diff --git a/examples/rust/src/bad_lib.rs b/examples/rust/src/bad_lib.rs index ee134a6a..ca81feda 100644 --- a/examples/rust/src/bad_lib.rs +++ b/examples/rust/src/bad_lib.rs @@ -1,5 +1,9 @@ fn bad() { - // Will fail clippy because we could just write println!("Hello World"). - println!("{}", "Hello World"); + // Will fail clippy because of the missing comma + let _missing_comma = &[ + -1, -2, -3 // <= no comma here + -4, -5, -6 + ]; + } diff --git a/examples/rust/src/warning_and_error.rs b/examples/rust/src/warning_and_error.rs new file mode 100644 index 00000000..d30fb81a --- /dev/null +++ b/examples/rust/src/warning_and_error.rs @@ -0,0 +1,11 @@ +fn main() { + // Will emit a clippy warning because we could just write println!("Hello World"). + println!("{}", "Hello World"); + + // Will fail clippy because of the missing comma + let _missing_comma = &[ + -1, -2, -3 // <= no comma here + -4, -5, -6 + ]; + +} \ No newline at end of file diff --git a/examples/rust/test/BUILD b/examples/rust/test/BUILD index 731dbc27..5be4f3d2 100644 --- a/examples/rust/test/BUILD +++ b/examples/rust/test/BUILD @@ -43,6 +43,14 @@ clippy_test( tags = ["manual"], ) +clippy_test( + name = "clippy_binary_with_warning_and_error", + srcs = ["//src:binary_with_warning_and_error"], + # Expected to fail based on current content of the file. + # Normally you'd fix the file instead of tagging this test. + tags = ["manual"], +) + machine_clippy_report( name = "machine_clippy_report", src = "//src:bad_lib", diff --git a/lint/clippy.bzl b/lint/clippy.bzl index d184d475..2cf73f9a 100644 --- a/lint/clippy.bzl +++ b/lint/clippy.bzl @@ -40,11 +40,6 @@ clippy = lint_clippy_aspect( Now your targets will be linted with clippy. If you wish a target to be excluded from linting, you can give them the `noclippy` tag. -Please note that, for now all clippy warnings are considered failures. -This is because rules_rust will fail the entire execution if there's even one error, -so we need to limit the reports to just warnings so that we can continue the target execution and generate useful output files. -Because we limit all errors to warnings, we must consider every warning as an error. - Please watch issue https://github.com/aspect-build/rules_lint/issues/385 for updates on this behavior. """ @@ -53,34 +48,26 @@ load("//lint/private:lint_aspect.bzl", "LintOptionsInfo", "OPTIONAL_SARIF_PARSER _MNEMONIC = "AspectRulesLintClippy" -def _marker_to_exit_code(ctx, marker, output, exit_code): - """Write 0 to exit_code if marker exists and the output is empty, fail otherwise. - - rules_rust won't write the exit code to the success_marker, so we assert that it exists and write the exit code ourselves. - If there is a success marker but the output is not empty, we mark it as a failure. - If there is no success marker, the action has failed anyway. - - Please note that all clippy warnings are considered failures. - - Args: - ctx (ctx): The rule or aspect context. Must have access to `ctx.actions.run_shell` - marker (File): A file that will only exist if the action has succeeded - exit_code (File): A file that will be written with the exit code 0 if marker exists - """ - if not exit_code: - # fail_on_violation is enabled, we don't have an exit_code file. - return +def _parse_wrapper_output_into_files(ctx, output_file, exit_code_file, raw_process_wrapper_wrapper_output): ctx.actions.run_shell( - outputs = [exit_code], - inputs = [marker, output], - arguments = [exit_code.path, output.path], command = """ - if [ -s $2 ]; then - echo '1' > $1 - else - echo '0' > $1 - fi - """, +exit_code=$(head -n 1 $1) +output=$(tail -n +2 $1) +echo "${output}" > $2 +echo "${exit_code}" > $3 +""", + arguments = [ + raw_process_wrapper_wrapper_output.path, + output_file.path, + exit_code_file.path, + ], + inputs = [ + raw_process_wrapper_wrapper_output, + ], + outputs = [ + output_file, + exit_code_file, + ], ) # buildifier: disable=function-docstring @@ -105,40 +92,48 @@ def _clippy_aspect_impl(target, ctx): noop_lint_action(ctx, outputs) return [info] - extra_options = [] + extra_options = [ + # If we don't pass any clippy options, rules_rust will (rightly) default to -Dwarnings, which turns all warnings into errors. + # They do this to force Bazel to re-run targets on failures. + # However, we don't need to do that because we keep track of output files and exit codes separately. + "-Wwarnings", + ] # FIXME: Implement support for --fix mode. Clippy has a --fix flag, but our patcher doesn't currently support running an action through a macro. # We have to either # (1) modify the patcher so that it can run an action through a macro, or # (2) modify rules_rust so that it gives us a struct with a command line we can run it with the patcher. - human_success_indicator = ctx.actions.declare_file(OUTFILE_FORMAT.format(label = target.label.name, mnemonic = _MNEMONIC, suffix = "human_success_indicator")) + raw_outputs = struct( + human = ctx.actions.declare_file(OUTFILE_FORMAT.format(label = target.label.name, mnemonic = _MNEMONIC, suffix = "raw_process_wrapper_wrapper_output_human")), + machine = ctx.actions.declare_file(OUTFILE_FORMAT.format(label = target.label.name, mnemonic = _MNEMONIC, suffix = "raw_process_wrapper_wrapper_output_machine")), + ) + rust_clippy_action.action( ctx, clippy_executable = clippy_bin, - process_wrapper = ctx.executable._process_wrapper, + process_wrapper = ctx.executable._process_wrapper_wrapper, crate_info = crate_info, config = ctx.file._config_file, - output = outputs.human.out, - success_marker = human_success_indicator, - cap_at_warnings = True, # We don't want to crash the process if there are clippy errors, we just want to report them. + output = raw_outputs.human, + cap_at_warnings = False, extra_clippy_flags = extra_options, ) - _marker_to_exit_code(ctx, human_success_indicator, outputs.human.out, outputs.human.exit_code) - machine_success_indicator = ctx.actions.declare_file(OUTFILE_FORMAT.format(label = target.label.name, mnemonic = _MNEMONIC, suffix = "machine_success_indicator")) + _parse_wrapper_output_into_files(ctx, outputs.human.out, outputs.human.exit_code, raw_outputs.human) + rust_clippy_action.action( ctx, clippy_executable = clippy_bin, - process_wrapper = ctx.executable._process_wrapper, + process_wrapper = ctx.executable._process_wrapper_wrapper, crate_info = crate_info, config = ctx.file._config_file, - output = outputs.machine.out, - success_marker = machine_success_indicator, - cap_at_warnings = True, + output = raw_outputs.machine, + cap_at_warnings = False, extra_clippy_flags = extra_options, error_format = "json", ) - _marker_to_exit_code(ctx, machine_success_indicator, outputs.machine.out, outputs.machine.exit_code) + + _parse_wrapper_output_into_files(ctx, outputs.machine.out, outputs.machine.exit_code, raw_outputs.machine) # FIXME: Rustc only gives us JSON output, which we can't turn into SARIF yet. # clippy uses rustc's IO format, which doesn't have a SARIF output mode built in, @@ -175,9 +170,9 @@ def lint_clippy_aspect(config, rule_kinds = DEFAULT_RULE_KINDS): "_rule_kinds": attr.string_list( default = rule_kinds, ), - "_process_wrapper": attr.label( - doc = "A process wrapper for running clippy on all platforms", - default = Label("@rules_rust//util/process_wrapper"), + "_process_wrapper_wrapper": attr.label( + doc = "A wrapper around the rules_rust process wrapper. See @aspect_rules_lint//lint/rust:process_wrapper_wrapper.sh for motivation and documetnation.", + default = Label("//lint/rust:process_wrapper_wrapper"), executable = True, cfg = "exec", ), diff --git a/lint/rust/BUILD.bazel b/lint/rust/BUILD.bazel index dce59186..89e2f1b2 100644 --- a/lint/rust/BUILD.bazel +++ b/lint/rust/BUILD.bazel @@ -1,4 +1,5 @@ load("@aspect_rules_js//js:defs.bzl", "js_binary", "js_library", "js_run_binary", "js_test") +load("@rules_shell//shell:sh_binary.bzl", "sh_binary") js_library( name = "diagnostic-formatter", @@ -38,3 +39,14 @@ js_test( "GOT": "$(rootpath :got.patch)", }, ) + +sh_binary( + name = "process_wrapper_wrapper", + srcs = ["process_wrapper_wrapper.bash"], + data = [ + "@rules_rust//util/process_wrapper", + ], + # Aspect visibility rules are not on by default. + visibility = ["//visibility:public"], + deps = ["@rules_shell//shell/runfiles"], +) diff --git a/lint/rust/process_wrapper_wrapper.bash b/lint/rust/process_wrapper_wrapper.bash new file mode 100755 index 00000000..883d1bf5 --- /dev/null +++ b/lint/rust/process_wrapper_wrapper.bash @@ -0,0 +1,64 @@ +#!/bin/bash +set -euo pipefail + +# Run the rustc process wrapper, but: +# - Instead of forwarding the exit code of the child process, capture it. +# - Instead of passing --stderr-file to the process wrapper, capture the output and write to that file after everything has finished, with the following format: +# ``` +# +# +# ``` + +# --- begin runfiles.bash initialization v3 --- +# Copy-pasted from the Bazel Bash runfiles library v3. +set -uo pipefail; set +e; f=bazel_tools/tools/bash/runfiles/runfiles.bash +# shellcheck disable=SC1090 +source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \ + source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \ + source "$0.runfiles/$f" 2>/dev/null || \ + source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \ + source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \ + { echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e +# --- end runfiles.bash initialization v3 --- + +process_wrapper_path=$(rlocation "rules_rust/util/process_wrapper/process_wrapper") + +stderr_file="" +cmd=("${process_wrapper_path}") + +# Parse arguments +while [[ $# -gt 0 ]]; do + case "$1" in + --stderr-file) + if [[ $# -lt 2 ]]; then + echo "Error: --stderr-file requires a value" >&2 + exit 1 + fi + stderr_file="$2" + shift 2 + ;; + *) + cmd+=("$1") + shift + ;; + esac +done + +# Ensure that _some_ command is left +if [[ ${#cmd[@]} -eq 0 ]]; then + echo "Error: no command to execute" >&2 + exit 1 +fi + +set +e # We allow failing for this command as we want to capture the exit code + +out=$(${cmd[@]} 2>&1) +exit_code="$?" + +set -e + +cat < "${stderr_file}" +${exit_code} +${out} +EOF +