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.

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

Support multiple configuration files for different modules in checkstyle #464

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 22 additions & 1 deletion docs/checkstyle.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions example/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ exports_files(
".flake8",
"pmd.xml",
"checkstyle.xml",
"checkstyle_subdir.xml",
"checkstyle-suppressions.xml",
".ruff.toml",
".shellcheckrc",
Expand Down
19 changes: 19 additions & 0 deletions example/checkstyle_subdir.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
"-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
"https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">

<module name="TreeWalker">
<module name="UnusedImports"/>
</module>

<module name="LineLength">
<property name="max" value="84"/>
</module>

<module name="SuppressionFilter">
<property name="file" value="checkstyle-suppressions.xml"/>
</module>

</module>
5 changes: 5 additions & 0 deletions example/src/subdir/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,8 @@ ts_project(
"//:node_modules/moment",
],
)

java_library(
name = "bar",
srcs = ["Bar.java"],
)
10 changes: 10 additions & 0 deletions example/src/subdir/Bar.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package src.subdir;

// Unused imports are suppressed in suppressions.xml, so this should not raise issue.
import java.util.Objects;
import java.io.BufferedInputStream;

public class Bar {
// Max line length set to 20, so this should raise issue.
protected void finalize(int a) {}
}
10 changes: 7 additions & 3 deletions example/tools/lint/linters.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,13 @@ pmd = lint_pmd_aspect(
pmd_test = lint_test(aspect = pmd)

checkstyle = lint_checkstyle_aspect(
binary = Label("@//tools/lint:checkstyle"),
config = Label("@//:checkstyle.xml"),
data = [Label("@//:checkstyle-suppressions.xml")],
binary = "@@//tools/lint:checkstyle",
config = "@@//:checkstyle.xml",
configs = {
"@@//:checkstyle.xml": "@@//src,@@//test",
Copy link
Member

Choose a reason for hiding this comment

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

I'm taking another look at what apple_rules_lint did to permit users to configure linters per-directory, i.e. https://github.com/apple/apple_rules_lint/blob/main/api.md#get_lint_config allows you to set tags and https://github.com/apple/apple_rules_lint/blob/main/lint/private/package_lint_config.bzl#L14 allows the linter configuration to be selected in the same package being visited.

I'm curious, you must have considered just using apple_rules_lint instead of Aspect, since this checkstyle case is the example on their main README and I assume it motivated their design for selecting configs. Did you come to understand any tradeoffs between these approaches?

"@@//:checkstyle_subdir.xml": "@@//src/subdir",
},
data = ["@@//:checkstyle-suppressions.xml"],
)

checkstyle_test = lint_test(aspect = checkstyle)
Expand Down
31 changes: 28 additions & 3 deletions lint/checkstyle.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ load("//lint/private:lint_aspect.bzl", "LintOptionsInfo", "filter_srcs", "noop_l

_MNEMONIC = "AspectRulesLintCheckstyle"

def rebuild_map(configs):
directory_to_config = {}
for config, directories in configs.items():
for directory in directories.split(","):
directory_to_config[directory] = config
return directory_to_config

def checkstyle_action(ctx, executable, srcs, config, data, stdout, exit_code = None, options = []):
"""Run Checkstyle as an action under Bazel.

Expand All @@ -42,6 +49,7 @@ def checkstyle_action(ctx, executable, srcs, config, data, stdout, exit_code = N
executable: label of the the Checkstyle program
srcs: java files to be linted
config: label of the checkstyle.xml file
configs: dictionary mapping package label to their respective checkstyle.xml file
data: labels of additional xml files such as suppressions.xml
stdout: output file to generate
exit_code: output file to write the exit code.
Expand Down Expand Up @@ -82,6 +90,17 @@ def _checkstyle_aspect_impl(target, ctx):
return []

files_to_lint = filter_srcs(ctx.rule)
config_file = ctx.file._config
config_maps = rebuild_map(ctx.attr._configs)
if ctx.attr._configs:
label = str(target.label)
keys = sorted(config_maps.keys(), key=len, reverse = True)
for key in keys:
if label.startswith(key):
config = config_maps[key]
config_file = config.files.to_list()[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

does this support allow_single_file so that it's just config.file ?
Maybe not according to bazelbuild/bazel#5355 (comment) but worth asking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem to have allow_single_file attribute.

break

outputs, info = output_files(_MNEMONIC, target, ctx)
if len(files_to_lint) == 0:
noop_lint_action(ctx, outputs)
Expand All @@ -91,7 +110,7 @@ def _checkstyle_aspect_impl(target, ctx):
ctx,
ctx.executable._checkstyle,
files_to_lint,
ctx.file._config,
config_file,
ctx.files._data,
outputs.human.out,
outputs.human.exit_code,
Expand All @@ -101,15 +120,15 @@ def _checkstyle_aspect_impl(target, ctx):
ctx,
ctx.executable._checkstyle,
files_to_lint,
ctx.file._config,
config_file,
ctx.files._data,
outputs.machine.out,
outputs.machine.exit_code,
["-f", "sarif"],
)
return [info]

def lint_checkstyle_aspect(binary, config, data = [], rule_kinds = ["java_binary", "java_library"]):
def lint_checkstyle_aspect(binary, config, configs = {}, data = [], rule_kinds = ["java_binary", "java_library"]):
"""A factory function to create a linter aspect.

Attrs:
Expand Down Expand Up @@ -147,6 +166,12 @@ def lint_checkstyle_aspect(binary, config, data = [], rule_kinds = ["java_binary
doc = "Config file",
default = config,
),
"_configs": attr.label_keyed_string_dict(
allow_empty = True,
allow_files = True,
doc = "Package and configuration files dictionary",
default = configs,
),
"_data": attr.label_list(
doc = "Additional files to make available to Checkstyle such as any included XML files",
allow_files = True,
Expand Down