Bug 59276 - Update Checkstyle to version 6.17 needs a configuration change
Summary: Update Checkstyle to version 6.17 needs a configuration change
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 9.0.0.M4
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-06 00:25 UTC by Konstantin Kolinko
Modified: 2016-06-15 13:57 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Konstantin Kolinko 2016-04-06 00:25:41 UTC
Filing into Bugzilla to better document the issue.

As I noted in commit message for r1737833, simply dropping in Checkstyle 6.17 (6.16.1, 6.16) - any version later than 6.15 - results in notable drop in performance of reruns.

A more detailed observation is that running Checkstyle 6.17 does not create the cache files (output/res/checkstyle/*), so 6.17 is so slow because it is running without a cache.

In changelog for Checkstyle 6.16 [1] there is the following item:

* Move Treewalker cache to Checker. Author: Andrei Selkin #569 [2]

[1] http://checkstyle.sourceforge.net/releasenotes.html
[2] https://github.com/checkstyle/checkstyle/issues/569

[3] Commit that fixes issue 569: https://github.com/MEZk/checkstyle/commit/d46c2cf0e9df06bb5f424dbd7645574f082f7609


This means that "setCacheFile()" method was added to Checker class. It means that our configurations (res/checkstyle/*) need to be updated:

The fix is to move

  <property name="cacheFile" ...>

from within <module name="TreeWalker">
into top-level <module name="Checker"> element.


It is easy to do the change for command-line users of checkstyle. A question is whether the same configuration files are used by developers running Checkstyle plugin in IDEs. Moving the property will remove caching from older versions of Xhexkstyle.


A temporary backwards-compatible workaround may be to keep the property in both places (as setting the duplicate property on TreeWalker does not cause a runtime error with 6.17), but it is expected that the setter method will be removed from TreeWalker in some future release [4]

[4] https://github.com/checkstyle/checkstyle/issues/2883
#2883 - Remove `cache` field from TreeWalker in Checkstyle 8.0
Comment 1 Konstantin Kolinko 2016-04-06 00:44:26 UTC
(In reply to Konstantin Kolinko from comment #0)
> 
> A temporary backwards-compatible workaround may be to keep the property in
> both places (as setting the duplicate property on TreeWalker does not cause
> a runtime error with 6.17), but it is expected that the setter method will
> be removed from TreeWalker in some future release [4]
> 
> [4] https://github.com/checkstyle/checkstyle/issues/2883
> #2883 - Remove `cache` field from TreeWalker in Checkstyle 8.0

Such workaround is not possible. Checkstyle 6.15 does not have setCacheFile() method on Walker. An attempt to run updated configuration with 6.15 results in

BUILD FAILED
\build.xml:586: Unable to create a Checker: configLocation
 {<...>\trunk\res\checkstyle\javax-checkstyle.xml}, classpath {null}.

without any further details provided by the error message.

I changed the configuration in Tomcat 9 in r1737903.

Tomcat 8, 8.5 are TODO.

Tomcat 7 is using Checkstyle 6.1.1 as running 6.2 requires Java 7.
Comment 2 Mark Thomas 2016-06-15 13:57:07 UTC
Fixed for 8.5.x and 8.0.x as well.