diff --git a/README.md b/README.md index 16ac861e..0e884097 100644 --- a/README.md +++ b/README.md @@ -32,9 +32,19 @@ We have a separate project for formatting, see +- JavaScript + - `eslint`: https://eslint.org/ +- Python + - `flake8`: + +### Adding a linter + +Please follow the steps in [lint/README.md](./lint/README.md) and then send us a PR. +Thanks!! ## Installation diff --git a/docs/BUILD.bazel b/docs/BUILD.bazel index a61f6888..0b09c0bd 100644 --- a/docs/BUILD.bazel +++ b/docs/BUILD.bazel @@ -12,4 +12,9 @@ stardoc_with_diff_test( bzl_library_target = "//lint:eslint", ) +stardoc_with_diff_test( + name = "flake8", + bzl_library_target = "//lint:flake8", +) + update_docs(name = "update") diff --git a/docs/flake8.md b/docs/flake8.md new file mode 100644 index 00000000..63853c83 --- /dev/null +++ b/docs/flake8.md @@ -0,0 +1,74 @@ + + +API for declaring a flake8 lint aspect that visits py_library rules. + +Typical usage: + +``` +load("@aspect_rules_lint//lint:flake8.bzl", "flake8_aspect") + +flake8 = flake8_aspect( + binary = "@@//:flake8", + config = "@@//:.flake8", +) +``` + + + + +## flake8_action + +
+flake8_action(ctx, executable, srcs, config, report, use_exit_code)
+
+ +Run flake8 as an action under Bazel. + +Based on https://flake8.pycqa.org/en/latest/user/invocation.html + + +**PARAMETERS** + + +| Name | Description | Default Value | +| :------------- | :------------- | :------------- | +| ctx | Bazel Rule or Aspect evaluation context | none | +| executable | label of the the flake8 program | none | +| srcs | python files to be linted | none | +| config | label of the flake8 config file (setup.cfg, tox.ini, or .flake8) | none | +| report | output file to generate | none | +| use_exit_code | whether to fail the build when a lint violation is reported | False | + + + + +## flake8_aspect + +
+flake8_aspect(binary, config)
+
+ +A factory function to create a linter aspect. + +Attrs: + binary: a flake8 executable. Can be obtained from rules_python like so: + + ``` + load("@rules_python//python/entry_points:py_console_script_binary.bzl", "py_console_script_binary") + + py_console_script_binary( + name = "flake8", + pkg = "@pip//flake8:pkg", + ) + ``` + config: the flake8 config file (`setup.cfg`, `tox.ini`, or `.flake8`) + +**PARAMETERS** + + +| Name | Description | Default Value | +| :------------- | :------------- | :------------- | +| binary |

-

| none | +| config |

-

| none | + + diff --git a/example/.aspect/cli/config.yaml b/example/.aspect/cli/config.yaml index b0b2eef7..6d29594e 100644 --- a/example/.aspect/cli/config.yaml +++ b/example/.aspect/cli/config.yaml @@ -7,3 +7,4 @@ plugins: lint_aspects: - //:lint.bzl%eslint - //:lint.bzl%buf + - //:lint.bzl%flake8 diff --git a/example/.flake8 b/example/.flake8 new file mode 100644 index 00000000..98524564 --- /dev/null +++ b/example/.flake8 @@ -0,0 +1,5 @@ +[flake8] +extend-ignore = E203 +exclude = external,bazel-out +max-complexity = 10 +max-line-length = 100 diff --git a/example/BUILD.bazel b/example/BUILD.bazel index 9c2c4f50..272600c3 100644 --- a/example/BUILD.bazel +++ b/example/BUILD.bazel @@ -1,13 +1,25 @@ load("@aspect_rules_js//js:defs.bzl", "js_library") load("@npm//:defs.bzl", "npm_link_all_packages") load("@npm//:eslint/package_json.bzl", eslint_bin = "bin") -load("@bazel_skylib//rules:build_test.bzl", "build_test") +load("@rules_python//python:pip.bzl", "compile_pip_requirements") +load("@rules_python//python/entry_points:py_console_script_binary.bzl", "py_console_script_binary") package(default_visibility = ["//visibility:public"]) +compile_pip_requirements( + name = "requirements", +) + npm_link_all_packages(name = "node_modules") -exports_files(["buf.yaml"], visibility = ["//visibility:public"]) +exports_files(["buf.yaml", ".flake8"], visibility = ["//visibility:public"]) + +# We can test that it works with: +# bazel run :flake8 -- --help +py_console_script_binary( + name = "flake8", + pkg = "@pip//flake8:pkg", +) eslint_bin.eslint_binary(name = "eslint") @@ -19,9 +31,3 @@ js_library( ":node_modules/@typescript-eslint/parser", ], ) - -# Just so there's at least one test target in the example -build_test( - name = "test", - targets = [":eslint"], -) \ No newline at end of file diff --git a/example/MODULE.bazel b/example/MODULE.bazel index 19b1c3d3..64aa3b90 100644 --- a/example/MODULE.bazel +++ b/example/MODULE.bazel @@ -1,8 +1,9 @@ "Bazel dependencies" bazel_dep(name = "aspect_rules_lint", dev_dependency = True, version = "0.0.0") -bazel_dep(name = "aspect_bazel_lib", version = "1.31.2") +bazel_dep(name = "aspect_bazel_lib", version = "1.31.2", dev_dependency = True) bazel_dep(name = "aspect_rules_js", version = "1.32.2", dev_dependency = True) bazel_dep(name = "aspect_rules_ts", version = "1.3.3", dev_dependency = True) +bazel_dep(name = "rules_python", version = "0.26.0", dev_dependency = True) bazel_dep(name = "bazel_skylib", dev_dependency = True, version = "1.4.1") # Needed due to rules_proto leaking the dependency bazel_dep(name = "protobuf", version = "21.7", repo_name = "com_google_protobuf") @@ -35,3 +36,17 @@ npm.npm_translate_lock( ) use_repo(npm, "npm") + +python_version = "3.9" +python = use_extension("@rules_python//python/extensions:python.bzl", "python") +python.toolchain( + python_version = python_version, +) + +pip = use_extension("@rules_python//python/extensions:pip.bzl", "pip") +pip.parse( + python_version = python_version, + hub_name = "pip", + requirements_lock = "//:requirements.txt", +) +use_repo(pip, "pip") diff --git a/example/lint.bzl b/example/lint.bzl index 30741a45..e08ace73 100644 --- a/example/lint.bzl +++ b/example/lint.bzl @@ -2,6 +2,7 @@ load("@aspect_rules_lint//lint:eslint.bzl", "eslint_aspect") load("@aspect_rules_lint//lint:buf.bzl", "buf_lint_aspect") +load("@aspect_rules_lint//lint:flake8.bzl", "flake8_aspect") buf = buf_lint_aspect( config = "@@//:buf.yaml", @@ -11,3 +12,8 @@ eslint = eslint_aspect( binary = "@@//:eslint", config = "@@//:eslintrc", ) + +flake8 = flake8_aspect( + binary = "@@//:flake8", + config = "@@//:.flake8", +) diff --git a/example/lint.sh b/example/lint.sh index 7d8cd24a..12b2711b 100755 --- a/example/lint.sh +++ b/example/lint.sh @@ -13,7 +13,7 @@ if [ "$#" -eq 0 ]; then fi # Produce report files -bazel build --aspects //:lint.bzl%eslint,//:lint.bzl%buf --output_groups=report $@ +bazel build --aspects //:lint.bzl%eslint,//:lint.bzl%buf,//:lint.bzl%flake8 --output_groups=report $@ # Show the results. # `-mtime -1`: only look at files modified in the last day, to mitigate showing stale results of old bazel runs. diff --git a/example/requirements.in b/example/requirements.in new file mode 100644 index 00000000..39304807 --- /dev/null +++ b/example/requirements.in @@ -0,0 +1 @@ +flake8 diff --git a/example/requirements.txt b/example/requirements.txt new file mode 100644 index 00000000..629326b6 --- /dev/null +++ b/example/requirements.txt @@ -0,0 +1,22 @@ +# +# This file is autogenerated by pip-compile with Python 3.9 +# by the following command: +# +# bazel run //:requirements.update +# +flake8==6.1.0 \ + --hash=sha256:d5b3857f07c030bdb5bf41c7f53799571d75c4491748a3adcd47de929e34cd23 \ + --hash=sha256:ffdfce58ea94c6580c77888a86506937f9a1a227dfcd15f245d694ae20a6b6e5 + # via -r requirements.in +mccabe==0.7.0 \ + --hash=sha256:348e0240c33b60bbdf4e523192ef919f28cb2c3d7d5c7794f74009290f236325 \ + --hash=sha256:6c2d30ab6be0e4a46919781807b4f0d834ebdd6c6e3dca0bda5a15f863427b6e + # via flake8 +pycodestyle==2.11.0 \ + --hash=sha256:259bcc17857d8a8b3b4a2327324b79e5f020a13c16074670f9c8c8f872ea76d0 \ + --hash=sha256:5d1013ba8dc7895b548be5afb05740ca82454fd899971563d2ef625d090326f8 + # via flake8 +pyflakes==3.1.0 \ + --hash=sha256:4132f6d49cb4dae6819e5379898f2b8cce3c5f23994194c24b77d5da2e36f774 \ + --hash=sha256:a0aae034c444db0071aa077972ba4768d40c830d9539fd45bf4cd3f8f6992efc + # via flake8 diff --git a/example/src/BUILD.bazel b/example/src/BUILD.bazel index 5b924e74..627e4dd2 100644 --- a/example/src/BUILD.bazel +++ b/example/src/BUILD.bazel @@ -17,3 +17,8 @@ proto_library( deps = [":unused"], visibility = ["//visibility:public"], ) + +py_library( + name = "unused_import", + srcs = ["unused_import.py"], +) \ No newline at end of file diff --git a/example/src/unused_import.py b/example/src/unused_import.py new file mode 100644 index 00000000..9497b29c --- /dev/null +++ b/example/src/unused_import.py @@ -0,0 +1,6 @@ +# Demo with just running flake8: +# $ bazel run --run_under="cd $PWD &&" -- :flake8 --config=.flake8 --exit-zero src/*.py +# INFO: Build completed successfully, 1 total action +# src/unused_import.py:6:1: F401 'os' imported but unused +# Error: bazel exited with exit code: 1 +import os diff --git a/lint/BUILD.bazel b/lint/BUILD.bazel index 9f22810d..566c0af4 100644 --- a/lint/BUILD.bazel +++ b/lint/BUILD.bazel @@ -18,3 +18,9 @@ bzl_library( "@aspect_rules_js//js:libs", ], ) + +bzl_library( + name = "flake8", + srcs = ["flake8.bzl"], + visibility = ["//visibility:public"], +) diff --git a/lint/README.md b/lint/README.md new file mode 100644 index 00000000..771b0ea8 --- /dev/null +++ b/lint/README.md @@ -0,0 +1,80 @@ +# Adding a new linter + +## Design invariants + +These will be part of reviewing PRs to this repo: + +1. Avoid adding dependencies to rules_lint, they belong in the example instead. For example in adding + eslint or flake8, it's up to the user to provide us the binary to run. + This ensures that the user can select the versions of their tools and the toolchains used to run them + rather than us baking these into rules_lint. + +2. Study the installation, CLI usage, and configuration documentation for the linter you want to add. + We'll need to adapt these to Bazel's idioms. As much as possible, copy or link to this documentation + in your code so that maintainers can understand it. + Usage of the linter under Bazel should be as similar as possible to how it's used outside Bazel, + so we don't end up with a unique bug "surface area" that only Bazel users encounter. + +## Step 1: Run linter in the example + +In the example folder, install the tool the same way a user would. We need an executable target +(typically a `*_binary`) that can be passed to our aspect factory function. + +For example, to install flake8, we need to do the normal install for rules_python, +create a `requirements_lock.txt` file, call `pip_parse` to install it, and find the `entry_point` +function that gives us a `py_binary` for it. + +Add a config file for the linter following their documentation. We want users to be able to continue +running the linter outside Bazel (e.g. using their editor plugin) and they should be assured that +they get the same result. + +Add a file under `src/` that violates one of the linter checks, and add a comment describing how we +can see the linter running. This should basically be a "demo" of how the linter works, without +rules_lint getting involved yet. + +Take a look at this commit as an example of step 1: +https://github.com/aspect-build/rules_lint/commit/7365b82957dd60898ef1051f5ae94539714a38f4 + +Please check in a commit at this point to make your PR easier for us to review. + +## Step 2: create linter + +Add the new_linter.bzl file in this folder. + +Add these three things: + +1. A `my_linter_action` function which declares one or more actions with `ctx.actions.run` or `ctx.actions.run_shell`. + It should accept the report output as a parameter. + You can use the "demo" from step 1 to guide you in setting the command-line options for the linter CLI. + Note: this function provides a reusable API for developers who want to use flake8 in a different way than we prescribe with aspects. + +2. A `_my_linter_aspect_impl` function, following the https://bazel.build/extending/aspects#aspect_implementation API. + 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 `report` output group. + Currently we also rely on the report output file being named following the convention `*-report.txt`, though this is + a design smell. + +3. A `my_linter_aspect` factory function. This is a higher-order function that returns an aspect. + This pattern allows us to capture arguments like labels and toolchains which aren't legal + in public aspect attributes. + +Then wire this up into the example and confirm that you can get the same linter result as you did in +step 1. + +Take a look at this commit as an example of step 2: +https://github.com/aspect-build/rules_lint/commit/29d275bcf7ecf5b99c6bff6913322fe1909302eb + +## Step 3: docs + +Add a rule in the `docs/` folder matching the existing ones, so that the API docs are auto-generated. +Run `bazel run docs:update` to create the Markdown file. + +Also add your new linter to the README. + +Here's a commit showing what it should look like: +https://github.com/aspect-build/rules_lint/commit/c3bf01a39c2e68b0b37918620aeafbf8ef0b2d85 + +## Step 4: Send the PR! + +We'd love to make your linter available to everyone. diff --git a/lint/flake8.bzl b/lint/flake8.bzl new file mode 100644 index 00000000..fece0690 --- /dev/null +++ b/lint/flake8.bzl @@ -0,0 +1,93 @@ +"""API for declaring a flake8 lint aspect that visits py_library rules. + +Typical usage: + +``` +load("@aspect_rules_lint//lint:flake8.bzl", "flake8_aspect") + +flake8 = flake8_aspect( + binary = "@@//:flake8", + config = "@@//:.flake8", +) +``` +""" + +def flake8_action(ctx, executable, srcs, config, report, use_exit_code = False): + """Run flake8 as an action under Bazel. + + Based on https://flake8.pycqa.org/en/latest/user/invocation.html + + Args: + ctx: Bazel Rule or Aspect evaluation context + executable: label of the the flake8 program + srcs: python files to be linted + config: label of the flake8 config file (setup.cfg, tox.ini, or .flake8) + report: output file to generate + use_exit_code: whether to fail the build when a lint violation is reported + """ + inputs = srcs + [config] + outputs = [report] + + # Wire command-line options, see + # https://flake8.pycqa.org/en/latest/user/options.html + args = ctx.actions.args() + args.add_all(srcs) + args.add(report, format="--output-file=%s") + args.add(config, format="--config=%s") + if not use_exit_code: + args.add("--exit-zero") + + ctx.actions.run( + inputs = inputs, + outputs = outputs, + executable = executable, + arguments = [args], + mnemonic = "flake8", + ) + +# buildifier: disable=function-docstring +def _flake8_aspect_impl(target, ctx): + if ctx.rule.kind in ["py_library"]: + report = ctx.actions.declare_file(target.label.name + ".flake8-report.txt") + flake8_action(ctx, ctx.executable._flake8, ctx.rule.files.srcs, ctx.file._config_file, report) + results = depset([report]) + else: + results = depset() + + return [ + OutputGroupInfo(report = results), + ] + +def flake8_aspect(binary, config): + """A factory function to create a linter aspect. + + Attrs: + binary: a flake8 executable. Can be obtained from rules_python like so: + + ``` + load("@rules_python//python/entry_points:py_console_script_binary.bzl", "py_console_script_binary") + + py_console_script_binary( + name = "flake8", + pkg = "@pip//flake8:pkg", + ) + ``` + config: the flake8 config file (`setup.cfg`, `tox.ini`, or `.flake8`) + """ + return aspect( + implementation = _flake8_aspect_impl, + # Edges we need to walk up the graph from the selected targets. + # Needed for linters that need semantic information like transitive type declarations. + # attr_aspects = ["deps"], + attrs = { + "_flake8": attr.label( + default = binary, + executable = True, + cfg = "exec", + ), + "_config_file": attr.label( + default = config, + allow_single_file = True, + ), + }, + )