Fix retrieving multiple target files from cache. (Bob Halley)

7 files changed, 119 insertions, 29 deletions

diff --git a/src/CHANGES.txt b/src/CHANGES.txt
index 9252736..3e0feaa 100644
--- a/src/CHANGES.txt
+++ b/src/CHANGES.txt
@@ -78,6 +78,12 @@ RELEASE 0.95 - XXX
     environments so long as the builders have the same signature for
     the environments in questions (that is, they're the same action).
 
+  From Bob Halley:
+
+  - When multiple targets are built by a single action, retrieve all
+    of them from cache, not just the first target, and exec the build
+    command if any of the targets isn't present in the cache.
+
   From Steven Knight:
 
   - Fix EnsureSConsVersion() so it checks against the SCons version,
diff --git a/src/engine/SCons/Node/FS.py b/src/engine/SCons/Node/FS.py
index 7c4b383..46d72d7 100644
--- a/src/engine/SCons/Node/FS.py
+++ b/src/engine/SCons/Node/FS.py
@@ -48,6 +48,7 @@ import SCons.Action
 import SCons.Errors
 import SCons.Node
 import SCons.Util
+import SCons.Sig.MD5
 import SCons.Warnings
 
 #
@@ -1311,20 +1312,18 @@ class File(Base):
             except OSError:
                 pass
 
-    def build(self):
-        """Actually build the file.
-
-        This overrides the base class build() method to check for the
-        existence of derived files in a CacheDir before going ahead and
-        building them.
+    def retrieve_from_cache(self):
+        """Try to retrieve the node's content from a cache
 
         This method is called from multiple threads in a parallel build,
         so only do thread safe stuff here. Do thread unsafe stuff in
         built().
+
+        Returns true iff the node was successfully retrieved.
         """
         b = self.is_derived()
         if not b and not self.has_src_builder():
-            return
+            return None
         if b and self.fs.CachePath:
             if self.fs.cache_show:
                 if CacheRetrieveSilent(self, None, None) == 0:
@@ -1336,10 +1335,10 @@ class File(Base):
                             for a in al:
                                 action.show(a)
                     self._for_each_action(do_print)
-                    return
+                    return 1
             elif CacheRetrieve(self, None, None) == 0:
-                return
-        SCons.Node.Node.build(self)
+                return 1
+        return None
 
     def built(self):
         """Called just after this node is sucessfully built."""
@@ -1509,10 +1508,13 @@ class File(Base):
             bsig = self.get_bsig()
             if bsig is None:
                 raise SCons.Errors.InternalError, "cachepath(%s) found a bsig of None" % self.path
-            bsig = str(bsig)
-            subdir = string.upper(bsig[0])
+            # Add the path to the cache signature, because multiple
+            # targets built by the same action will all have the same
+            # build signature, and we have to differentiate them somehow.
+            cache_sig = SCons.Sig.MD5.collect([bsig, self.path])
+            subdir = string.upper(cache_sig[0])
             dir = os.path.join(self.fs.CachePath, subdir)
-            return dir, os.path.join(dir, bsig)
+            return dir, os.path.join(dir, cache_sig)
         return None, None
 
     def target_from_source(self, prefix, suffix, splitext=SCons.Util.splitext):
diff --git a/src/engine/SCons/Node/FSTests.py b/src/engine/SCons/Node/FSTests.py
index 739d4e9..132e8f6 100644
--- a/src/engine/SCons/Node/FSTests.py
+++ b/src/engine/SCons/Node/FSTests.py
@@ -1528,7 +1528,8 @@ class CacheDirTestCase(unittest.TestCase):
             self.retrieved = []
             built_it = None
 
-            f1.build()
+            r = f1.retrieve_from_cache()
+            assert r == 1, r
             assert self.retrieved == [f1], self.retrieved
             assert built_it is None, built_it
 
@@ -1536,9 +1537,10 @@ class CacheDirTestCase(unittest.TestCase):
             self.retrieved = []
             built_it = None
 
-            f1.build()
+            r = f1.retrieve_from_cache()
+            assert r is None, r
             assert self.retrieved == [f1], self.retrieved
-            assert built_it, built_it
+            assert built_it is None, built_it
         finally:
             SCons.Node.FS.CacheRetrieve = save_CacheRetrieve
 
@@ -1554,7 +1556,8 @@ class CacheDirTestCase(unittest.TestCase):
             self.retrieved = []
             built_it = None
 
-            f2.build()
+            r = f2.retrieve_from_cache()
+            assert r == 1, r
             assert self.retrieved == [f2], self.retrieved
             assert built_it is None, built_it
 
@@ -1562,9 +1565,10 @@ class CacheDirTestCase(unittest.TestCase):
             self.retrieved = []
             built_it = None
 
-            f2.build()
+            r = f2.retrieve_from_cache()
+            assert r is None, r
             assert self.retrieved == [f2], self.retrieved
-            assert built_it, built_it
+            assert built_it is None, built_it
         finally:
             SCons.Node.FS.CacheRetrieveSilent = save_CacheRetrieveSilent
 
@@ -1602,12 +1606,19 @@ class CacheDirTestCase(unittest.TestCase):
 
         # Verify how the cachepath() method determines the name
         # of the file in cache.
-        f5 = fs.File("cd.f5")
-        f5.set_bsig('a_fake_bsig')
-        cp = f5.cachepath()
-        dirname = os.path.join('cache', 'A')
-        filename = os.path.join(dirname, 'a_fake_bsig')
-        assert cp == (dirname, filename), cp
+        def my_collect(list):
+            return list[0]
+        save_collect = SCons.Sig.MD5.collect
+        SCons.Sig.MD5.collect = my_collect
+        try:
+            f5 = fs.File("cd.f5")
+            f5.set_bsig('a_fake_bsig')
+            cp = f5.cachepath()
+            dirname = os.path.join('cache', 'A')
+            filename = os.path.join(dirname, 'a_fake_bsig')
+            assert cp == (dirname, filename), cp
+        finally:
+            SCons.Sig.MD5.collect = save_collect
 
         # Verify that no bsig raises an InternalERror
         f6 = fs.File("cd.f6")
@@ -1661,7 +1672,8 @@ class CacheDirTestCase(unittest.TestCase):
             self.retrieved = []
             built_it = None
 
-            f8.build()
+            r = f8.retrieve_from_cache()
+            assert r == 1, r
             assert self.retrieved == [f8], self.retrieved
             assert built_it is None, built_it
 
@@ -1669,9 +1681,10 @@ class CacheDirTestCase(unittest.TestCase):
             self.retrieved = []
             built_it = None
 
-            f8.build()
+            r = f8.retrieve_from_cache()
+            assert r is None, r
             assert self.retrieved == [f8], self.retrieved
-            assert built_it, built_it
+            assert built_it is None, built_it
         finally:
             SCons.Node.FS.CacheRetrieveSilent = save_CacheRetrieveSilent
 
diff --git a/src/engine/SCons/Node/NodeTests.py b/src/engine/SCons/Node/NodeTests.py
index 08e4260..6b8c8ae 100644
--- a/src/engine/SCons/Node/NodeTests.py
+++ b/src/engine/SCons/Node/NodeTests.py
@@ -253,6 +253,12 @@ class NodeTestCase(unittest.TestCase):
             assert str(act.built_target[0]) == "xxx", str(act.built_target[0])
             assert act.built_source == ["yyy", "zzz"], act.built_source
 
+    def test_retrieve_from_cache(self):
+        """Test the base retrieve_from_cache() method"""
+        n = SCons.Node.Node()
+        r = n.retrieve_from_cache()
+        assert r == 0, r
+
     def test_visited(self):
         """Test the base visited() method
 
diff --git a/src/engine/SCons/Node/__init__.py b/src/engine/SCons/Node/__init__.py
index 28264ce..0e453d2 100644
--- a/src/engine/SCons/Node/__init__.py
+++ b/src/engine/SCons/Node/__init__.py
@@ -176,6 +176,17 @@ class Node:
         executor = self.get_executor()
         executor(self, func)
 
+    def retrieve_from_cache(self):
+        """Try to retrieve the node's content from a cache
+
+        This method is called from multiple threads in a parallel build,
+        so only do thread safe stuff here. Do thread unsafe stuff in
+        built().
+
+        Returns true iff the node was successfully retrieved.
+        """
+        return 0
+        
     def build(self):
         """Actually build the node.
 
diff --git a/src/engine/SCons/Taskmaster.py b/src/engine/SCons/Taskmaster.py
index 90d1452..66492de 100644
--- a/src/engine/SCons/Taskmaster.py
+++ b/src/engine/SCons/Taskmaster.py
@@ -92,7 +92,13 @@ class Task:
         prepare(), executed() or failed()."""
 
         try:
-            self.targets[0].build()
+            everything_was_cached = 1
+            for t in self.targets:
+                if not t.retrieve_from_cache():
+                    everything_was_cached = 0
+                    break
+            if not everything_was_cached:
+                self.targets[0].build()
         except KeyboardInterrupt:
             raise
         except SystemExit:
diff --git a/src/engine/SCons/TaskmasterTests.py b/src/engine/SCons/TaskmasterTests.py
index 019611a..63190a5 100644
--- a/src/engine/SCons/TaskmasterTests.py
+++ b/src/engine/SCons/TaskmasterTests.py
@@ -31,6 +31,7 @@ import SCons.Errors
 
 
 built_text = None
+cache_text = []
 visited_nodes = []
 executed = None
 scan_called = 0
@@ -40,6 +41,7 @@ class Node:
         self.name = name
         self.kids = kids
         self.scans = scans
+        self.cached = 0
         self.scanned = 0
         self.scanner = None
         self.builder = Node.build
@@ -55,6 +57,12 @@ class Node:
         for kid in kids:
             kid.parents.append(self)
 
+    def retrieve_from_cache(self):
+        global cache_text
+        if self.cached:
+            cache_text.append(self.name + " retrieved")
+        return self.cached
+
     def build(self):
         global built_text
         built_text = self.name + " built"
@@ -654,6 +662,7 @@ class TaskmasterTestCase(unittest.TestCase):
 
         """
         global built_text
+        global cache_text
 
         n1 = Node("n1")
         tm = SCons.Taskmaster.Taskmaster([n1])
@@ -708,6 +717,43 @@ class TaskmasterTestCase(unittest.TestCase):
         else:
             raise TestFailed, "did not catch expected BuildError"
 
+        built_text = None
+        cache_text = []
+        n5 = Node("n5")
+        n6 = Node("n6")
+        n6.cached = 1
+        tm = SCons.Taskmaster.Taskmaster([n5])
+        t = tm.next_task()
+        # This next line is moderately bogus.  We're just reaching
+        # in and setting the targets for this task to an array.  The
+        # "right" way to do this would be to have the next_task() call
+        # set it up by having something that approximates a real Builder
+        # return this list--but that's more work than is probably
+        # warranted right now.
+        t.targets = [n5, n6]
+        t.execute()
+        assert built_text == "n5 built", built_text
+        assert cache_text == [], cache_text
+
+        built_text = None
+        cache_text = []
+        n7 = Node("n7")
+        n8 = Node("n8")
+        n7.cached = 1
+        n8.cached = 1
+        tm = SCons.Taskmaster.Taskmaster([n7])
+        t = tm.next_task()
+        # This next line is moderately bogus.  We're just reaching
+        # in and setting the targets for this task to an array.  The
+        # "right" way to do this would be to have the next_task() call
+        # set it up by having something that approximates a real Builder
+        # return this list--but that's more work than is probably
+        # warranted right now.
+        t.targets = [n7, n8]
+        t.execute()
+        assert built_text is None, built_text
+        assert cache_text == ["n7 retrieved", "n8 retrieved"], cache_text
+
     def test_exception(self):
         """Test generic Taskmaster exception handling