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

CheckstyleAntTask: substitude setConfig(File) with setConfig(String) #4449

Closed
dimo414 opened this Issue Jun 13, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@dimo414
Contributor

dimo414 commented Jun 13, 2017

Ant's configUrl requires a full URL - relative paths don't work

Background in Issue #3119:

As I understand it, you should be able to specify a relative path (to a resource on the classpath) in the configUrl attribute of the Ant task. However I'm getting an error when I attempt to do so, and instead have to specify the full resource URL (jar:file:PATH_TO_JAR!/PATH_TO_RESOURCE), which is brittle and overspecified.


$ ant -version
Apache Ant(TM) version 1.10.1 compiled on February 2 2017

$ hg diff
diff -r 57a890e81233 build.xml
--- a/build.xml Sat Jun 10 15:06:52 2017 -0700
+++ b/build.xml Tue Jun 13 09:18:42 2017 -0700
@@ -124,7 +124,7 @@

     <!-- Checkstyle -->
     <target name="checkstyle" depends="build-test" description="Style linter">
-      <checkstyle configUrl="jar:file:lib/checkstyle-7.8.1-all.jar!/google_checks.xml" maxWarnings="0">
+      <checkstyle configUrl="/google_checks.xml" maxWarnings="0">
         <fileset dir="${core.src}" includes="**/*.java"/>
       </checkstyle>
       <checkstyle configUrl="jar:file:lib/checkstyle-7.8.1-all.jar!/google_checks.xml" maxWarnings="0">

$ ant checkstyle
Buildfile: /.../build.xml

init:

build:

build-test:

checkstyle:

BUILD FAILED
/.../build.xml:127: java.net.MalformedURLException: no protocol: /google_checks.xml
	at java.net.URL.<init>(URL.java:593)
	at java.net.URL.<init>(URL.java:490)
	at java.net.URL.<init>(URL.java:439)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at org.apache.tools.ant.IntrospectionHelper$11.set(IntrospectionHelper.java:1206)
	at org.apache.tools.ant.IntrospectionHelper$AttributeSetter.setObject(IntrospectionHelper.java:1512)
	at org.apache.tools.ant.IntrospectionHelper.setAttribute(IntrospectionHelper.java:412)
	at org.apache.tools.ant.RuntimeConfigurable.maybeConfigure(RuntimeConfigurable.java:528)
	at org.apache.tools.ant.RuntimeConfigurable.maybeConfigure(RuntimeConfigurable.java:464)
	at org.apache.tools.ant.Task.maybeConfigure(Task.java:202)
	at org.apache.tools.ant.UnknownElement.configure(UnknownElement.java:200)
	at org.apache.tools.ant.UnknownElement.maybeConfigure(UnknownElement.java:164)
	at org.apache.tools.ant.Task.perform(Task.java:347)
	at org.apache.tools.ant.Target.execute(Target.java:435)
	at org.apache.tools.ant.Target.performTasks(Target.java:456)
	at org.apache.tools.ant.Project.executeSortedTargets(Project.java:1405)
	at org.apache.tools.ant.Project.executeTarget(Project.java:1376)
	at org.apache.tools.ant.helper.DefaultExecutor.executeTargets(DefaultExecutor.java:41)
	at org.apache.tools.ant.Project.executeTargets(Project.java:1260)
	at org.apache.tools.ant.Main.runBuild(Main.java:857)
	at org.apache.tools.ant.Main.startAnt(Main.java:236)
	at org.apache.tools.ant.launch.Launcher.run(Launcher.java:287)
	at org.apache.tools.ant.launch.Launcher.main(Launcher.java:113)

Total time: 0 seconds

Checkstyle runs successfully without this local mod (i.e. with the fully-qualified URL). I see this same error on both OSX and Windows.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 15, 2017

Member

interesting that I do not see any "checkstyle" in stacktrace , so resolving to URL is done by ANT completely.
We do load of in sequence that I described at - 20506e1#diff-2be5da6521926564b0fd9efc2fc60390R124

