Merged fix for #2720 (-jN build failures), pull request #13.

From Alexey Klimkin with test work and cleanup by Dirk Baechle.

5 files changed, 120 insertions, 16 deletions

diff --git a/src/CHANGES.txt b/src/CHANGES.txt
index fada1b7e..f32c3f17 100644
--- a/src/CHANGES.txt
+++ b/src/CHANGES.txt
@@ -6,8 +6,15 @@
 
 RELEASE 2.X.X - 
 
-  From Rob Managan:
+  From Alexey Klimkin:
+    - Fixed the Taskmaster, curing spurious build failures in
+      multi-threaded runs (#2720).
+    
+  From Dirk Baechle:
+    - Improved documentation of command-line variables (#2809).
+    - Fixed scons-doc.py to properly convert main XML files (#2812).
 
+  From Rob Managan:
     - Updated the TeX builder to support LaTeX's multibib package.
 
 
diff --git a/src/engine/SCons/Node/FS.py b/src/engine/SCons/Node/FS.py
index f106d46d..f31ca831 100644
--- a/src/engine/SCons/Node/FS.py
+++ b/src/engine/SCons/Node/FS.py
@@ -2715,7 +2715,7 @@ class File(Base):
         so only do thread safe stuff here. Do thread unsafe stuff in
         built().
 
-        Returns true iff the node was successfully retrieved.
+        Returns true if the node was successfully retrieved.
         """
         if self.nocache:
             return None
diff --git a/src/engine/SCons/Node/__init__.py b/src/engine/SCons/Node/__init__.py
index 2fbe2c6f..992284dc 100644
--- a/src/engine/SCons/Node/__init__.py
+++ b/src/engine/SCons/Node/__init__.py
@@ -216,6 +216,7 @@ class Node(object):
         self.precious = None
         self.noclean = 0
         self.nocache = 0
+        self.cached = 0 # is this node pulled from cache?
         self.always_build = None
         self.includes = None
         self.attributes = self.Attrs() # Generic place to stick information about the Node.
@@ -306,7 +307,7 @@ class Node(object):
         so only do thread safe stuff here. Do thread unsafe stuff in
         built().
 
-        Returns true iff the node was successfully retrieved.
+        Returns true if the node was successfully retrieved.
         """
         return 0
 
diff --git a/src/engine/SCons/Taskmaster.py b/src/engine/SCons/Taskmaster.py
index 58a8d90a..64ab84da 100644
--- a/src/engine/SCons/Taskmaster.py
+++ b/src/engine/SCons/Taskmaster.py
@@ -227,20 +227,26 @@ class Task(object):
         if T: T.write(self.trace_message(u'Task.execute()', self.node))
 
         try:
-            everything_was_cached = 1
+            cached_targets = []
             for t in self.targets:
-                if t.retrieve_from_cache():
-                    # Call the .built() method without calling the
-                    # .push_to_cache() method, since we just got the
-                    # target from the cache and don't need to push
-                    # it back there.
-                    t.set_state(NODE_EXECUTED)
-                    t.built()
-                else:
-                    everything_was_cached = 0
+                if not t.retrieve_from_cache():
                     break
-            if not everything_was_cached:
+                cached_targets.append(t)
+            if len(cached_targets) < len(self.targets):
+                # Remove targets before building. It's possible that we
+                # partially retrieved targets from the cache, leaving
+                # them in read-only mode. That might cause the command
+                # to fail.
+                #
+                for t in cached_targets:
+                    try:
+                        t.fs.unlink(t.path)
+                    except (IOError, OSError):
+                        pass
                 self.targets[0].build()
+            else:
+                for t in cached_targets:
+                    t.cached = 1
         except SystemExit:
             exc_value = sys.exc_info()[1]
             raise SCons.Errors.ExplicitExit(self.targets[0], exc_value.code)
@@ -292,7 +298,8 @@ class Task(object):
                 for side_effect in t.side_effects:
                     side_effect.set_state(NODE_NO_STATE)
                 t.set_state(NODE_EXECUTED)
-                t.push_to_cache()
+                if not t.cached:
+                    t.push_to_cache()
                 t.built()
             t.visited()
 
diff --git a/src/engine/SCons/TaskmasterTests.py b/src/engine/SCons/TaskmasterTests.py
index 66c6b9c4..85ade8df 100644
--- a/src/engine/SCons/TaskmasterTests.py
+++ b/src/engine/SCons/TaskmasterTests.py
@@ -86,16 +86,61 @@ class Node(object):
 
     def prepare(self):
         self.prepared = 1
+        self.get_binfo()        
 
     def build(self):
         global built_text
         built_text = self.name + " built"
 
+    def remove(self):
+        pass
+
+    # The following four methods new_binfo(), del_binfo(),
+    # get_binfo(), clear() as well as its calls have been added
+    # to support the cached_execute() test (issue #2720).
+    # They are full copies (or snippets) of their actual
+    # counterparts in the Node class...
+    def new_binfo(self):
+        binfo = "binfo"
+        return binfo
+
+    def del_binfo(self):
+        """Delete the build info from this node."""
+        try:
+            delattr(self, 'binfo')
+        except AttributeError:
+            pass
+
+    def get_binfo(self):
+        """Fetch a node's build information."""
+        try:
+            return self.binfo
+        except AttributeError:
+            pass
+
+        binfo = self.new_binfo()
+        self.binfo = binfo
+
+        return binfo
+    
+    def clear(self):
+        # The del_binfo() call here isn't necessary for normal execution,
+        # but is for interactive mode, where we might rebuild the same
+        # target and need to start from scratch.
+        self.del_binfo()
+
     def built(self):
         global built_text
         if not self.cached:
             built_text = built_text + " really"
-
+            
+        # Clear the implicit dependency caches of any Nodes
+        # waiting for this Node to be built.
+        for parent in self.waiting_parents:
+            parent.implicit = None
+
+        self.clear()
+        
     def has_builder(self):
         return not self.builder is None
 
@@ -954,6 +999,50 @@ class TaskmasterTestCase(unittest.TestCase):
         assert built_text is None, built_text
         assert cache_text == ["n7 retrieved", "n8 retrieved"], cache_text
 
+    def test_cached_execute(self):
+        """Test executing a task with cached targets
+        """
+        # In issue #2720 Alexei Klimkin detected that the previous
+        # workflow for execute() led to problems in a multithreaded build.
+        # We have:
+        #    task.prepare()
+        #    task.execute()
+        #    task.executed()
+        #        -> node.visited()
+        # for the Serial flow, but
+        #    - Parallel -           - Worker -
+        #      task.prepare()
+        #      requestQueue.put(task)
+        #                           task = requestQueue.get()
+        #                           task.execute()
+        #                           resultQueue.put(task)
+        #      task = resultQueue.get()
+        #      task.executed()
+        #        ->node.visited()
+        # in parallel. Since execute() used to call built() when a target
+        # was cached, it could unblock dependent nodes before the binfo got
+        # restored again in visited(). This resulted in spurious
+        # "file not found" build errors, because files fetched from cache would
+        # be seen as not up to date and wouldn't be scanned for implicit
+        # dependencies.
+        #
+        # The following test ensures that execute() only marks targets as cached,
+        # but the actual call to built() happens in executed() only.
+        # Like this, the binfo should still be intact after calling execute()...
+        global cache_text
+
+        n1 = Node("n1")
+        # Mark the node as being cached
+        n1.cached = 1
+        tm = SCons.Taskmaster.Taskmaster([n1])
+        t = tm.next_task()
+        t.prepare()
+        t.execute()
+        assert cache_text == ["n1 retrieved"], cache_text
+        # If no binfo exists anymore, something has gone wrong...
+        has_binfo = hasattr(n1, 'binfo')
+        assert has_binfo == True, has_binfo
+
     def test_exception(self):
         """Test generic Taskmaster exception handling