Move pre- and post-actions lists from Node to Executor so expansions of ${TARGETS[1:]} work, and the actions aren't executed multiple times.

8 files changed, 137 insertions, 107 deletions

diff --git a/src/CHANGES.txt b/src/CHANGES.txt
index 0763a00..5edaafb 100644
--- a/src/CHANGES.txt
+++ b/src/CHANGES.txt
@@ -279,6 +279,9 @@ RELEASE 0.97 - XXX
     of Python function actions, so that changing the location of an
     otherwise unmodified Python function doesn't cause rebuilds.
 
+  - Fix AddPreAction() and AddPostAction() when an action has more than
+    one target file:  attach the actions to the Executor, not the Node.
+
   From Wayne Lee:
 
   - Avoid "maximum recursion limit" errors when removing $(-$) pairs
diff --git a/src/engine/SCons/Environment.py b/src/engine/SCons/Environment.py
index 58440d6..0e9b642 100644
--- a/src/engine/SCons/Environment.py
+++ b/src/engine/SCons/Environment.py
@@ -1125,15 +1125,21 @@ class Base(SubstitutionEnvironment):
     def AddPreAction(self, files, action):
         nodes = self.arg2nodes(files, self.fs.Entry)
         action = SCons.Action.Action(action)
-        for n in nodes:
-            n.add_pre_action(action)
+        uniq = {}
+        for executor in map(lambda n: n.get_executor(), nodes):
+            uniq[executor] = 1
+        for executor in uniq.keys():
+            executor.add_pre_action(action)
         return nodes
-    
+
     def AddPostAction(self, files, action):
         nodes = self.arg2nodes(files, self.fs.Entry)
         action = SCons.Action.Action(action)
-        for n in nodes:
-            n.add_post_action(action)
+        uniq = {}
+        for executor in map(lambda n: n.get_executor(), nodes):
+            uniq[executor] = 1
+        for executor in uniq.keys():
+            executor.add_post_action(action)
         return nodes
 
     def Alias(self, target, source=[], action=None, **kw):
diff --git a/src/engine/SCons/Executor.py b/src/engine/SCons/Executor.py
index afd6c49..d8bcafb 100644
--- a/src/engine/SCons/Executor.py
+++ b/src/engine/SCons/Executor.py
@@ -30,6 +30,7 @@ Nodes.
 
 __revision__ = "__FILE__ __REVISION__ __DATE__ __DEVELOPER__"
 
+import string
 
 from SCons.Debug import logInstanceCreation
 import SCons.Util
@@ -48,15 +49,25 @@ class Executor:
     def __init__(self, action, env=None, overridelist=[{}],
                  targets=[], sources=[], builder_kw={}):
         if __debug__: logInstanceCreation(self, 'Executor.Executor')
-        if not action:
-            raise SCons.Errors.UserError, "Executor must have an action."
-        self.action = action
+        self.set_action_list(action)
+        self.pre_actions = []
+        self.post_actions = []
         self.env = env
         self.overridelist = overridelist
         self.targets = targets
         self.sources = sources[:]
         self.builder_kw = builder_kw
 
+    def set_action_list(self, action):
+        if not SCons.Util.is_List(action):
+            if not action:
+                raise SCons.Errors.UserError, "Executor must have an action."
+            action = [action]
+        self.action_list = action
+
+    def get_action_list(self):
+        return self.pre_actions + self.action_list + self.post_actions
+
     def get_build_env(self):
         """Fetch or create the appropriate build Environment
         for this Executor.
@@ -98,9 +109,12 @@ class Executor:
 
     def do_execute(self, target, exitstatfunc, kw):
         """Actually execute the action list."""
-        apply(self.action,
-              (self.targets, self.sources, self.get_build_env(), exitstatfunc),
-              self.get_kw(kw))
+        env = self.get_build_env()
+        kw = self.get_kw(kw)
+        for act in self.get_action_list():
+            apply(act,
+                  (self.targets, self.sources, env, exitstatfunc),
+                  kw)
 
     # use extra indirection because with new-style objects (Python 2.2
     # and above) we can't override special methods, and nullify() needs
@@ -120,12 +134,20 @@ class Executor:
         slist = filter(lambda x, s=self.sources: x not in s, sources)
         self.sources.extend(slist)
 
+    def add_pre_action(self, action):
+        self.pre_actions.append(action)
+
+    def add_post_action(self, action):
+        self.post_actions.append(action)
+
     # another extra indirection for new-style objects and nullify...
 
     def my_str(self):
-        return self.action.genstring(self.targets,
-                                     self.sources,
-                                     self.get_build_env())
+        env = self.get_build_env()
+        get = lambda action, t=self.targets, s=self.sources, e=env: \
+                     action.genstring(t, s, e)
+        return string.join(map(get, self.get_action_list()), "\n")
+
 
     def __str__(self):
         "__cacheable__"
@@ -143,9 +165,10 @@ class Executor:
         or source Nodes there are.
         __cacheable__
         """
