Refactor Executor creation from Builders to Actions to set up better for batch builders.

4 files changed, 131 insertions, 77 deletions

diff --git a/src/engine/SCons/Action.py b/src/engine/SCons/Action.py
index 353ead3..51e9f03 100644
--- a/src/engine/SCons/Action.py
+++ b/src/engine/SCons/Action.py
@@ -237,6 +237,11 @@ class ActionBase:
         self.presub_env = None      # don't need this any more
         return lines
 
+    def get_executor(self, env, overrides, tlist, slist, executor_kw):
+        """Return the Executor for this Action."""
+        return SCons.Executor.Executor(self, env, overrides,
+                                       tlist, slist, executor_kw)
+
 if not SCons.Memoize.has_metaclass:
     _Base = ActionBase
     class ActionBase(SCons.Memoize.Memoizer, _Base):
diff --git a/src/engine/SCons/ActionTests.py b/src/engine/SCons/ActionTests.py
index 890abb2..f0905b6 100644
--- a/src/engine/SCons/ActionTests.py
+++ b/src/engine/SCons/ActionTests.py
@@ -322,8 +322,11 @@ class ActionTestCase(unittest.TestCase):
         assert a2 is a1, a2
 
 class ActionBaseTestCase(unittest.TestCase):
-    # Maybe write this in the future...
-    pass
+    def test_get_executor(self):
+        """Test the ActionBase.get_executor() method"""
+        a = SCons.Action.Action('foo')
+        x = a.get_executor({}, {}, [], [], {})
+        assert not x is None, x
  
 class _ActionActionTestCase(unittest.TestCase):
 
diff --git a/src/engine/SCons/Builder.py b/src/engine/SCons/Builder.py
index ec78d89..edc3f71 100644
--- a/src/engine/SCons/Builder.py
+++ b/src/engine/SCons/Builder.py
@@ -282,9 +282,10 @@ def Builder(**kw):
 
     return ret
 
-def _init_nodes(builder, env, overrides, executor_kw, tlist, slist):
-    """Initialize lists of target and source nodes with all of
-    the proper Builder information.
+def _node_errors(builder, env, tlist, slist):
+    """Validate that the lists of target and source nodes are
+    legal for this builder and environment.  Raise errors or
+    issue warnings as appropriate.
     """
 
     # First, figure out if there are any errors in the way the targets
@@ -294,12 +295,13 @@ def _init_nodes(builder, env, overrides, executor_kw, tlist, slist):
             raise UserError, "Multiple ways to build the same target were specified for: %s" % str(t)
         if t.has_explicit_builder():
             if not t.env is None and not t.env is env:
-                t_contents = t.builder.action.get_contents(tlist, slist, t.env)
-                contents = t.builder.action.get_contents(tlist, slist, env)
+                action = t.builder.action
+                t_contents = action.get_contents(tlist, slist, t.env)
+                contents = action.get_contents(tlist, slist, env)
 
                 if t_contents == contents:
                     SCons.Warnings.warn(SCons.Warnings.DuplicateEnvironmentWarning,
-                                        "Two different environments were specified for target %s,\n\tbut they appear to have the same action: %s"%(str(t), t.builder.action.genstring(tlist, slist, t.env)))
+                                        "Two different environments were specified for target %s,\n\tbut they appear to have the same action: %s"%(str(t), action.genstring(tlist, slist, t.env)))
 
                 else:
                     raise UserError, "Two environments with different actions were specified for the same target: %s"%str(t)
@@ -319,36 +321,6 @@ def _init_nodes(builder, env, overrides, executor_kw, tlist, slist):
         if len(slist) > 1:
             raise UserError, "More than one source given for single-source builder: targets=%s sources=%s" % (map(str,tlist), map(str,slist))
 
-    # The targets are fine, so find or make the appropriate Executor to
-    # build this particular list of targets from this particular list of
-    # sources.
-    executor = None
-    if builder.multi:
-        try:
-            executor = tlist[0].get_executor(create = 0)
-        except AttributeError:
-            pass
-        else:
-            executor.add_sources(slist)
-    if executor is None:
-        if not builder.action:
-            raise UserError, "Builder %s must have an action to build %s."%(builder.get_name(env or builder.env), map(str,tlist))
-        executor = SCons.Executor.Executor(builder.action,
-                                           env or builder.env,
-                                           [],  # env already has overrides
-                                           tlist,
-                                           slist,
-                                           executor_kw)
-
-    # Now set up the relevant information in the target Nodes themselves.
-    for t in tlist:
-        t.cwd = env.fs.getcwd()
-        t.builder_set(builder)
-        t.env_set(env)
-        t.add_source(slist)
-        t.set_executor(executor)
-        t.set_explicit(builder.is_explicit)
-
 class EmitterProxy:
     """This is a callable class that can act as a
     Builder emitter.  It holds on to a string that
@@ -458,7 +430,7 @@ class BuilderBase:
         try:
             index = env['BUILDERS'].values().index(self)
             return env['BUILDERS'].keys()[index]
-        except (AttributeError, KeyError, ValueError):
+        except (AttributeError, KeyError, TypeError, ValueError):
             try:
                 return self.name
             except AttributeError:
@@ -478,7 +450,25 @@ class BuilderBase:
                 return [path[:-len(suf)], path[-len(suf):]]
         return SCons.Util.splitext(path)
 
-    def _create_nodes(self, env, overwarn, target = None, source = None):
+    def get_single_executor(self, env, tlist, slist, executor_kw):
+        if not self.action:
+            raise UserError, "Builder %s must have an action to build %s."%(self.get_name(env or self.env), map(str,tlist))
+        return self.action.get_executor(env or self.env,
+                                        [],  # env already has overrides
+                                        tlist,
+                                        slist,
+                                        executor_kw)
+
+    def get_multi_executor(self, env, tlist, slist, executor_kw):
+        try:
+            executor = tlist[0].get_executor(create = 0)
+        except (AttributeError, IndexError):
+            return self.get_single_executor(env, tlist, slist, executor_kw)
+        else:
+            executor.add_sources(slist)
+            return executor
+
+    def _create_nodes(self, env, target = None, source = None):
         """Create and return lists of target and source nodes.
         """
         def _adjustixes(files, pre, suf):
@@ -494,8 +484,6 @@ class BuilderBase:
                 result.append(f)
             return result
 
-        overwarn.warn()
-
         src_suf = self.get_src_suffix(env)
 
         target_factory = env.get_factory(self.target_factory)
@@ -536,7 +524,7 @@ class BuilderBase:
             target, source = self.emitter(target=tlist, source=slist, env=env)
 
             # Now delete the temporary builders that we attached to any
-            # new targets, so that _init_nodes() doesn't do weird stuff
+            # new targets, so that _node_errors() doesn't do weird stuff
             # to them because it thinks they already have builders.
             for t in new_targets:
                 if t.builder is self:
@@ -561,14 +549,36 @@ class BuilderBase:
                 if not src is None: src = [src]
                 result.extend(self._execute(env, tgt, src, overwarn))
             return result
+
+        overwarn.warn()
         
-        tlist, slist = self._create_nodes(env, overwarn, target, source)
+        tlist, slist = self._create_nodes(env, target, source)
 
         if len(tlist) == 1:
             builder = self
         else:
             builder = ListBuilder(self, env, tlist)
-        _init_nodes(builder, env, overwarn.data, executor_kw, tlist, slist)
+
+        # Check for errors with the specified target/source lists.
+        _node_errors(builder, env, tlist, slist)
+
+        # The targets are fine, so find or make the appropriate Executor to
+        # build this particular list of targets from this particular list of
+        # sources.
+        if builder.multi:
+            get_executor = builder.get_multi_executor
+        else:
+            get_executor = builder.get_single_executor
+        executor = get_executor(env, tlist, slist, executor_kw)
+
+        # Now set up the relevant information in the target Nodes themselves.
+        for t in tlist:
+            t.cwd = env.fs.getcwd()
+            t.builder_set(builder)
+            t.env_set(env)
+            t.add_source(slist)
+            t.set_executor(executor)
+            t.set_explicit(builder.is_explicit)
 
         return SCons.Node.NodeList(tlist)
 
diff --git a/src/engine/SCons/BuilderTests.py b/src/engine/SCons/BuilderTests.py
index fbf79f4..6ad5785 100644
--- a/src/engine/SCons/BuilderTests.py
+++ b/src/engine/SCons/BuilderTests.py
@@ -144,6 +144,12 @@ class Environment:
     def __cmp__(self, other):
         return cmp(self.scanner, other.scanner) or cmp(self.d, other.d)
 
+class MyAction:
+    def __init__(self, action):
+        self.action = action
+    def get_executor(self, env, overrides, tlist, slist, executor_kw):
+        return ['executor'] + [self.action]
+
 class MyNode_without_target_from_source:
     def __init__(self, name):
         self.name = name
@@ -386,6 +392,29 @@ class BuilderTestCase(unittest.TestCase):
         builder = SCons.Builder.Builder(generator=generator)
         assert builder.action.generator == generator
 
+    def test_get_name(self):
+        """Test the get_name() method
+        """
+
+    def test_get_single_executor(self):
+        """Test the get_single_executor() method
+        """
+        b = SCons.Builder.Builder(action='foo')
+        x = b.get_single_executor({}, [], [], {})
+        assert not x is None, x
+
+    def test_get_multi_executor(self):
+        """Test the get_multi_executor() method
+        """
+        b = SCons.Builder.Builder(action='foo', multi=1)
+        t1 = MyNode('t1')
+        s1 = MyNode('s1')
+        s2 = MyNode('s2')
+        x1 = b.get_multi_executor({}, [t1], [s1], {})
+        t1.executor = x1
+        x2 = b.get_multi_executor({}, [t1], [s2], {})
+        assert x1 is x2, "%s is not %s" % (repr(x1), repr(x2))
+
     def test_cmp(self):
         """Test simple comparisons of Builder objects
         """
@@ -724,7 +753,7 @@ class BuilderTestCase(unittest.TestCase):
         builder1 = SCons.Builder.Builder(action='foo',
                                          src_suffix='.bar',
                                          suffix='.foo')
-        builder2 = SCons.Builder.MultiStepBuilder(action='bar',
+        builder2 = SCons.Builder.MultiStepBuilder(action=MyAction('act'),
                                                   src_builder = builder1,
                                                   src_suffix = '.foo')
 
@@ -733,17 +762,20 @@ class BuilderTestCase(unittest.TestCase):
 
         tgt = builder2(env, target='baz',
                        source=['test.bar', 'test2.foo', 'test3.txt'])[0]
-        assert str(tgt.sources[0]) == 'test.foo', str(tgt.sources[0])
-        assert str(tgt.sources[0].sources[0]) == 'test.bar', \
-               str(tgt.sources[0].sources[0])
-        assert str(tgt.sources[1]) == 'test2.foo', str(tgt.sources[1])
-        assert str(tgt.sources[2]) == 'test3.txt', str(tgt.sources[2])
+	s = str(tgt)
+	assert s == 'baz', s
+	s = map(str, tgt.sources)
+	assert s == ['test.foo', 'test2.foo', 'test3.txt'], s
+	s = map(str, tgt.sources[0].sources)
+	assert s == ['test.bar'], s
 
         tgt = builder2(env, None, 'aaa.bar')[0]
-        assert str(tgt) == 'aaa', str(tgt)
-        assert str(tgt.sources[0]) == 'aaa.foo', str(tgt.sources[0])
-        assert str(tgt.sources[0].sources[0]) == 'aaa.bar', \
-               str(tgt.sources[0].sources[0])
+	s = str(tgt)
+	assert s == 'aaa', s
+	s = map(str, tgt.sources)
+	assert s == ['aaa.foo'], s
+	s = map(str, tgt.sources[0].sources)
+	assert s == ['aaa.bar'], s
 
         builder3 = SCons.Builder.MultiStepBuilder(action = 'foo',
                                                   src_builder = 'xyzzy',
@@ -753,21 +785,23 @@ class BuilderTestCase(unittest.TestCase):
         builder4 = SCons.Builder.Builder(action='bld4',
                                          src_suffix='.i',
                                          suffix='_wrap.c')
-        builder5 = SCons.Builder.MultiStepBuilder(action='bld5',
+        builder5 = SCons.Builder.MultiStepBuilder(action=MyAction('act'),
                                                   src_builder=builder4,
                                                   suffix='.obj',
                                                   src_suffix='.c')
-        builder6 = SCons.Builder.MultiStepBuilder(action='bld6',
+        builder6 = SCons.Builder.MultiStepBuilder(action=MyAction('act'),
                                                   src_builder=builder5,
                                                   suffix='.exe',
                                                   src_suffix='.obj')
         tgt = builder6(env, 'test', 'test.i')[0]
-        assert str(tgt) == 'test.exe', str(tgt)
-        assert str(tgt.sources[0]) == 'test_wrap.obj', str(tgt.sources[0])
-        assert str(tgt.sources[0].sources[0]) == 'test_wrap.c', \
-               str(tgt.sources[0].sources[0])
-        assert str(tgt.sources[0].sources[0].sources[0]) == 'test.i', \
-               str(tgt.sources[0].sources[0].sources[0])
+	s = str(tgt)
+        assert s == 'test.exe', s
+	s = map(str, tgt.sources)
+	assert s == ['test_wrap.obj'], s
+	s = map(str, tgt.sources[0].sources)
+	assert s == ['test_wrap.c'], s
+	s = map(str, tgt.sources[0].sources[0].sources)
+	assert s == ['test.i'], s
         
     def test_CompositeBuilder(self):
         """Testing CompositeBuilder class."""
@@ -1440,30 +1474,31 @@ class BuilderTestCase(unittest.TestCase):
                                      'B2': b2,
                                      'B3': b3,
                                      'B4': b4})
-        assert b1.get_name(env) == 'bldr1', b1.get_name(env)
-        assert b2.get_name(env) == 'bldr2', b2.get_name(env)
-        assert b3.get_name(env) == 'bldr3', b3.get_name(env)
-        assert b4.get_name(env) == 'bldr4', b4.get_name(env)
-        assert b5.get_name(env) == 'builder5', b5.get_name(env)
         # With no name, get_name will return the class.  Allow
         # for caching...
-        assert b6.get_name(env) in [
+        b6_names = [
             'SCons.Builder.BuilderBase',
             "<class 'SCons.Builder.BuilderBase'>",
             'SCons.Memoize.BuilderBase',
             "<class 'SCons.Memoize.BuilderBase'>",
-            ], b6.get_name(env)
+        ]
+
+        assert b1.get_name(env) == 'bldr1', b1.get_name(env)
+        assert b2.get_name(env) == 'bldr2', b2.get_name(env)
+        assert b3.get_name(env) == 'bldr3', b3.get_name(env)
+        assert b4.get_name(env) == 'bldr4', b4.get_name(env)
+        assert b5.get_name(env) == 'builder5', b5.get_name(env)
+        assert b6.get_name(env) in b6_names, b6.get_name(env)
+
         assert b1.get_name(env2) == 'B1', b1.get_name(env2)
         assert b2.get_name(env2) == 'B2', b2.get_name(env2)
         assert b3.get_name(env2) == 'B3', b3.get_name(env2)
         assert b4.get_name(env2) == 'B4', b4.get_name(env2)
         assert b5.get_name(env2) == 'builder5', b5.get_name(env2)
-        assert b6.get_name(env2) in [
-            'SCons.Builder.BuilderBase',
-            "<class 'SCons.Builder.BuilderBase'>",
-            'SCons.Memoize.BuilderBase',
-            "<class 'SCons.Memoize.BuilderBase'>",
-            ], b6.get_name(env2)
+        assert b6.get_name(env2) in b6_names, b6.get_name(env2)
+
+        assert b5.get_name(None) == 'builder5', b5.get_name(None)
+        assert b6.get_name(None) in b6_names, b6.get_name(None)
 
         for B in b3.get_src_builders(env):
             assert B.get_name(env) == 'bldr1'
@@ -1472,7 +1507,8 @@ class BuilderTestCase(unittest.TestCase):
 
         tgts = b1(env, target = [outfile, outfile2], source='moo')
         for t in tgts:
-            assert t.builder.get_name(env) == 'ListBuilder(bldr1)'
+            name = t.builder.get_name(env)
+            assert name == 'ListBuilder(bldr1)', name
             # The following are not symbolically correct, because the
             # ListBuilder was only created on behalf of env, so it
             # would probably be OK if better correctness