Plugin does not support XML entity includes

[TWiStErRob]

To reuse and make the size of the configuration file manageable it could be a good practice to split the checkstyle configuration into multiple XML files and DTD entity declarations to pull them in.

<!-- checkstyle.xml -->
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN" "http://www.puppycrawl.com/dtds/configuration_1_3.dtd" [
	<!ENTITY myBarModules SYSTEM "./bar.xml">
	]>

<module name="Checker">
	<module name="TreeWalker">
		<property name="tabWidth" value="4" />
		&myBarModules;
	</module>
</module>

<!-- bar.xml -->
<module name="whatever">
	...
</module>

A live example of this can be found in the Android support library, and the included files reside in the prebuilts/checkstyle repo.

Expected behavior: the path to the included file should be relative to the file that is including it. So ./bar.xml should be in the same folder as the checkstyle.xml, so does bar.xml (without any prefix). /bar.xml should link to the root folder on the drive where checkstyle.xml resides (at least on Windows, on Unix it should be the usual /).
So if P:\projects\blah\config\checkstyle.xml, then
<!ENTITY foo SYSTEM "./bar.xml"> → P:\projects\blah\config\bar.xml
<!ENTITY foo SYSTEM "./foo/bar.xml"> → P:\projects\blah\config\foo\bar.xml
<!ENTITY foo SYSTEM "bar.xml"> → P:\projects\blah\config\bar.xml
<!ENTITY foo SYSTEM "foo/bar.xml"> → P:\projects\blah\config\foo\bar.xml
<!ENTITY foo SYSTEM "/bar.xml"> → P:\bar.xml
<!ENTITY foo SYSTEM "/foo/bar.xml"> → P:\bar.xml

Actual behavior: The relative paths are resolved against the IDEA installation directory, for example:
<!ENTITY foo SYSTEM "./bar.xml"> → P:\tools\ide\idea\bin\bar.xml
<!ENTITY foo SYSTEM "./foo/bar.xml"> → P:\tools\ide\idea\bin\foo\bar.xml

Note: the same configuration XML works if ran through the command line (Gradle) checkstyle.

[tsjensen]

Isn't this a core Checkstyle feature request rather than one for the plugin?

[TWiStErRob]

It works from Gradle and when checkstyle is called directly so I'm guessing that it's the IDEA plugin that needs to set some parameters on the config parser, or checkstyle to set the cwd?

[jshiell]

Thanks for the suggestion. We do all sorts of hackery to make relative paths resolve correctly, so I imagine we'll just need to apply the same hackery here. I'll add it to the list.

It probably won't be this weekend, I fear, as I've just come off production support and don't want to see a computer for a couple of days 😄

[TWiStErRob]

No worries, I just opened this so it's known, take your time.

[jshiell]

Hmm. This is harder than it looks.

• We run in-process, so changing the global directory could potentially break other things (assuming, of course, the user.dir property change works in this case anyway).
• The Checkstyle parser is encapsulated - we can (and do) provide a property resolver, but there's no legit way to provide an entity resolver.

I see two options at present:

• Pre-parse the XML, and produce a mutated copy with absolute paths before passing this to Checkstyle.
• Go full evil wizard and hack it into Checkstyle via reflection.

The latter is, of course, madness. So we're really left with the pre-parse answer at present, which will require a little more time & thought (mostly to ensure I haven't missed anything obvious).

[TWiStErRob]

Thabks for the update, nice to see you being so responsive.

I wonder how it works in Gradle, that probably runs in-process as well, with cwd likely being rootProject.projectDir; so even that shouldn't work with paths being relative to checkstyle.xml.

Is it possible to use some $foo var? Not sure what kind of magic/path variables are supported by you or checkstyle itself.

You have to stay in-process, right? Otherwise it would be slow to do continuous checking on currently edited file.

[jshiell]

Some more investigation looks to be required - you're quite right, the Gradle plugin appears to run in-process. In fact, it appears to invoke the Ant plugin, which is part of the core Checkstyle distribution. I had a quick look there, but haven't spotted anything yet, so it'll take a bit more looking. It does seem a promising line of enquiry though, which I'll continue when I've a moment.

The $foo magic is unlikely - the property support is core Checkstyle, which we support via the PropertyResolver interface. I don't believe it supports them in this instance, as they're resolved post-parse.

In theory we could run a background task ala the Gradle daemon or similar, but it's not a simple solution. And I don't believe forking for each scan would be performant - it'd be fine for the solo scans, but I believe the inspection would not be great with this approach.

[jshiell]

Still at a bit of a loss on this one. I've extended our own EntityResolver to handle it, which means it can now read properties. But I still can't work out a robust way of injecting our resolver into Checkstyle's parse - it looks like we'd have to override ConfiguratonLoader.loadConfiguration and forcibly overwrite the resolver. This would be straightforward enough if we supported but one version of Checkstyle; however, we support a lot more, and there are changes in this class across this. We could try and ignore on failure ofc, but it seems more than a little dangerous.

[TWiStErRob]

How bad is that change over time? What's the link that needs to be done? (is it a method that sets a field via reflection, whose name changes between versions, or more complicated than that?)

[jshiell]

Potentially painful. saxHandler is assigned in the private constructor, but the helper methods both instantiate the class and immediately call it, given us no time to do the switch. Hence I imagine we'd need to write our own helper replacement, and do some introspection to allow us to call the private constructor.

It'll be fragile, but as long as it's covered by the test cases that run against each version of Checkstyle it's potentially workable.