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 2 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
2 changes: 1 addition & 1 deletion .bazelversion
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
7.1.1
7.4.1
# The first line of this file is used by Bazelisk and Bazel to be sure
# the right version of Bazel is used to build and test this repo.
# This also defines which version is used on CI.
Expand Down
3 changes: 2 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) {}
}
4 changes: 4 additions & 0 deletions example/tools/lint/linters.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ pmd_test = lint_test(aspect = pmd)
checkstyle = lint_checkstyle_aspect(
binary = "@@//tools/lint:checkstyle",
config = "@@//:checkstyle.xml",
configs = {
"@@//src": "@@//:checkstyle.xml",
"@@//src/subdir": "@@//:checkstyle_subdir.xml",
},
data = ["@@//:checkstyle-suppressions.xml"],
)

Expand Down
22 changes: 19 additions & 3 deletions lint/checkstyle.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,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 +83,15 @@ def _checkstyle_aspect_impl(target, ctx):
return []

files_to_lint = filter_srcs(ctx.rule)
config_file = ctx.file._config
if ctx.attr._configs:
label = str(target.label)
keys = sorted(ctx.attr._configs.keys(), key=len, reverse = True)
for key in keys:
if label.startswith(key):
config = ctx.attr._configs[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
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want longest matching prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the use case in the example, both //src/subdir and //src package have configuration files. I was thinking of matching the closest one.

Copy link
Member

Choose a reason for hiding this comment

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

I'd expect it to use the config from the nearest ancestor, matching most other tools

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Checkstyle tool seems to retrieve the configuration file from the specified parameter. In our setup, we typically organize all Checkstyle configuration files within the checkstyle directory. It might make more sense to pass the configuration as a map instead.

Copy link
Member

Choose a reason for hiding this comment

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

How about a prefactoring where you move your checkstyle configs closer to the folder they govern? Having them all in a single directory seems like it makes it hard to reason about, even without taking Bazel into account. If someone wanted to update the checkstyle config for their code, how would they figure out which one to edit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point but in our case, every folder shares the same checkstyle.xml file, but some have different import-control.xml files. Managing them in a folder like this seems easier.

outputs, info = output_files(_MNEMONIC, target, ctx)
if len(files_to_lint) == 0:
noop_lint_action(ctx, outputs)
Expand All @@ -91,7 +101,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 +111,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 +157,12 @@ def lint_checkstyle_aspect(binary, config, data = [], rule_kinds = ["java_binary
doc = "Config file",
default = config,
),
"_configs": attr.string_keyed_label_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
Loading