-        return self.action.get_contents(self.targets,
-                                        self.sources,
-                                        self.get_build_env())
+        env = self.get_build_env()
+        get = lambda action, t=self.targets, s=self.sources, e=env: \
+                     action.get_contents(t, s, e)
+        return string.join(map(get, self.get_action_list()), "")
 
     def get_timestamp(self):
         """Fetch a time stamp for this Executor.  We don't have one, of
@@ -208,8 +231,9 @@ class Executor:
         return map(func, self.get_unignored_sources(ignore))
 
 
+_Executor = Executor
 
-class Null:
+class Null(_Executor):
     """A null Executor, with a null build Environment, that does
     nothing when the rest of the methods call it.
 
@@ -217,8 +241,10 @@ class Null:
     disassociate Builders from Nodes entirely, so we're not
     going to worry about unit tests for this--at least for now.
     """
-    def __init__(self):
+    def __init__(self, *args, **kw):
         if __debug__: logInstanceCreation(self, 'Executor.Null')
+        kw['action'] = []
+        apply(_Executor.__init__, (self,), kw)
     def get_build_env(self):
         class NullEnvironment:
             def get_scanner(self, key):
@@ -226,16 +252,8 @@ class Null:
         return NullEnvironment()
     def get_build_scanner_path(self):
         return None
-    def __call__(self, *args, **kw):
-        pass
     def cleanup(self):
         pass
-    def get_missing_sources(self):
-        return []
-    def get_unignored_sources(self, ignore=[]):
-        return []
-    def process_sources(self, func, ignore=[]):
-        return []
 
 
 
diff --git a/src/engine/SCons/ExecutorTests.py b/src/engine/SCons/ExecutorTests.py
index 9ff30e5..fd16322 100644
--- a/src/engine/SCons/ExecutorTests.py
+++ b/src/engine/SCons/ExecutorTests.py
@@ -103,7 +103,7 @@ class ExecutorTestCase(unittest.TestCase):
         """Test creating an Executor"""
         source_list = ['s1', 's2']
         x = SCons.Executor.Executor('a', 'e', ['o'], 't', source_list)
-        assert x.action == 'a', x.builder
+        assert x.action_list == ['a'], x.action_list
         assert x.env == 'e', x.env
         assert x.overridelist == ['o'], x.overridelist
         assert x.targets == 't', x.targets
@@ -116,6 +116,26 @@ class ExecutorTestCase(unittest.TestCase):
         else:
             raise "Did not catch expected UserError"
 
+    def test__action_list(self):
+        """Test the {get,set}_action_list() methods"""
+        x = SCons.Executor.Executor('a', 'e', 'o', 't', ['s1', 's2'])
+
+        l = x.get_action_list()
+        assert l == ['a'], l
+
+        x.add_pre_action('pre')
+        x.add_post_action('post')
+        l = x.get_action_list()
+        assert l == ['pre', 'a', 'post'], l
+
+        x.set_action_list('b')
+        l = x.get_action_list()
+        assert l == ['pre', 'b', 'post'], l
+
+        x.set_action_list(['c'])
+        l = x.get_action_list()
+        assert l == ['pre', 'c', 'post'], l
+
     def test_get_build_env(self):
         """Test fetching and generating a build environment"""
         x = SCons.Executor.Executor(MyAction(), MyEnvironment(e=1), [],
@@ -196,11 +216,12 @@ class ExecutorTestCase(unittest.TestCase):
 
         env = MyEnvironment()
         a = MyAction([action1, action2])
-        b = MyBuilder(env, {})
-        b.action = a
-        n = MyNode('n', [pre], [post])
-        n.builder = b
-        n.build()
+        t = MyNode('t')
+
+        x = SCons.Executor.Executor(a, env, [], t, ['s1', 's2'])
+        x.add_pre_action(pre)
+        x.add_post_action(post)
+        x(t, lambda x: x)
         assert result == ['pre', 'action1', 'action2', 'post'], result
         del result[:]
 
@@ -210,9 +231,10 @@ class ExecutorTestCase(unittest.TestCase):
                 errfunc(1)
             return 1
 
-        n = MyNode('n', [pre_err], [post])
-        n.builder = b
-        n.build()
+        x = SCons.Executor.Executor(a, env, [], t, ['s1', 's2'])
+        x.add_pre_action(pre_err)
+        x.add_post_action(post)
+        x(t, lambda x: x)
         assert result == ['pre_err', 'action1', 'action2', 'post'], result
         del result[:]
 
@@ -220,7 +242,7 @@ class ExecutorTestCase(unittest.TestCase):
             raise "errfunc %s" % stat
 
         try:
-            n.build(errfunc)
+            x(t, errfunc)
         except:
             assert sys.exc_type == "errfunc 1", sys.exc_type
         else:
@@ -257,6 +279,22 @@ class ExecutorTestCase(unittest.TestCase):
         x.add_sources(['s3', 's1', 's4'])
         assert x.sources == ['s1', 's2', 's3', 's4'], x.sources
 
+    def test_add_pre_action(self):
+        """Test adding pre-actions to an Executor"""
+        x = SCons.Executor.Executor('b', 'e', 'o', 't', ['s1', 's2'])
+        x.add_pre_action('a1')
+        assert x.pre_actions == ['a1']
+        x.add_pre_action('a2')
+        assert x.pre_actions == ['a1', 'a2']
+
+    def test_add_post_action(self):
+        """Test adding post-actions to an Executor"""
+        x = SCons.Executor.Executor('b', 'e', 'o', 't', ['s1', 's2'])
+        x.add_post_action('a1')
+        assert x.post_actions == ['a1']
+        x.add_post_action('a2')
+        assert x.post_actions == ['a1', 'a2']
+
     def test___str__(self):
         """Test the __str__() method"""
         env = MyEnvironment(S='string')
@@ -265,6 +303,15 @@ class ExecutorTestCase(unittest.TestCase):
         c = str(x)
         assert c == 'GENSTRING action1 action2 t s', c
 
+        x = SCons.Executor.Executor(MyAction(), env, [], ['t'], ['s'])
+        x.add_pre_action(MyAction(['pre']))
+        x.add_post_action(MyAction(['post']))
+        c = str(x)
+        expect = 'GENSTRING pre t s\n' + \
+                 'GENSTRING action1 action2 t s\n' + \
+                 'GENSTRING post t s'
+        assert c == expect, c
+
     def test_nullify(self):
         """Test the nullify() method"""
         env = MyEnvironment(S='string')
@@ -301,8 +348,10 @@ class ExecutorTestCase(unittest.TestCase):
 
         x = SCons.Executor.Executor(MyAction(actions=['grow']), env, [],
                                     ['t'], ['s'])
+        x.add_pre_action(MyAction(['pre']))
+        x.add_post_action(MyAction(['post']))
         c = x.get_contents()
-        assert c == 'grow t s', c
+        assert c == 'pre t sgrow t spost t s', c
 
     def test_get_timestamp(self):
         """Test fetching the "timestamp" """
diff --git a/src/engine/SCons/Node/FS.py b/src/engine/SCons/Node/FS.py
index f2234f7..5ae14fc 100644
--- a/src/engine/SCons/Node/FS.py
+++ b/src/engine/SCons/Node/FS.py
@@ -1058,11 +1058,16 @@ class Dir(Base):
         self.entries['.'] = self
         self.entries['..'] = self.dir
         self.cwd = self
-        self.builder = get_MkdirBuilder()
         self.searched = 0
         self._sconsign = None
         self.build_dirs = []
 
+        # Don't just reset the executor, replace its action list,
+        # because it might have some pre-or post-actions that need to
+        # be preserved.
+        self.builder = get_MkdirBuilder()
+        self.get_executor().set_action_list(self.builder.action)
+
     def disambiguate(self):
         return self
 
diff --git a/src/engine/SCons/Node/FSTests.py b/src/engine/SCons/Node/FSTests.py
index 8626185..6978a03 100644
--- a/src/engine/SCons/Node/FSTests.py
+++ b/src/engine/SCons/Node/FSTests.py
@@ -307,6 +307,7 @@ class BuildDirTestCase(unittest.TestCase):
         try:
             dir_made = []
             d9.builder = Builder(fs.Dir, action=MkdirAction(dir_made))
+            d9.reset_executor()
             f9.exists()
             expect = os.path.join('build', 'var2', 'new_dir')
             assert dir_made[0].path == expect, dir_made[0].path
@@ -895,6 +896,7 @@ class FSTestCase(_tempdirTestCase):
         assert not built_it
         d1.add_source([SCons.Node.Node()])    # XXX FAKE SUBCLASS ATTRIBUTE
         d1.builder_set(Builder(fs.File))
+        d1.reset_executor()
         d1.env_set(Environment())
         d1.build()
         assert built_it
@@ -903,6 +905,7 @@ class FSTestCase(_tempdirTestCase):
         assert not built_it
         f1.add_source([SCons.Node.Node()])    # XXX FAKE SUBCLASS ATTRIBUTE
         f1.builder_set(Builder(fs.File))
+        f1.reset_executor()
         f1.env_set(Environment())
         f1.build()
         assert built_it
@@ -1343,6 +1346,18 @@ class FSTestCase(_tempdirTestCase):
 
 class DirTestCase(_tempdirTestCase):
 
+    def test__morph(self):
+        """Test handling of actions when morphing an Entry into a Dir"""
+        test = self.test
+        e = self.fs.Entry('eee')
+        x = e.get_executor()
+        x.add_pre_action('pre')
+        x.add_post_action('post')
+        e.must_be_a_Dir()
+        a = x.get_action_list()
+        assert a[0] == 'pre', a
+        assert a[2] == 'post', a
+
     def test_entry_exists_on_disk(self):
         """Test the Dir.entry_exists_on_disk() method
         """
@@ -2179,6 +2194,7 @@ class prepareTestCase(unittest.TestCase):
         dir_made = []
         new_dir = fs.Dir("new_dir")
         new_dir.builder = Builder(fs.Dir, action=MkdirAction(dir_made))
+        new_dir.reset_executor()
         xyz = fs.File(os.path.join("new_dir", "xyz"))
 
         xyz.set_state(SCons.Node.up_to_date)
diff --git a/src/engine/SCons/Node/NodeTests.py b/src/engine/SCons/Node/NodeTests.py
index 67ac05f..70b9672 100644
--- a/src/engine/SCons/Node/NodeTests.py
+++ b/src/engine/SCons/Node/NodeTests.py
@@ -106,24 +106,6 @@ class MyListAction(MyActionBase):
     def __call__(self, target, source, env, errfunc):
         for A in self.list:
             A(target, source, env, errfunc)
-        
-class MyNonGlobalAction(MyActionBase):
-    def __init__(self):
-        self.order = 0
-        self.built_it = None
-        self.built_target =  None
-        self.built_source =  None
-
-    def __call__(self, target, source, env, errfunc):
-        # Okay, so not ENTIRELY non-global...
-        global built_order
-        self.built_it = 1
-        self.built_target = target
-        self.built_source = source
-        self.built_args = env
-        built_order = built_order + 1
-        self.order = built_order
-        return 0
 
 class Environment:
     def __init__(self, **kw):
@@ -310,37 +292,6 @@ class NodeTestCase(unittest.TestCase):
         assert built_args["on"] == 3, built_args
         assert built_args["off"] == 4, built_args
 
-        built_it = None
-        built_order = 0
-        node = MyNode("xxx")
-        node.builder_set(Builder())
-        node.env_set(Environment())
-        node.sources = ["yyy", "zzz"]
-        pre1 = MyNonGlobalAction()
-        pre2 = MyNonGlobalAction()
-        post1 = MyNonGlobalAction()
-        post2 = MyNonGlobalAction()
-        node.add_pre_action(pre1)
-        node.add_pre_action(pre2)
-        node.add_post_action(post1)
-        node.add_post_action(post2)
-        node.build()
-        assert built_it
-        assert pre1.built_it
-        assert pre2.built_it
-        assert post1.built_it
-        assert post2.built_it
-        assert pre1.order == 1, pre1.order
-        assert pre2.order == 2, pre1.order
-        # The action of the builder itself is order 3...
-        assert post1.order == 4, pre1.order
-        assert post2.order == 5, pre1.order
-
-        for act in [ pre1, pre2, post1, post2 ]:
-            assert type(act.built_target[0]) == type(MyNode("bar")), type(act.built_target[0])
-            assert str(act.built_target[0]) == "xxx", str(act.built_target[0])
-            assert act.built_source == ["yyy", "zzz"], act.built_source
-
     def test_get_build_scanner_path(self):
         """Test the get_build_scanner_path() method"""
         n = SCons.Node.Node()
diff --git a/src/engine/SCons/Node/__init__.py b/src/engine/SCons/Node/__init__.py
index f9390ea..4a1faaa 100644
--- a/src/engine/SCons/Node/__init__.py
+++ b/src/engine/SCons/Node/__init__.py
@@ -179,12 +179,8 @@ class Node:
             try:
                 act = self.builder.action
             except AttributeError:
-                executor = SCons.Executor.Null()
+                executor = SCons.Executor.Null(targets=[self])
             else:
-                if self.pre_actions:
-                    act = self.pre_actions + act
-                if self.post_actions:
-                    act = act + self.post_actions
                 executor = SCons.Executor.Executor(act,
                                                    self.env or self.builder.env,
                                                    [self.builder.overrides],
@@ -833,20 +829,6 @@ class Node:
         the command interpreter literally."""
         return 1
 
-    def add_pre_action(self, act):
-        """Adds an Action performed on this Node only before
-        building it."""
-        self.pre_actions.append(act)
-        # executor must be recomputed to include new pre-actions
-        self.reset_executor()
-
-    def add_post_action(self, act):
-        """Adds and Action performed on this Node only after
-        building it."""
-        self.post_actions.append(act)
-        # executor must be recomputed to include new pre-actions
-        self.reset_executor()
-
     def render_include_tree(self):
         """
         Return a text representation, suitable for displaying to the