Applied Benoit Belley's patch in ticket 1957 improve the robustness of

GetBuildFailures().  New function convert_to_buildError, and use it in
several places so all build failures now go through it and are
returned as BuildError exceptions.  Had a small effect on output
formatting in many tests but no significant change to behavior.  I
reworked the patch a little to keep SCons exit status values the same
as before; this patch could make it simpler to change them in some
cases, e.g. exit with the errno of the failed action if desired.  One
nice side effect of this patch is that more scons errors print the
node that caused the error now.

9 files changed, 255 insertions, 86 deletions

diff --git a/src/engine/SCons/Action.py b/src/engine/SCons/Action.py
index c24d0be..cfd4a50 100644
--- a/src/engine/SCons/Action.py
+++ b/src/engine/SCons/Action.py
@@ -946,31 +946,38 @@ class FunctionAction(_ActionAction):
         return "%s(target, source, env)" % name
 
     def execute(self, target, source, env):
-        rsources = map(rfile, source)
+        exc_info = (None,None,None)
         try:
-            result = self.execfunction(target=target, source=rsources, env=env)
-        except EnvironmentError, e:
-            # If an IOError/OSError happens, raise a BuildError.
-            # Report the name of the file or directory that caused the
-            # error, which might be different from the target being built
-            # (for example, failure to create the directory in which the
-            # target file will appear).
-            try: filename = e.filename
-            except AttributeError: filename = None
-            result = SCons.Errors.BuildError(node=target,
-                                             errstr=e.strerror,
-                                             status=1,
-                                             filename=filename,
-                                             action=self,
-                                             command=self.strfunction(target, source, env))
-        else:
+            rsources = map(rfile, source)
+            try:
+                result = self.execfunction(target=target, source=rsources, env=env)
+            except Exception, e:
+                result = e
+                exc_info = sys.exc_info()
+
             if result:
-                msg = "Error %s" % result
-                result = SCons.Errors.BuildError(errstr=msg,
-                                                 status=result,
-                                                 action=self,
-                                                 command=self.strfunction(target, source, env))
-        return result
+                result = SCons.Errors.convert_to_BuildError(result, exc_info)
+                result.node=target
+                result.action=self
+                result.command=self.strfunction(target, source, env)
+
+                # FIXME: This maintains backward compatibility with respect to
+                # which type of exceptions were returned by raising an
+                # exception and which ones were returned by value. It would
+                # probably be best to always return them by value here, but
+                # some codes do not check the return value of Actions and I do
+                # not have the time to modify them at this point.
+                if (exc_info[1] and 
+                    not isinstance(exc_info[1],EnvironmentError)):
+                    raise result
+
+            return result
+        finally:
+            # Break the cycle between the traceback object and this
+            # function stack frame. See the sys.exc_info() doc info for
+            # more information about this issue.
+            del exc_info
+
 
     def get_presig(self, target, source, env):
         """Return the signature contents of this callable action."""
diff --git a/src/engine/SCons/CacheDirTests.py b/src/engine/SCons/CacheDirTests.py
index e31f67b..3715bf1 100644
--- a/src/engine/SCons/CacheDirTests.py
+++ b/src/engine/SCons/CacheDirTests.py
@@ -241,7 +241,8 @@ class FileTestCase(BaseTestCase):
             warn_caught = 0
             try:
                 f7.built()
-            except SCons.Warnings.CacheWriteErrorWarning:
+            except SCons.Errors.BuildError, e:
+                assert e.exc_info[0] == SCons.Warnings.CacheWriteErrorWarning
                 warn_caught = 1
             assert warn_caught
         finally:
diff --git a/src/engine/SCons/Errors.py b/src/engine/SCons/Errors.py
index fc55cf4..e663fb4 100644
--- a/src/engine/SCons/Errors.py
+++ b/src/engine/SCons/Errors.py
@@ -30,20 +30,88 @@ and user errors in SCons.
 
 __revision__ = "__FILE__ __REVISION__ __DATE__ __DEVELOPER__"
 
+import SCons.Util
 
+import exceptions
 
 class BuildError(Exception):
-    def __init__(self, node=None, errstr="Unknown error", status=0,
-                       filename=None, executor=None, action=None, command=None,
-                       *args):
-        self.node = node
+    """ Errors occuring while building.
+
+    BuildError have the following attributes:
+
+        Information about the cause of the build error:
+        -----------------------------------------------
+
+        errstr : a description of the error message
+
+        status : the return code of the action that caused the build
+                 error. Must be set to a non-zero value even if the
+                 build error is not due to an action returning a
+                 non-zero returned code.
+
+        exitstatus : SCons exit status due to this build error.
+                     Must be nonzero unless due to an explicit Exit()
+                     call.  Not always the same as status, since
+                     actions return a status code that should be
+                     respected, but SCons typically exits with 2
+                     irrespective of the return value of the failed
+                     action.
+
+        filename : The name of the file or directory that caused the
+                   build error. Set to None if no files are associated with
+                   this error. This might be different from the target
+                   being built. For example, failure to create the
+                   directory in which the target file will appear. It
+                   can be None if the error is not due to a particular
+                   filename.
+
+        exc_info : Info about exception that caused the build
+                   error. Set to (None, None, None) if this build
+                   error is not due to an exception.
+
+
+        Information about the cause of the location of the error:
+        ---------------------------------------------------------
+
+        node : the error occured while building this target node(s)
+        
+        executor : the executor that caused the build to fail (might
+                   be None if the build failures is not due to the
+                   executor failing)
+        
+        action : the action that caused the build to fail (might be
+                 None if the build failures is not due to the an
+                 action failure)
+
+        command : the command line for the action that caused the
+                  build to fail (might be None if the build failures
+                  is not due to the an action failure)
+        """
+
+    def __init__(self, 
+                 node=None, errstr="Unknown error", status=2, exitstatus=2,
+                 filename=None, executor=None, action=None, command=None,
+                 exc_info=(None, None, None)):
+        
         self.errstr = errstr
         self.status = status
+        self.exitstatus = exitstatus
         self.filename = filename
+        self.exc_info = exc_info
+
+        self.node = node
         self.executor = executor
         self.action = action
         self.command = command
-        apply(Exception.__init__, (self,) + args)
+
+        Exception.__init__(self, node, errstr, status, exitstatus, filename, 
+                           executor, action, command, exc_info)
+
+    def __str__(self):
+        if self.filename:
+            return self.filename + ': ' + self.errstr
+        else:
+            return self.errstr
 
 class InternalError(Exception):
     pass
@@ -61,11 +129,68 @@ class ExplicitExit(Exception):
     def __init__(self, node=None, status=None, *args):
         self.node = node
         self.status = status
+        self.exitstatus = status
         apply(Exception.__init__, (self,) + args)
 
-class TaskmasterException(Exception):
-    def __init__(self, node=None, exc_info=(None, None, None), *args):
-        self.node = node
-        self.errstr = "Exception"
-        self.exc_info = exc_info
-        apply(Exception.__init__, (self,) + args)
+def convert_to_BuildError(status, exc_info=None):
+    """
+    Convert any return code a BuildError Exception.
+
+    `status' can either be a return code or an Exception.
+    The buildError.status we set here will normally be
+    used as the exit status of the "scons" process.
+    """
+    if not exc_info and isinstance(status, Exception):
+        exc_info = (status.__class__, status, None)
+
+    if isinstance(status, BuildError):
+        buildError = status
+        buildError.exitstatus = 2   # always exit with 2 on build errors
+    elif isinstance(status, ExplicitExit):
+        status = status.status
+        errstr = 'Explicit exit, status %s' % status
+        buildError = BuildError(
+            errstr=errstr,
+            status=status,      # might be 0, OK here
+            exitstatus=status,      # might be 0, OK here
+            exc_info=exc_info)
+    elif isinstance(status, (StopError, UserError)):
+        buildError = BuildError(
+            errstr=str(status),
+            status=2,
+            exitstatus=2,
+            exc_info=exc_info)
+    elif isinstance(status, exceptions.EnvironmentError):
+        # If an IOError/OSError happens, raise a BuildError.
+        # Report the name of the file or directory that caused the
+        # error, which might be different from the target being built
+        # (for example, failure to create the directory in which the
+        # target file will appear).
+        try: filename = status.filename
+        except AttributeError: filename = None
+        buildError = BuildError( 
+            errstr=status.strerror,
+            status=status.errno,
+            exitstatus=2,
+            filename=filename,
+            exc_info=exc_info)
+    elif isinstance(status, Exception):
+        buildError = BuildError(
+            errstr='%s : %s' % (status.__class__.__name__, status),
+            status=2,
+            exitstatus=2,
+            exc_info=exc_info)
+    elif SCons.Util.is_String(status):
+        buildError = BuildError(
+            errstr=status,
+            status=2,
+            exitstatus=2)
+    else:
+        buildError = BuildError(
+            errstr="Error %s" % status,
+            status=status,
+            exitstatus=2)
+    
+    #import sys
+    #sys.stderr.write("convert_to_BuildError: status %s => (errstr %s, status %s)"%(status,buildError.errstr, buildError.status))
+    return buildError
diff --git a/src/engine/SCons/ErrorsTests.py b/src/engine/SCons/ErrorsTests.py
index 93d516c..893d284 100644
--- a/src/engine/SCons/ErrorsTests.py
+++ b/src/engine/SCons/ErrorsTests.py
@@ -32,10 +32,49 @@ class ErrorsTestCase(unittest.TestCase):
     def test_BuildError(self):
         """Test the BuildError exception."""
         try:
-            raise SCons.Errors.BuildError(node = "n", errstr = "foo")
+            raise SCons.Errors.BuildError(
+                errstr = "foo", status=57, filename="file", exc_info=(1,2,3),
+                node = "n", executor="e", action="a", command="c")
         except SCons.Errors.BuildError, e:
-            assert e.node == "n"
             assert e.errstr == "foo"
+            assert e.status == 57
+            assert e.exitstatus == 2, e.exitstatus
+            assert e.filename == "file"
+            assert e.exc_info == (1,2,3)
+
+            assert e.node == "n"
+            assert e.executor == "e"
+            assert e.action == "a"
+            assert e.command == "c"
+
+        try:
+            raise SCons.Errors.BuildError("n", "foo", 57, 3, "file", 
+                                          "e", "a", "c", (1,2,3))
+        except SCons.Errors.BuildError, e:
+            assert e.errstr == "foo", e.errstr
+            assert e.status == 57, e.status
+            assert e.exitstatus == 3, e.exitstatus
+            assert e.filename == "file", e.filename
+            assert e.exc_info == (1,2,3), e.exc_info
+
+            assert e.node == "n"
+            assert e.executor == "e"
+            assert e.action == "a"
+            assert e.command == "c"
+
+        try:
+            raise SCons.Errors.BuildError()
+        except SCons.Errors.BuildError, e:
+            assert e.errstr == "Unknown error"
+            assert e.status == 2
+            assert e.exitstatus == 2
+            assert e.filename == None
+            assert e.exc_info == (None, None, None)
+
+            assert e.node == None
+            assert e.executor == None
+            assert e.action == None
+            assert e.command == None
 
     def test_InternalError(self):
         """Test the InternalError exception."""
@@ -58,14 +97,6 @@ class ErrorsTestCase(unittest.TestCase):
         except SCons.Errors.ExplicitExit, e:
             assert e.node == "node"
 
-    def test_TaskmasterException(self):
-        """Test the TaskmasterException exception."""
-        try:
-            raise SCons.Errors.TaskmasterException("tm exception", (1, 2, 3))
-        except SCons.Errors.TaskmasterException, e:
-            assert e.node == "tm exception"
-            assert e.exc_info == (1, 2, 3)
-
 if __name__ == "__main__":
     suite = unittest.makeSuite(ErrorsTestCase, 'test_')
     if not unittest.TextTestRunner().run(suite).wasSuccessful():
diff --git a/src/engine/SCons/Executor.py b/src/engine/SCons/Executor.py
index fe3cb62..6b9ab0f 100644
--- a/src/engine/SCons/Executor.py
+++ b/src/engine/SCons/Executor.py
@@ -134,7 +134,11 @@ class Executor:
                 raise status
             elif status:
                 msg = "Error %s" % status
-                raise SCons.Errors.BuildError(errstr=msg, executor=self, action=act)
+                raise SCons.Errors.BuildError(
+                    errstr=msg, 
+                    node=self.targets,
+                    executor=self, 
+                    action=act)
         return status
 
     # use extra indirection because with new-style objects (Python 2.2
diff --git a/src/engine/SCons/Node/FSTests.py b/src/engine/SCons/Node/FSTests.py
index 4081a7a..3d1afdc 100644
--- a/src/engine/SCons/Node/FSTests.py
+++ b/src/engine/SCons/Node/FSTests.py
@@ -361,7 +361,7 @@ class VariantDirTestCase(unittest.TestCase):
 
         save_Link = SCons.Node.FS.Link
         def Link_IOError(target, source, env):
-            raise IOError, "Link_IOError"
+            raise IOError, (17, "Link_IOError")
         SCons.Node.FS.Link = SCons.Action.Action(Link_IOError, None)
 
         test.write(['work', 'src', 'IOError'], "work/src/IOError\n")
diff --git a/src/engine/SCons/Script/Main.py b/src/engine/SCons/Script/Main.py
index 7b6b52b..62a52d6 100644
--- a/src/engine/SCons/Script/Main.py
+++ b/src/engine/SCons/Script/Main.py
@@ -231,54 +231,51 @@ class BuildTask(SCons.Taskmaster.Task):
         # Handle the failure of a build task.  The primary purpose here
         # is to display the various types of Errors and Exceptions
         # appropriately.
-        status = 2
         exc_info = self.exc_info()
         try:
             t, e, tb = exc_info
         except ValueError:
             t, e = exc_info
             tb = None
+
         if t is None:
             # The Taskmaster didn't record an exception for this Task;
             # see if the sys module has one.
-            t, e = sys.exc_info()[:2]
-
-        def nodestring(n):
-            if not SCons.Util.is_List(n):
-                n = [ n ]
-            return string.join(map(str, n), ', ')
+            try:
+                t, e, tb = sys.exc_info()[:]
+            except ValueError:
+                t, e = exc_info
+                tb = None
+                
+        # Deprecated string exceptions will have their string stored
+        # in the first entry of the tuple.
+        if e is None:
+            e = t
+
+        buildError = SCons.Errors.convert_to_BuildError(e)
+        if not buildError.node:
+            buildError.node = self.node
+
+        node = buildError.node
+        if not SCons.Util.is_List(node):
+                node = [ node ]
+        nodename = string.join(map(str, node), ', ')
 
         errfmt = "scons: *** [%s] %s\n"
+        sys.stderr.write(errfmt % (nodename, buildError))
 
-        if t == SCons.Errors.BuildError:
-            tname = nodestring(e.node)
-            errstr = e.errstr
-            if e.filename:
-                errstr = e.filename + ': ' + errstr
-            sys.stderr.write(errfmt % (tname, errstr))
-        elif t == SCons.Errors.TaskmasterException:
-            tname = nodestring(e.node)
-            sys.stderr.write(errfmt % (tname, e.errstr))
-            type, value, trace = e.exc_info
+        if (buildError.exc_info[2] and buildError.exc_info[1] and 
+            not isinstance(
+                buildError.exc_info[1], 
+                (EnvironmentError, SCons.Errors.StopError, SCons.Errors.UserError))):
+            type, value, trace = buildError.exc_info
             traceback.print_exception(type, value, trace)
-        elif t == SCons.Errors.ExplicitExit:
-            status = e.status
-            tname = nodestring(e.node)
-            errstr = 'Explicit exit, status %s' % status
-            sys.stderr.write(errfmt % (tname, errstr))
-        else:
-            if e is None:
-                e = t
-            s = str(e)
-            if t == SCons.Errors.StopError and not self.options.keep_going:
-                s = s + '  Stop.'
-            sys.stderr.write("scons: *** %s\n" % s)
-
-            if tb and print_stacktrace:
-                sys.stderr.write("scons: internal stack trace:\n")
-                traceback.print_tb(tb, file=sys.stderr)
+        elif tb and print_stacktrace:
+            sys.stderr.write("scons: internal stack trace:\n")
+            traceback.print_tb(tb, file=sys.stderr)
 
-        self.do_failed(status)
+        self.exception = (e, buildError, tb) # type, value, traceback
+        self.do_failed(buildError.exitstatus)
 
         self.exc_clear()
 
@@ -1273,6 +1270,8 @@ def main():
     except SConsPrintHelpException:
         parser.print_help()
         exit_status = 0
+    except SCons.Errors.BuildError, e:
+        exit_status = e.exitstatus
     except:
         # An exception here is likely a builtin Python exception Python
         # code in an SConscript file.  Show them precisely what the
diff --git a/src/engine/SCons/Taskmaster.py b/src/engine/SCons/Taskmaster.py
index f8c494d..979b9e9 100644
--- a/src/engine/SCons/Taskmaster.py
+++ b/src/engine/SCons/Taskmaster.py
@@ -225,9 +225,11 @@ class Task:
             raise
         except SCons.Errors.BuildError:
             raise
-        except:
-            raise SCons.Errors.TaskmasterException(self.targets[0],
-                                                   sys.exc_info())
+        except Exception, e:
+            buildError = SCons.Errors.convert_to_BuildError(e)
+            buildError.node = self.targets[0]
+            buildError.exc_info = sys.exc_info()
+            raise buildError
 
     def executed_without_callbacks(self):
         """
diff --git a/src/engine/SCons/TaskmasterTests.py b/src/engine/SCons/TaskmasterTests.py
index 9324581..0735cc1 100644
--- a/src/engine/SCons/TaskmasterTests.py
+++ b/src/engine/SCons/TaskmasterTests.py
@@ -876,9 +876,9 @@ class TaskmasterTestCase(unittest.TestCase):
         t = tm.next_task()
         try:
             t.execute()
-        except SCons.Errors.TaskmasterException, e:
+        except SCons.Errors.BuildError, e:
             assert e.node == n4, e.node
-            assert e.errstr == "Exception", e.errstr
+            assert e.errstr == "OtherError : ", e.errstr
             assert len(e.exc_info) == 3, e.exc_info
             exc_traceback = sys.exc_info()[2]
             assert type(e.exc_info[2]) == type(exc_traceback), e.exc_info[2]