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

No way of providing suppressions.xml to checkstyle_config #3

Closed
thirtyseven opened this issue Dec 28, 2021 · 4 comments · Fixed by #9
Closed

No way of providing suppressions.xml to checkstyle_config #3

thirtyseven opened this issue Dec 28, 2021 · 4 comments · Fixed by #9

Comments

@thirtyseven
Copy link
Contributor

thirtyseven commented Dec 28, 2021

Common usage of checkstyle involves defining errors to suppress in a separate file and including it like so:

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

However, such files will not be visible to any checkstyle_test targets due to sandboxing, resulting in an error like the following:

com.puppycrawl.tools.checkstyle.api.CheckstyleException: cannot initialize module SuppressionFilter - Unable to find: suppressions.xml
	at com.puppycrawl.tools.checkstyle.Checker.setupChild(Checker.java:473)
	at com.puppycrawl.tools.checkstyle.api.AutomaticBean.configure(AutomaticBean.java:201)
	at com.puppycrawl.tools.checkstyle.Main.runCheckstyle(Main.java:405)
	at com.puppycrawl.tools.checkstyle.Main.runCli(Main.java:332)
	at com.puppycrawl.tools.checkstyle.Main.execute(Main.java:191)
	at com.puppycrawl.tools.checkstyle.Main.main(Main.java:126)
Caused by: com.puppycrawl.tools.checkstyle.api.CheckstyleException: Unable to find: suppressions.xml
	at com.puppycrawl.tools.checkstyle.utils.CommonUtil.getResourceFromClassPath(CommonUtil.java:457)
	at com.puppycrawl.tools.checkstyle.utils.CommonUtil.getFilepathOrClasspathUri(CommonUtil.java:434)
	at com.puppycrawl.tools.checkstyle.utils.CommonUtil.getUriByFilename(CommonUtil.java:385)
	at com.puppycrawl.tools.checkstyle.filters.SuppressionsLoader.loadSuppressions(SuppressionsLoader.java:222)
	at com.puppycrawl.tools.checkstyle.filters.SuppressionFilter.finishLocalSetup(SuppressionFilter.java:274)
	at com.puppycrawl.tools.checkstyle.api.AutomaticBean.configure(AutomaticBean.java:197)
	at com.puppycrawl.tools.checkstyle.Checker.setupChild(Checker.java:468)
	... 5 more
Checkstyle ends with 1 errors.

Worse, since checkstyle_cli does not check the exit code of checkstyle, and the string ERROR is not printed on stdout in the message above, any checkstyle_test that defines a suppression filter will pass without actually being linted.

Two possible changes to make here, let me know if a PR implementing them would be welcome:

  1. Add a data parameter to checkstyle_config allowing extra files to be included in runfiles
  2. Update the checkstyle_cli script to bubble up return code of checkstyle
@thirtyseven thirtyseven changed the title No way of providing suppressions.xml to checkstyle test No way of providing suppressions.xml to checkstyle_config Dec 28, 2021
@shs96c
Copy link
Collaborator

shs96c commented Dec 29, 2021

Being able to include arbitrary files using the data attribute makes sense, and I can't believe that we're not handling the return code properly 🤦!

A PR would be most welcome :) Thank you!

@shs96c
Copy link
Collaborator

shs96c commented Jan 4, 2022

@thirtyseven I'm back in the office and able to help if you'd like any support or guidance with the PR.

@thirtyseven
Copy link
Contributor Author

Thanks @shs96c might have some cycles for it later this week

@shs96c
Copy link
Collaborator

shs96c commented Jan 5, 2022

That's great news :) I'm on the public Bazel slack channel and work UK hours if you need help or just want to chat about this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants