Fix issues in ecj compiler's error reporting (#51)

* Fix passing compilerArgs arguments properly so that multi-dash options like --add-modules can be passed; report compiler errors that ecj mumbles about on stdout instead of properly reporting them in the compiler output log.

* Obey the "errorsAsWarnings" option for compiler command line errors too.

* Add JUnit test for -- arguments and showing errors that are on stdout

* Add license.

Author: fjalvingh

plexus-compilers/plexus-compiler-eclipse/src/main/java/org/codehaus/plexus/compiler/eclipse/EcjFailureException.java

@@ -0,0 +1,19 @@
+package org.codehaus.plexus.compiler.eclipse;
+
+/**
+ * @author <a href="mailto:[email protected]">Frits Jalvingh</a>
+ * Created on 22-4-18.
+ */
+public class EcjFailureException extends RuntimeException {
+    private final String ecjOutput;
+
+    public EcjFailureException(String ecjOutput) {
+        super("Failed to run the ecj compiler: " + ecjOutput);
+        this.ecjOutput = ecjOutput;
+    }
+
+    public String getEcjOutput()
+    {
+        return ecjOutput;
+    }
+}

plexus-compilers/plexus-compiler-eclipse/src/main/java/org/codehaus/plexus/compiler/eclipse/EclipseJavaCompiler.java

@@ -64,8 +64,10 @@ public EclipseJavaCompiler()
     public CompilerResult performCompile(CompilerConfiguration config )
         throws CompilerException
     {
-        // Build settings from configuration
         List<String> args = new ArrayList<>();
+        args.add("-noExit");                            // Make sure ecj does not System.exit on us 8-/
+
+        // Build settings from configuration
         if ( config.isDebug() )
         {
             args.add("-preserveAllLocals");
@@ -146,13 +148,44 @@ else if(extras.containsKey("-errorsAsWarnings"))
 
         for(Entry<String, String> entry : extras.entrySet())
         {
+            /*
+             * The compiler mojo makes quite a mess of passing arguments, depending on exactly WHICH
+             * way is used to pass them. The method method using <compilerArguments> uses the tag names
+             * of its contents to denote option names, and so the compiler mojo happily adds a '-' to
+             * all of the names there and adds them to the "custom compiler arguments" map as a
+             * name, value pair where the name always contains a single '-'. The Eclipse compiler (and
+             * javac too, btw) has options with two dashes (like --add-modules for java 9). These cannot
+             * be passed using a <compilerArguments> tag.
+             *
+             * The other method is to use <compilerArgs>, where each SINGLE argument needs to be passed
+             * using an <arg>xxxx</arg> tag. In there the xxx is not manipulated by the compiler mojo, so
+             * if it starts with a dash or more dashes these are perfectly preserved. But of course these
+             * single <arg> entries are not a pair. So the compiler mojo adds them as pairs of (xxxx, null).
+             *
+             * We use that knowledge here: if a pair has a null value then do not mess up the key but
+             * render it as a single value. This should ensure that something like:
+             * <compilerArgs>
+             *     <arg>--add-modules</arg>
+             *     <arg>java.se.ee</arg>
+             * </compilerArgs>
+             *
+             * is actually added to the command like as such.
+             *
+             * (btw: the above example will still give an error when using ecj <= 4.8M6:
+             *      invalid module name: java.se.ee
+             * but that seems to be a bug in ecj).
+             */
             String opt = entry.getKey();
-            if(! opt.startsWith("-"))
-                opt = "-" + opt;					// compiler mojo apparently messes with this; make sure we are safe
-            args.add(opt);
-            String value = entry.getValue();
-            if(null != value && ! value.isEmpty())
-                args.add(value);
+            String optionValue = entry.getValue();
+            if(null == optionValue) {
+                //-- We have an option from compilerArgs: use the key as-is as a single option value
+                args.add(opt);
+            } else {
+                if(!opt.startsWith("-"))
+                    opt = "-" + opt;
+                args.add(opt);
+                args.add(optionValue);
+            }
         }
 
         // Output path
@@ -209,65 +242,102 @@ else if(extras.containsKey("-errorsAsWarnings"))
 
         // Compile! Send all errors to xml temp file.
         File errorF = null;
-        try {
+        try
+        {
             errorF = File.createTempFile("ecjerr-", ".xml");
 
             args.add("-log");
             args.add(errorF.toString());
 
             // Add all sources.
             int argCount = args.size();
-            for(String source : config.getSourceLocations()) {
+            for(String source : config.getSourceLocations())
+            {
                 File srcFile = new File(source);
-                if(srcFile.exists()) {
-                    Set<String> ss = getSourceFilesForSourceRoot( config, source );
+                if(srcFile.exists())
+                {
+                    Set<String> ss = getSourceFilesForSourceRoot(config, source);
                     args.addAll(ss);
                 }
             }
             args.addAll(extraSourceDirs);
-            if(args.size() == argCount) {
+            if(args.size() == argCount)
+            {
                 //-- Nothing to do -> bail out
                 return new CompilerResult(true, Collections.EMPTY_LIST);
             }
 
+            getLogger().debug("ecj command line: " + args);
+
             StringWriter sw = new StringWriter();
             PrintWriter devNull = new PrintWriter(sw);
 
             //BatchCompiler.compile(args.toArray(new String[args.size()]), new PrintWriter(System.err), new PrintWriter(System.out), new CompilationProgress() {
-            BatchCompiler.compile(args.toArray(new String[args.size()]), devNull, devNull, new CompilationProgress() {
-                @Override public void begin(int i) {
+            boolean success = BatchCompiler.compile(args.toArray(new String[args.size()]), devNull, devNull, new CompilationProgress() {
+                @Override
+                public void begin(int i)
+                {
                 }
 
-                @Override public void done() {
+                @Override
+                public void done()
+                {
                 }
 
-                @Override public boolean isCanceled() {
+                @Override
+                public boolean isCanceled()
+                {
                     return false;
                 }
 
-                @Override public void setTaskName(String s) {
+                @Override
+                public void setTaskName(String s)
+                {
                 }
 
-                @Override public void worked(int i, int i1) {
+                @Override
+                public void worked(int i, int i1)
+                {
                 }
             });
             getLogger().debug(sw.toString());
 
             List<CompilerMessage> messageList;
             boolean hasError = false;
-            if(errorF.length() < 80) {
-                throw new IOException("Failed to run the ECJ compiler:\n" + sw.toString());
+            if(errorF.length() < 80)
+            {
+                throw new EcjFailureException(sw.toString());
             }
             messageList = new EcjResponseParser().parse(errorF, errorsAsWarnings);
 
-            for(CompilerMessage compilerMessage : messageList) {
-                if(compilerMessage.isError()) {
+            for(CompilerMessage compilerMessage : messageList)
+            {
+                if(compilerMessage.isError())
+                {
                     hasError = true;
                     break;
                 }
             }
-            return new CompilerResult(! hasError, messageList);
-
+            if(!hasError && !success && !errorsAsWarnings)
+            {
+                CompilerMessage.Kind kind = errorsAsWarnings ? CompilerMessage.Kind.WARNING : CompilerMessage.Kind.ERROR;
+
+                //-- Compiler reported failure but we do not seem to have one -> probable exception
+                CompilerMessage cm = new CompilerMessage("[ecj] The compiler reported an error but has not written it to its logging", kind);
+                messageList.add(cm);
+                hasError = true;
+
+                //-- Try to find the actual message by reporting the last 5 lines as a message
+                String stdout = getLastLines(sw.toString(), 5);
+                if(stdout.length() > 0)
+                {
+                    cm = new CompilerMessage("[ecj] The following line(s) might indicate the issue:\n" + stdout, kind);
+                    messageList.add(cm);
+                }
+            }
+            return new CompilerResult(!hasError || errorsAsWarnings, messageList);
+        } catch(EcjFailureException x) {
+            throw x;
         } catch(Exception x) {
             throw new RuntimeException(x);				// sigh
         } finally {
@@ -279,6 +349,36 @@ else if(extras.containsKey("-errorsAsWarnings"))
         }
     }
 
+    private String getLastLines(String text, int lines)
+    {
+        List<String> lineList = new ArrayList<>();
+        text = text.replace("\r\n", "\n");
+        text = text.replace("\r", "\n");            // make sure eoln is \n
+
+        int index = text.length();
+        while(index > 0) {
+            int before = text.lastIndexOf('\n', index - 1);
+
+            if(before + 1 < index) {                        // Non empty line?
+                lineList.add(text.substring(before + 1, index));
+                lines--;
+                if(lines <= 0)
+                    break;
+            }
+
+            index = before;
+        }
+
+        StringBuilder sb = new StringBuilder();
+        for(int i = lineList.size() - 1; i >= 0; i--)
+        {
+            String s = lineList.get(i);
+            sb.append(s);
+            sb.append(System.getProperty("line.separator"));        // 8-/
+        }
+        return sb.toString();
+    }
+
     static private void append(StringBuilder warns, String s) {
         if(warns.length() > 0)
             warns.append(',');

plexus-compilers/plexus-compiler-eclipse/src/test/java/org/codehaus/plexus/compiler/eclipse/EclipseCompilerDashedArgumentsTest.java

@@ -0,0 +1,100 @@
+package org.codehaus.plexus.compiler.eclipse;
+
+/**
+ * The MIT License
+ *
+ * Copyright (c) 2005, The Codehaus
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of
+ * this software and associated documentation files (the "Software"), to deal in
+ * the Software without restriction, including without limitation the rights to
+ * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies
+ * of the Software, and to permit persons to whom the Software is furnished to do
+ * so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+import org.codehaus.plexus.PlexusTestCase;
+import org.codehaus.plexus.compiler.Compiler;
+import org.codehaus.plexus.compiler.CompilerConfiguration;
+import org.codehaus.plexus.util.FileUtils;
+import org.junit.Assert;
+
+import java.io.File;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * @author <a href="mailto:[email protected]">Frits Jalvingh</a>
+ * Created on 22-4-18.
+ */
+public class EclipseCompilerDashedArgumentsTest extends PlexusTestCase {
+
+    public static final String BAD_DOUBLEDASH_OPTION = "--grubbelparkplace";
+
+    private CompilerConfiguration getConfig() throws Exception {
+        String sourceDir = getBasedir() + "/src/test-input/src/main";
+
+        List<String> filenames = FileUtils.getFileNames( new File( sourceDir ), "**/*.java", null, false, true );
+        Collections.sort( filenames );
+        Set<File> files = new HashSet<>();
+        for(String filename : filenames)
+        {
+            files.add(new File(filename));
+        }
+
+        CompilerConfiguration compilerConfig = new CompilerConfiguration();
+        compilerConfig.setDebug(false);
+        compilerConfig.setShowDeprecation(false);
+
+//            compilerConfig.setClasspathEntries( getClasspath() );
+        compilerConfig.addSourceLocation( sourceDir );
+        compilerConfig.setOutputLocation( getBasedir() + "/target/eclipse/classes");
+        FileUtils.deleteDirectory( compilerConfig.getOutputLocation() );
+//        compilerConfig.addInclude( filename );
+        compilerConfig.setForceJavacCompilerUse(false);
+        compilerConfig.setSourceFiles(files);
+
+        compilerConfig.setTargetVersion("1.8");
+        compilerConfig.setSourceVersion("1.8");
+        return compilerConfig;
+    }
+
+    /**
+     * Start the eclipse compiler with a bad option that has two dashes. It should abort, and the error
+     * message should show the actual bad option with two dashes. This ensures that both dashes are passed
+     * to the compiler proper.
+     *
+     * This also tests that con-compile errors are shown properly, as the error caused by
+     * the invalid option is not part of the error output but part of the data sent to stdout/stderr.
+     */
+    public void testDoubleDashOptionsArePassedWithTwoDashes() throws Exception
+    {
+        Compiler compiler = (Compiler) lookup( Compiler.ROLE, "eclipse" );
+        CompilerConfiguration config = getConfig();
+        config.addCompilerCustomArgument(BAD_DOUBLEDASH_OPTION, "b0rk3d");
+
+        try
+        {
+            compiler.performCompile(config);
+            Assert.fail("Expected an exception to be thrown");
+        } catch(EcjFailureException x) {
+            String ecjOutput = x.getEcjOutput();
+            Assert.assertTrue("The output should report the failure with two dashes: " + ecjOutput
+                    , ecjOutput.contains(BAD_DOUBLEDASH_OPTION) && ! ecjOutput.contains("-" + BAD_DOUBLEDASH_OPTION)
+            );
+        }
+    }
+}