fixing fork bugs:

- permitting fork in incremental mode only when using tag file
- pulling aspectjtools also from the task classpath
also failonerror default was not true as stated in the docs

2 files changed, 128 insertions, 68 deletions

diff --git a/taskdefs/src/org/aspectj/tools/ant/taskdefs/AjcTask.java b/taskdefs/src/org/aspectj/tools/ant/taskdefs/AjcTask.java
index f2b57fab3..68a03cc98 100644
--- a/taskdefs/src/org/aspectj/tools/ant/taskdefs/AjcTask.java
+++ b/taskdefs/src/org/aspectj/tools/ant/taskdefs/AjcTask.java
@@ -15,43 +15,16 @@
 
 package org.aspectj.tools.ant.taskdefs;
 
-import org.apache.tools.ant.BuildException;
-import org.apache.tools.ant.Location;
-import org.apache.tools.ant.Project;
-import org.apache.tools.ant.taskdefs.Copy;
-import org.apache.tools.ant.taskdefs.Execute;
-import org.apache.tools.ant.taskdefs.Expand;
-import org.apache.tools.ant.taskdefs.Javac;
-import org.apache.tools.ant.taskdefs.LogStreamHandler;
-import org.apache.tools.ant.taskdefs.MatchingTask;
-import org.apache.tools.ant.taskdefs.Zip;
-import org.apache.tools.ant.types.Commandline;
-import org.apache.tools.ant.types.CommandlineJava;
-import org.apache.tools.ant.types.FileSet;
-import org.apache.tools.ant.types.Path;
-import org.apache.tools.ant.types.PatternSet;
-import org.apache.tools.ant.types.Reference;
-import org.apache.tools.ant.types.ZipFileSet;
+import java.io.*;
+import java.util.*;
+
+import org.apache.tools.ant.*;
+import org.apache.tools.ant.taskdefs.*;
+import org.apache.tools.ant.types.*;
 import org.aspectj.bridge.*;
-import org.aspectj.bridge.IMessage;
-import org.aspectj.bridge.IMessageHandler;
-import org.aspectj.bridge.IMessageHolder;
-import org.aspectj.bridge.MessageHandler;
 import org.aspectj.tools.ajc.Main;
 import org.aspectj.tools.ajc.Main.MessagePrinter;
-import org.aspectj.util.FileUtil;
-import org.aspectj.util.LangUtil;
-
-import java.io.File;
-import java.io.FileWriter;
-import java.io.IOException;
-import java.io.PrintWriter;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.Iterator;
-import java.util.List;
-import java.util.StringTokenizer;
+import org.aspectj.util.*;
 
 
 /**
@@ -182,43 +155,66 @@ public class AjcTask extends MatchingTask {
     }
     
    /**
-     * Find aspectjtools.jar in the system classpath.
+     * Find aspectjtools.jar on the task or system classpath.
      * Accept <code>aspectj{-}tools{...}.jar</code>
      * mainly to support build systems using maven-style 
      * re-naming 
      * (e.g., <code>aspectj-tools-1.1.0.jar</code>.
+     * Note that we search the task classpath first, 
+     * though an entry on the system classpath would be loaded first,
+     * because it seems more correct as the more specific one.
      * @return readable File for aspectjtools.jar, or null if not found.
      */            
     public static File findAspectjtoolsJar() {
-        final Path classpath = Path.systemClasspath;
-        final String[] paths = classpath.list();
+        File result = null;
+        ClassLoader loader = AjcTask.class.getClassLoader();
+        if (loader instanceof AntClassLoader) {
+            AntClassLoader taskLoader = (AntClassLoader) loader;
+            String cp = taskLoader.getClasspath();
+            String[] cps = LangUtil.splitClasspath(cp);
+            for (int i = 0; (i < cps.length) && (null == result); i++) {
+                result = isAspectjtoolsjar(cps[i]);
+            }
+        }
+        if (null == result) {
+            final Path classpath = Path.systemClasspath;
+            final String[] paths = classpath.list();
+            for (int i = 0; (i < paths.length) && (null == result); i++) {
+                result = isAspectjtoolsjar(paths[i]);
+            }
+        }
+        return (null == result? null : result.getAbsoluteFile());
+    }
+    
+    /** @return File if readable jar with aspectj tools name, or null */
+    private static File isAspectjtoolsjar(String path) {
+        if (null == path) {
+            return null;
+        }
         final String prefix = "aspectj";        
         final String infix = "tools";        
         final String altInfix = "-tools";        
         final String suffix = ".jar";
-        final int prefixLength = prefix.length();
-        final int minLength = prefixLength + infix.length() 
-                            + suffix.length();        
-        for (int i = 0; i < paths.length; i++) {
-            String path = paths[i];
-            if (!paths[i].endsWith(suffix)) {
-                continue;
+        final int prefixLength = 7; // prefix.length();
+        final int minLength = 16;
+        // prefixLength + infix.length() + suffix.length();        
+        if (!path.endsWith(suffix)) {
+            return null;
+        }
+        int loc = path.lastIndexOf(prefix);
+        if ((-1 != loc) && ((loc + minLength) <= path.length())) {
+            String rest = path.substring(loc+prefixLength);
+            if (-1 != rest.indexOf(File.pathSeparator)) {
+                return null;
             }
-            int loc = path.lastIndexOf(prefix);
-            if ((-1 != loc) && ((loc + minLength) <= path.length())) {
-                String rest = path.substring(loc+prefixLength);
-                if (-1 != rest.indexOf(File.pathSeparator)) {
-                    continue;
-                }
-                if (rest.startsWith(infix)
-                    || rest.startsWith(altInfix)) {
-                    File result = new File(path);
-                    if (result.canRead() && result.isFile()) {
-                        return result;
-                    }
+            if (rest.startsWith(infix)
+                || rest.startsWith(altInfix)) {
+                File result = new File(path);
+                if (result.canRead() && result.isFile()) {
+                    return result;
                 }
             }
-        }
+        }        
         return null;
     }
 
@@ -278,7 +274,6 @@ public class AjcTask extends MatchingTask {
 	
 	// ------- single entries dumped into cmd
     protected GuardedCommand cmd;
-    private int cmdLength;
 	
 	// ------- lists resolved in addListArgs() at execute() time
     private Path srcdir;
@@ -352,13 +347,12 @@ public class AjcTask extends MatchingTask {
         bootclasspath = null;
         classpath = null;
         cmd = new GuardedCommand();
-        cmdLength = 0;
         copyInjars = false;
         destDir = DEFAULT_DESTDIR;
         executing = false;
         executingInOtherVM = false;
         extdirs = null;
-        failonerror = false;
+        failonerror = true;  // non-standard default
         forkclasspath = null;
         inIncrementalMode = false;
         inIncrementalFileMode = false;
@@ -417,7 +411,7 @@ public class AjcTask extends MatchingTask {
     
     public void setIncremental(boolean incremental) {  
         cmd.addFlag("-incremental", incremental);
-        inIncrementalMode = true;
+        inIncrementalMode = incremental;
     }
 
     public void setHelp(boolean help) {  
@@ -859,6 +853,7 @@ public class AjcTask extends MatchingTask {
         } else {
             executing = true;
         }
+        verifyOptions();
         try {
             String[] args = makeCommand();
             if (verbose || listFileArgs) { // XXX if listFileArgs, only do that
@@ -937,6 +932,17 @@ public class AjcTask extends MatchingTask {
     }   
 
     /**
+     * @throw BuildException if options conflict
+     */
+    protected void verifyOptions() {
+        if (fork && isInIncrementalMode() && !isInIncrementalFileMode()) {
+            String m = "can fork incremental only using tag file";
+            throw new BuildException(m);
+        }
+
+    }
+
+    /**
      * Run the compile in the same VM by
      * loading the compiler (Main), 
      * setting up any message holders,
@@ -1060,13 +1066,29 @@ public class AjcTask extends MatchingTask {
         javaCmd.setClassname(org.aspectj.tools.ajc.Main.class.getName());
         
         final Path vmClasspath = javaCmd.createClasspath(getProject());
-        if ((null != forkclasspath) 
-            && (0 != forkclasspath.size())) {
-            vmClasspath.addExisting(forkclasspath);
-        } else {
-            File aspectjtools = findAspectjtoolsJar();
-            if (null != aspectjtools) {
-                vmClasspath.createPathElement().setLocation(aspectjtools);
+        {
+            File aspectjtools = null;
+            int vmClasspathSize = vmClasspath.size();
+            if ((null != forkclasspath) 
+                && (0 != forkclasspath.size())) {
+                vmClasspath.addExisting(forkclasspath);
+            } else {                
+                aspectjtools = findAspectjtoolsJar();
+                if (null != aspectjtools) {
+                    vmClasspath.createPathElement().setLocation(aspectjtools);
+                }
+            }
+            int newVmClasspathSize = vmClasspath.size();
+            if (vmClasspathSize == newVmClasspathSize) {
+                String m = "unable to find aspectjtools to fork - ";
+                if (null != aspectjtools) {
+                    m += "tried " + aspectjtools.toString();
+                } else if (null != forkclasspath) {
+                    m += "tried " + forkclasspath.toString();
+                } else {
+                    m += "define forkclasspath or put aspectjtools on classpath";
+                }
+                throw new BuildException(m);
             }
         }
         if (null != maxMem) {
diff --git a/taskdefs/testsrc/org/aspectj/tools/ant/taskdefs/AjcTaskTest.java b/taskdefs/testsrc/org/aspectj/tools/ant/taskdefs/AjcTaskTest.java
index 89acd8d8f..8d96abd67 100644
--- a/taskdefs/testsrc/org/aspectj/tools/ant/taskdefs/AjcTaskTest.java
+++ b/taskdefs/testsrc/org/aspectj/tools/ant/taskdefs/AjcTaskTest.java
@@ -176,6 +176,9 @@ public class AjcTaskTest extends TestCase {
 
     public void testFindAspectjtoolsJar() {
         File toolsJar = AjcTask.findAspectjtoolsJar();
+        if (null != toolsJar) {
+            assertNull("tools jar found?: " + toolsJar, toolsJar);
+        }
         // not found when unit testing b/c not on system classpath
         // so just checking for exceptions.
         // XXX need aspect to stub out System.getProperty(..) 
@@ -213,6 +216,35 @@ public class AjcTaskTest extends TestCase {
         return task;
     }
     
+    public void testDefaultListForkedNoTools() {
+        AjcTask task = getTask("default.lst");
+        task.setFork(true);
+        boolean passed = false;
+        try {
+            runTest(task, BuildException.class, MessageHolderChecker.NONE);
+            passed = true;
+        } finally {
+            if (!passed) {
+                String m = "AjcTaskTest.testDefaultListForkedNoTools()"
+                    + " fails if aspectjtools.jar is on the classpath";
+                System.err.println(m);
+            }
+        }
+    }
+
+    public void testDefaultListForkedIncremental() {
+        AjcTask task = getTask("default.lst");
+        task.setFork(true);
+        task.setIncremental(true);
+        runTest(task, BuildException.class, MessageHolderChecker.NONE);
+    }
+
+    /** failonerror should default to true, unlike other booleans */
+    public void testCompileErrorFailOnErrorDefault() {
+        AjcTask task = getTask("compileError.lst");
+        runTest(task, BuildException.class, MessageHolderChecker.ONE_ERROR);
+    }
+    
     public void testDefaultList() {
         AjcTask task = getTask("default.lst");
         runTest(task, NO_EXCEPTION, MessageHolderChecker.INFOS);
@@ -220,6 +252,7 @@ public class AjcTaskTest extends TestCase {
 
     public void testCompileErrorList() {
         AjcTask task = getTask("compileError.lst");
+        task.setFailonerror(false);
         runTest(task, NO_EXCEPTION, MessageHolderChecker.ONE_ERROR);
     }
 
@@ -230,6 +263,7 @@ public class AjcTaskTest extends TestCase {
 
     public void testNoSuchFileList() {
         AjcTask task = getTask("NoSuchFile.lst");
+        task.setFailonerror(false);
         runTest(task, NO_EXCEPTION, MessageHolderChecker.ONE_ERROR_ONE_ABORT);
     }
 
@@ -260,6 +294,7 @@ public class AjcTaskTest extends TestCase {
 
     public void testNoFile() {
         AjcTask task = getTask(NOFILE);
+        task.setFailonerror(false);
         runTest(task, NO_EXCEPTION, MessageHolderChecker.ONE_ERROR_ONE_ABORT);
     }
     
@@ -268,16 +303,19 @@ public class AjcTaskTest extends TestCase {
 
     public void testCompileErrorFile() {
         AjcTask task = getTask("compileError.lst");
+        task.setFailonerror(false);
         runTest(task, NO_EXCEPTION, MessageHolderChecker.ONE_ERROR);
     }
 
     public void testCompileWarningFile() {
         AjcTask task = getTask("compileWarning.lst");
+        task.setFailonerror(false);
         runTest(task, NO_EXCEPTION, MessageHolderChecker.ONE_WARNING);
     }
 
     public void testNoSuchFile() {
         AjcTask task = getTask("NoSuchFile.lst");
+        task.setFailonerror(false);
         runTest(task, NO_EXCEPTION, MessageHolderChecker.ONE_ERROR_ONE_ABORT);
     }