As we use the same config file for multiple of usages (ant, cli, maven, eclipse, .......) ... we cannot change anything. Looks like nuance of ANT pre-processing of URL types (public void setConfigUrl(URL url) {(code)), so such syntax might be required.

ToDo if that is feature of ANT, we need to have a link to specification and probably(I am not sure now!!!) make new setter for config with String type, to let ANT do nothing and trigger checkstyle logic on priority of files load.

@dimo414 , if you have a time please help us and your self to resolve this issue.

Member

romani commented Jun 15, 2017

interesting that I do not see any "checkstyle" in stacktrace , so resolving to URL is done by ANT completely.
We do load of in sequence that I described at - 20506e1#diff-2be5da6521926564b0fd9efc2fc60390R124

As we use the same config file for multiple of usages (ant, cli, maven, eclipse, .......) ... we cannot change anything. Looks like nuance of ANT pre-processing of URL types (public void setConfigUrl(URL url) {(code)), so such syntax might be required.

ToDo if that is feature of ANT, we need to have a link to specification and probably(I am not sure now!!!) make new setter for config with String type, to let ANT do nothing and trigger checkstyle logic on priority of files load.

@dimo414 , if you have a time please help us and your self to resolve this issue.

@dimo414

This comment has been minimized.

Show comment
Hide comment
@dimo414

dimo414 Jun 16, 2017

Contributor

I don't think it's a nuance of Ant's preprocessing - Ant's just constructing a URL instance since that's the type the setConfigUrl() method expects. The exception is coming from java.net.URL's constructor, so there's nothing Ant can really do about it.

I agree swapping setConfigUrl() to take a String instead seems like the right fix, I'll try to verify that works and send a pull request if it does.

Contributor

dimo414 commented Jun 16, 2017

I don't think it's a nuance of Ant's preprocessing - Ant's just constructing a URL instance since that's the type the setConfigUrl() method expects. The exception is coming from java.net.URL's constructor, so there's nothing Ant can really do about it.

I agree swapping setConfigUrl() to take a String instead seems like the right fix, I'll try to verify that works and send a pull request if it does.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 19, 2017

Member

by ANT , I meant , its desire to make an URL object from string itself.

such code was introduced 15 years ago - https://github.com/checkstyle/checkstyle/blame/745e21f1e4d202dd8dba577585f35f7d3f10d839/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java#L222
A lot has changed from that time.

for all other plugins - config is string to resource.
Looks like ANT had special signature only due to the fact that it is lack our attention.

In Ant we have:
public void setConfig(File file)
public void setConfigUrl(URL url)

in other plugins we have only one method to set location of config - by String.
In Main.java

        // create a configuration
        final Configuration config = ConfigurationLoader.loadConfiguration(
                cliOptions.configLocation, new PropertiesExpander(props),
                !cliOptions.executeIgnoredModules);

ConfigurationLoader.java do load by String.

So we should convert URL to String, and introduce breaking compatibility by API , but it should not affect users who just use checkstyle by config.

Member

romani commented Jun 19, 2017

by ANT , I meant , its desire to make an URL object from string itself.

such code was introduced 15 years ago - https://github.com/checkstyle/checkstyle/blame/745e21f1e4d202dd8dba577585f35f7d3f10d839/src/main/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTask.java#L222
A lot has changed from that time.

for all other plugins - config is string to resource.
Looks like ANT had special signature only due to the fact that it is lack our attention.

In Ant we have:
public void setConfig(File file)
public void setConfigUrl(URL url)

in other plugins we have only one method to set location of config - by String.
In Main.java

        // create a configuration
        final Configuration config = ConfigurationLoader.loadConfiguration(
                cliOptions.configLocation, new PropertiesExpander(props),
                !cliOptions.executeIgnoredModules);

ConfigurationLoader.java do load by String.

So we should convert URL to String, and introduce breaking compatibility by API , but it should not affect users who just use checkstyle by config.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 21, 2017

Member

@rnveach , as we start breaking compatibilities, we could remove update ANT code to follow general way for config definiton - only by String
So currently we have:

public void setConfig(File file)
public void setConfigUrl(URL url)

we could make it accessible only by one method:

public void setConfig(String file)

are you agree with such change ?

Member

romani commented Jun 21, 2017

@rnveach , as we start breaking compatibilities, we could remove update ANT code to follow general way for config definiton - only by String
So currently we have:

public void setConfig(File file)
public void setConfigUrl(URL url)

we could make it accessible only by one method:

public void setConfig(String file)

are you agree with such change ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 21, 2017

Member

are you agree with such change ?

I agree with this change.

Member

rnveach commented Jun 21, 2017

are you agree with such change ?

I agree with this change.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 21, 2017

Member

@dimo414 , as you hit this ground , please create only public void setConfig(String file) method, all previous method should be removed.

Member

romani commented Jun 21, 2017

@dimo414 , as you hit this ground , please create only public void setConfig(String file) method, all previous method should be removed.

@romani romani closed this in #4463 Jun 25, 2017

romani added a commit that referenced this issue Jun 25, 2017

Issue #4449: Remove the Ant configUrl attribute and let config accept…
… arbitrary strings, to be consistent with other config logic

@romani romani added this to the 8.0 milestone Jun 25, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 25, 2017

Member

Fix is merged

Member

romani commented Jun 25, 2017

Fix is merged

@romani romani changed the title from Ant's configUrl requires a full URL - relative paths don't work to CheckstyleAntTask: substitude setConfig(File) with setConfig(String) Jun 27, 2017

stkent added a commit to btkelly/gnag that referenced this issue Jul 9, 2018

Bump to latest checkstyle version
Sources for changes:

http://checkstyle.sourceforge.net/releasenotes.html#Release_8.1

"Make SuppressionCommentFilter and SuppressWithNearbyCommentFilter
children of TreeWalker."

http://checkstyle.sourceforge.net/releasenotes.html#Release_8.0

"CheckstyleAntTask: substitude setConfig(File) with setConfig(String)."

(more context available here:
checkstyle/checkstyle#4449. The overloaded
methods were removed in favor of always accepting a string and
internally parsing whether that string represented a File, URL, etc., so
I took our File/URL inputs and converted to strings :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment