groovy

Groovyc ignores includeAntRuntime when not forked.

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: None
  • Fix Version/s: 1.7.2, 1.8-beta-1
  • Component/s: Ant integration
  • Labels:
    None
  • Number of attachments :
    1

Description

I have an issue with the joint compiler in non-fork mode. The includeAntRuntime property is ignored and the antruntime is always included. This is different to the ant javac task. It is one if the strange things of ant that the antruntime is included by default. But not being able to turn it off is a problem. Basically this means you have to fork groovyc when you want to have a sane build.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Hans Dockter added a comment -

I was wrong. Groovyc is not ignoring the includeAntRuntime property in non fork mode. This issue can be closed. Sorry.

Show
Hans Dockter added a comment - I was wrong. Groovyc is not ignoring the includeAntRuntime property in non fork mode. This issue can be closed. Sorry.
Hide
Permalink
Hans Dockter added a comment -

I reopen the issue. I have observed the following behavior:

Ant Groovyc only takes includeAntRuntime=false into account if fork is set to true. Considering the fact, that the default for fork is false, this is a clear invitation to classloader hell. In particular, as Ant Javac considers includeAntRuntime=false in fork as well as non-fork mode and I guess this is what people would expect from groovyc as well. People using Groovy are also likely to use Groovyc via the AntBuilder from an application, which makes the issue even more critical, as the classpath then has usually more entries then just the ant libs.

The nested javac task of Groovyc takes includeAntRuntime into account. At least if I run my tests from compiled classes. If I run the script with the AntBuilder code via 'groovy bugtest.groovy' the includeAntRuntime has no effect on the nested javac task. For groovy-1.6-beta-1 I have observed, that the includeAntRuntime property can't be set any longer for the nested javac task.

Show
Hans Dockter added a comment - I reopen the issue. I have observed the following behavior: Ant Groovyc only takes includeAntRuntime=false into account if fork is set to true. Considering the fact, that the default for fork is false, this is a clear invitation to classloader hell. In particular, as Ant Javac considers includeAntRuntime=false in fork as well as non-fork mode and I guess this is what people would expect from groovyc as well. People using Groovy are also likely to use Groovyc via the AntBuilder from an application, which makes the issue even more critical, as the classpath then has usually more entries then just the ant libs. The nested javac task of Groovyc takes includeAntRuntime into account. At least if I run my tests from compiled classes. If I run the script with the AntBuilder code via 'groovy bugtest.groovy' the includeAntRuntime has no effect on the nested javac task. For groovy-1.6-beta-1 I have observed, that the includeAntRuntime property can't be set any longer for the nested javac task.
Hide
Permalink
Danno Ferrin added a comment -

What's really lacking here is a unit test or a self-contained example of how to trip it up so it can be created out of context of Gradle or GMaven or whatever else is tripping things up. I have a possible fix, but I am not sure if it would work and this would create a frustrating game of check-in ping-pong without some way to verify it myself.

Index: src/main/org/codehaus/groovy/ant/Groovyc.java
===================================================================
--- src/main/org/codehaus/groovy/ant/Groovyc.java	(revision 13862)
+++ src/main/org/codehaus/groovy/ant/Groovyc.java	Mon Nov 03 12:27:18 MST 2008
@@ -837,7 +837,9 @@
 
 
     protected GroovyClassLoader buildClassLoaderFor() {
-        ClassLoader parent = this.getClass().getClassLoader();
+        ClassLoader parent = getIncludeantruntime()
+            ? this.getClass().getClassLoader()
+            : new AntClassLoader(new GroovyClassLoader(null), getProject(), getClasspath());
         if (parent instanceof AntClassLoader) {
             AntClassLoader antLoader = (AntClassLoader) parent;
             String[] pathElm = antLoader.getClasspath().split(File.pathSeparator);

Patch is against 1.7-SNAPSHOT (svn head) could be tweaked to go against 1.6 branch and (with a bit mroe tweaking) 1.5.

Show
Danno Ferrin added a comment - What's really lacking here is a unit test or a self-contained example of how to trip it up so it can be created out of context of Gradle or GMaven or whatever else is tripping things up. I have a possible fix, but I am not sure if it would work and this would create a frustrating game of check-in ping-pong without some way to verify it myself. Index: src/main/org/codehaus/groovy/ant/Groovyc.java =================================================================== --- src/main/org/codehaus/groovy/ant/Groovyc.java (revision 13862) +++ src/main/org/codehaus/groovy/ant/Groovyc.java Mon Nov 03 12:27:18 MST 2008 @@ -837,7 +837,9 @@ protected GroovyClassLoader buildClassLoaderFor() { - ClassLoader parent = this .getClass().getClassLoader(); + ClassLoader parent = getIncludeantruntime() + ? this .getClass().getClassLoader() + : new AntClassLoader( new GroovyClassLoader( null ), getProject(), getClasspath()); if (parent instanceof AntClassLoader) { AntClassLoader antLoader = (AntClassLoader) parent; String [] pathElm = antLoader.getClasspath().split(File.pathSeparator); Patch is against 1.7-SNAPSHOT (svn head) could be tweaked to go against 1.6 branch and (with a bit mroe tweaking) 1.5.
Hide
Permalink
Hans Dockter added a comment -

Thanks for looking into this. I will try to attach a test case tomorrow or the day after.

Show
Hans Dockter added a comment - Thanks for looking into this. I will try to attach a test case tomorrow or the day after.
Hide
Permalink
Paul King added a comment -

Hans, any luck getting time to look at this proposed fix?

Show
Paul King added a comment - Hans, any luck getting time to look at this proposed fix?
Hide
Permalink
Hans Dockter added a comment -

My apologies for having forgotten about the test case. Here it is now. The zip contains a project. The project has a src dir with one groovy class. This class has a reference to a class from commons-math. The testscript uses the groovyc ant task, to compile the source class. No classpath elements have been assigned to the groovyc task.

If you execute the test script with groovy testscript.groovy, the compile fails, as it should be. There is no commons-math in the classpath.

If you execute the test script with groovy -cp lib/commons-math-1.1.jar testscript.groovy, the compile succeeds. This should not be the case, as we have set includeAntRuntime to false. If the isolation were working, you would need to assign the groovy and commons-math jar to the groovyc classpath to have a successful compile.

I had no time yet to give the above patch a try by myself.

Show
Hans Dockter added a comment - My apologies for having forgotten about the test case. Here it is now. The zip contains a project. The project has a src dir with one groovy class. This class has a reference to a class from commons-math. The testscript uses the groovyc ant task, to compile the source class. No classpath elements have been assigned to the groovyc task. If you execute the test script with groovy testscript.groovy , the compile fails, as it should be. There is no commons-math in the classpath. If you execute the test script with groovy -cp lib/commons-math-1.1.jar testscript.groovy , the compile succeeds. This should not be the case, as we have set includeAntRuntime to false. If the isolation were working, you would need to assign the groovy and commons-math jar to the groovyc classpath to have a successful compile. I had no time yet to give the above patch a try by myself.
Hide
Permalink
Paul King added a comment -

I have applied a modified version of Danno's suggested patch. It seems to work for me for the test cases I have devised. It would be great if someone else can try this out (grab trunk or a CI server output artifact).

Show
Paul King added a comment - I have applied a modified version of Danno's suggested patch. It seems to work for me for the test cases I have devised. It would be great if someone else can try this out (grab trunk or a CI server output artifact).

People

  • Assignee:
    Paul King
    Reporter:
    Hans Dockter
Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: