Improve logging and exception behavior of Perforce tasks

PR: 18154
Submitted by: Matt Bishop (Matt at thebishops dot org)


git-svn-id: https://svn.apache.org/repos/asf/ant/core/trunk@274995 13f79535-47bb-0310-9956-ffa450edef68

WHATSNEW

@@ -133,6 +133,9 @@ Fixed bugs:
 * Perforce tasks relying on output from the server such as <p4change>
   and <p4label> were hanging. Bugzilla Reports 18129 and 18956.
 
+* Improve exception and logging behavior of Perforce tasks.
+  Bugzilla report 18154.
+
 * build.sh install had a problem on cygwin (with REALANTHOME).
   Bugzilla Report 17257
 

src/main/org/apache/tools/ant/taskdefs/optional/perforce/P4Base.java

@@ -77,6 +77,7 @@ import org.apache.tools.ant.types.Commandline;
  * @see P4Label
  * @see org.apache.tools.ant.taskdefs.Execute
  * @author <A HREF="mailto:leslie.hughes@rubus.com">Les Hughes</A>
+ * @author <a href="mailto:matt@thebishops.org">Matt Bishop</a>
  */
 public abstract class P4Base extends org.apache.tools.ant.Task {
 
@@ -107,6 +108,45 @@ public abstract class P4Base extends org.apache.tools.ant.Task {
      * Forms half of low level API */
     protected String P4CmdOpts = "";
 
+    /** Set by the task or a handler to indicate that the task has failed.  BuildExceptions
+     * can also be thrown to indicate failure. */
+    private boolean inError = false;
+
+    /** If inError is set, then errorMessage needs to contain the reason why. */
+    private String errorMessage = "";
+    /**
+     * gets whether or not the task has encountered an error
+     * @return error flag
+     * @since ant 1.6
+     */
+    public boolean getInError() {
+        return inError;
+    }
+
+    /**
+     * sets the error flag on the task
+     * @param inError if true an error has been encountered by the handler
+     * @since ant 1.6
+     */
+    public void setInError(boolean inError) {
+        this.inError = inError;
+    }
+
+    /**
+     * gets the error message recorded by the Perforce handler
+     * @return error message
+     */
+    public String getErrorMessage() {
+        return errorMessage;
+    }
+
+    /**
+     * sets the error message
+     * @param errorMessage line of error output
+     */
+    public void setErrorMessage(String errorMessage) {
+        this.errorMessage = errorMessage;
+    }
     //Setters called by Ant
 
     /**
@@ -234,7 +274,9 @@ public abstract class P4Base extends org.apache.tools.ant.Task {
      */
     protected void execP4Command(String command, P4Handler handler) throws BuildException {
         try {
-
+            // reset error flags before executing the command
+            inError = false;
+            errorMessage = "";
             Commandline commandline = new Commandline();
             commandline.setExecutable("p4");
 
@@ -267,6 +309,10 @@ public abstract class P4Base extends org.apache.tools.ant.Task {
 
             try {
                 exe.execute();
+
+                if (inError && failOnError) {
+                    throw new BuildException(errorMessage);
+                }
             } catch (IOException e) {
                 throw new BuildException(e);
             } finally {

src/main/org/apache/tools/ant/taskdefs/optional/perforce/SimpleP4OutputHandler.java

@@ -61,11 +61,13 @@ package org.apache.tools.ant.taskdefs.optional.perforce;
 
 import org.apache.tools.ant.BuildException;
 import org.apache.tools.ant.Project;
+import org.apache.tools.ant.util.StringUtils;
 
 /**
  * simple implementation of P4HandlerAdapter used by tasks which are not
  * actually processing the output from Perforce
  * @author <A HREF="mailto:leslie.hughes@rubus.com">Les Hughes</A>
+ * @author <a href="mailto:matt@thebishops.org">Matt Bishop</a>
  */
 public class SimpleP4OutputHandler extends P4HandlerAdapter {
 
@@ -81,8 +83,10 @@ public class SimpleP4OutputHandler extends P4HandlerAdapter {
 
     /**
      * process one line of stderr/stdout
+     * if error conditions are detected, then setters are called on the
+     * parent
      * @param line line of output
-     * @throws BuildException if errror output is received
+     * @throws BuildException does not throw exceptions any more
      */
     public void process(String line) throws BuildException {
         if (parent.util.match("/^exit/", line)) {
@@ -90,25 +94,39 @@ public class SimpleP4OutputHandler extends P4HandlerAdapter {
         }
 
         //Throw exception on errors (except up-to-date)
-        //p4 -s is unpredicatable. For example a server down
-        //does not return error: markup
         //
+        //When a server is down, the code expects :
+        //Perforce client error:
+        //Connect to server failed; check $P4PORT.
+        //TCP connect to localhost:1666 failed.
+        //connect: localhost:1666: Connection refused
         //Some forms producing commands (p4 -s change -o) do tag the output
         //others don't.....
         //Others mark errors as info, for example edit a file
         //which is already open for edit.....
         //Just look for error: - catches most things....
-        //when running labelsync, if view elements are in sync, Perforce produces a line of output
-        //looking like this one :
-        //error: //depot/file2 - label in sync.
-
-        if (parent.util.match("/error:/", line) && !parent.util.match("/up-to-date/", line)
-            && !parent.util.match("/label in sync/", line)) {
-            throw new BuildException(line);
 
+        if (parent.util.match("/^error:/", line)
+            || parent.util.match("/^Perforce client error:/", line)) {
+            //when running labelsync, if view elements are in sync,
+            //Perforce produces a line of output
+            //looking like this one :
+            //error: //depot/file2 - label in sync.
+            if (!parent.util.match("/label in sync/", line)
+                && !parent.util.match("/up-to-date/", line)) {
+                parent.setInError(true);
+            } else {
+                //sync says "error:" when a file is up-to-date
+                line = parent.util.substitute("s/^[^:]*: //", line);
+            }
+        } else if (parent.util.match("/^info.*?:/", line)) {
+            //sometimes there's "info1:
+            line = parent.util.substitute("s/^[^:]*: //", line);
         }
+        parent.log(line, parent.getInError() ? Project.MSG_ERR : Project.MSG_INFO);
 
-        parent.log(parent.util.substitute("s/^[^:]*: //", line), Project.MSG_INFO);
-
+        if (parent.getInError()) {
+            parent.setErrorMessage(parent.getErrorMessage() + line + StringUtils.LINE_SEP);
+        }
     }
 }