diff options
| -rw-r--r-- | src/CHANGES.txt | 5 | ||||
| -rw-r--r-- | src/engine/SCons/Builder.py | 152 | ||||
| -rw-r--r-- | src/engine/SCons/BuilderTests.py | 29 | ||||
| -rw-r--r-- | src/engine/SCons/Environment.py | 28 | ||||
| -rw-r--r-- | src/engine/SCons/EnvironmentTests.py | 99 | ||||
| -rw-r--r-- | src/engine/SCons/Script/__init__.py | 2 | ||||
| -rw-r--r-- | src/engine/SCons/Warnings.py | 3 | ||||
| -rw-r--r-- | test/option--warn.py | 40 | ||||
| -rw-r--r-- | test/overrides.py | 37 |
9 files changed, 372 insertions, 23 deletions
diff --git a/src/CHANGES.txt b/src/CHANGES.txt index e306bd9..45295c6 100644 --- a/src/CHANGES.txt +++ b/src/CHANGES.txt @@ -68,6 +68,11 @@ RELEASE 0.96 - XXX - Add a --debug=presub option to print actions prior to substitution. + - Add a warning upon use of the override keywords "targets" and + "sources" when calling Builders. These are usually mistakes which + are otherwise silently (and confusingly) turned into construction + variable overrides. + From Simon Perkins: - Fix a bug introduced in building shared libraries under MinGW. diff --git a/src/engine/SCons/Builder.py b/src/engine/SCons/Builder.py index 0da770a..3d1fa87 100644 --- a/src/engine/SCons/Builder.py +++ b/src/engine/SCons/Builder.py @@ -3,19 +3,95 @@ Builder object subsystem. A Builder object is a callable that encapsulates information about how -to execute actions to create a Node (file) from other Nodes (files), and -how to create those dependencies for tracking. +to execute actions to create a target Node (file) from source Nodes +(files), and how to create those dependencies for tracking. -The main entry point here is the Builder() factory method. This -provides a procedural interface that creates the right underlying -Builder object based on the keyword arguments supplied and the types of -the arguments. +The main entry point here is the Builder() factory method. This provides +a procedural interface that creates the right underlying Builder object +based on the keyword arguments supplied and the types of the arguments. The goal is for this external interface to be simple enough that the vast majority of users can create new Builders as necessary to support building new types of files in their configurations, without having to dive any deeper into this subsystem. +The base class here is BuilderBase. This is a concrete base class which +does, in fact, represent most Builder objects that we (or users) create. + +There is (at present) one subclasses: + + MultiStepBuilder + + This is a Builder that knows how to "chain" Builders so that + users can specify a source file that requires multiple steps + to turn into a target file. A canonical example is building a + program from yacc input file, which requires invoking a builder + to turn the .y into a .c, the .c into a .o, and the .o into an + executable program. + +There is also two proxies that look like Builders: + + CompositeBuilder + + This proxies for a Builder with an action that is actually a + dictionary that knows how to map file suffixes to a specific + action. This is so that we can invoke different actions + (compilers, compile options) for different flavors of source + files. + + ListBuilder + + This proxies for a Builder *invocation* where the target + is a list of files, not a single file. + +Builders and their proxies have the following public interface methods +used by other modules: + + __call__() + THE public interface. Calling a Builder object (with the + use of internal helper methods) sets up the target and source + dependencies, appropriate mapping to a specific action, and the + environment manipulation necessary for overridden construction + variable. This also takes care of warning about possible mistakes + in keyword arguments. + + targets() + Returns the list of targets for a specific builder instance. + + add_emitter() + Adds an emitter for a specific file suffix, used by some Tool + modules to specify that (for example) a yacc invocation on a .y + can create a .h *and* a .c file. + + add_action() + Adds an action for a specific file suffix, heavily used by + Tool modules to add their specific action(s) for turning + a source file into an object file to the global static + and shared object file Builders. + +There are the following methods for internal use within this module: + + _execute() + The internal method that handles the heavily lifting when a + Builder is called. This is used so that the __call__() methods + can set up warning about possible mistakes in keyword-argument + overrides, and *then* execute all of the steps necessary so that + the warnings only occur once. + + get_name() + Returns the Builder's name within a specific Environment, + primarily used to try to return helpful information in error + messages. + + adjust_suffix() + get_prefix() + get_suffix() + get_src_suffix() + set_src_suffix() + Miscellaneous stuff for handling the prefix and suffix + manipulation we use in turning source file names into target + file names. + """ # @@ -126,6 +202,40 @@ class ListEmitter(UserList.UserList): target, source = e(target, source, env) return (target, source) +# These are a common errors when calling a Builder; +# they are similar to the 'target' and 'source' keyword args to builders, +# so we issue warnings when we see them. The warnings can, of course, +# be disabled. +misleading_keywords = { + 'targets' : 'target', + 'sources' : 'source', +} + +class OverrideWarner(UserDict.UserDict): + """A class for warning about keyword arguments that we use as + overrides in a Builder call. + + This class exists to handle the fact that a single MultiStepBuilder + call can actually invoke multiple builders as a result of a single + user-level Builder call. This class only emits the warnings once, + no matter how many Builders are invoked. + """ + def __init__(self, dict): + UserDict.UserDict.__init__(self, dict) + self.already_warned = None + def warn(self): + if self.already_warned: + return + for k in self.keys(): + try: + alt = misleading_keywords[k] + except KeyError: + pass + else: + SCons.Warnings.warn(SCons.Warnings.MisleadingKeywordsWarning, + "Did you mean to use `%s' instead of `%s'?" % (alt, k)) + self.already_warned = 1 + def Builder(**kw): """A factory for builder objects.""" composite = None @@ -333,7 +443,7 @@ class BuilderBase: def splitext(self, path): return SCons.Util.splitext(path) - def _create_nodes(self, env, overrides, target = None, source = None): + def _create_nodes(self, env, overwarn, target = None, source = None): """Create and return lists of target and source nodes. """ def _adjustixes(files, pre, suf): @@ -349,7 +459,9 @@ class BuilderBase: result.append(f) return result - env = env.Override(overrides) + overwarn.warn() + + env = env.Override(overwarn.data) src_suf = self.get_src_suffix(env) @@ -399,19 +511,24 @@ class BuilderBase: return tlist, slist - def __call__(self, env, target = None, source = _null, **overrides): + def _execute(self, env, target = None, source = _null, overwarn={}): if source is _null: source = target target = None - tlist, slist = self._create_nodes(env, overrides, target, source) + tlist, slist = self._create_nodes(env, overwarn, target, source) if len(tlist) == 1: - _init_nodes(self, env, overrides, tlist, slist) - tlist = tlist[0] + builder = self + result = tlist[0] else: - _init_nodes(ListBuilder(self, env, tlist), env, overrides, tlist, slist) + builder = ListBuilder(self, env, tlist) + result = tlist + _init_nodes(builder, env, overwarn.data, tlist, slist) - return tlist + return result + + def __call__(self, env, target = None, source = _null, **kw): + return self._execute(env, target, source, OverrideWarner(kw)) def adjust_suffix(self, suff): if suff and not suff[0] in [ '.', '_', '$' ]: @@ -525,7 +642,7 @@ class MultiStepBuilder(BuilderBase): self.sdict = {} self.cached_src_suffixes = {} # source suffixes keyed on id(env) - def __call__(self, env, target = None, source = _null, **kw): + def _execute(self, env, target = None, source = _null, overwarn={}): if source is _null: source = target target = None @@ -552,7 +669,7 @@ class MultiStepBuilder(BuilderBase): for snode in slist: base, ext = self.splitext(str(snode)) if sdict.has_key(ext): - tgt = apply(sdict[ext], (env, None, snode), kw) + tgt = sdict[ext]._execute(env, None, snode, overwarn) # Only supply the builder with sources it is capable # of building. if SCons.Util.is_List(tgt): @@ -566,8 +683,7 @@ class MultiStepBuilder(BuilderBase): else: final_sources.append(snode) - return apply(BuilderBase.__call__, - (self, env, target, final_sources), kw) + return BuilderBase._execute(self, env, target, final_sources, overwarn) def get_src_builders(self, env): """Return all the src_builders for this Builder. diff --git a/src/engine/SCons/BuilderTests.py b/src/engine/SCons/BuilderTests.py index e2dd0e9..bd254b0 100644 --- a/src/engine/SCons/BuilderTests.py +++ b/src/engine/SCons/BuilderTests.py @@ -261,6 +261,35 @@ class BuilderTestCase(unittest.TestCase): target = builder(env, source='n21') assert target.name == 'p-n21.s', target + def test_mistaken_variables(self): + """Test keyword arguments that are often mistakes + """ + import SCons.Warnings + env = Environment() + builder = SCons.Builder.Builder(action="foo") + + save_warn = SCons.Warnings.warn + warned = [] + def my_warn(exception, warning, warned=warned): + warned.append(warning) + SCons.Warnings.warn = my_warn + + try: + target = builder(env, 'mistaken1', sources='mistaken1.c') + assert warned == ["Did you mean to use `source' instead of `sources'?"], warned + del warned[:] + + target = builder(env, 'mistaken2', targets='mistaken2.c') + assert warned == ["Did you mean to use `target' instead of `targets'?"], warned + del warned[:] + + target = builder(env, 'mistaken3', targets='mistaken3', sources='mistaken3.c') + assert "Did you mean to use `source' instead of `sources'?" in warned, warned + assert "Did you mean to use `target' instead of `targets'?" in warned, warned + del warned[:] + finally: + SCons.Warnings.warn = save_warn + def test_action(self): """Test Builder creation diff --git a/src/engine/SCons/Environment.py b/src/engine/SCons/Environment.py index 1b85fe8..57056ad 100644 --- a/src/engine/SCons/Environment.py +++ b/src/engine/SCons/Environment.py @@ -122,6 +122,21 @@ def apply_tools(env, tools, toolpath): else: tool(env) +# These names are controlled by SCons; users should never set or override +# them. This warning can optionally be turned off, but scons will still +# ignore the illegal variable names even if it's off. +reserved_construction_var_names = \ + ['TARGET', 'TARGETS', 'SOURCE', 'SOURCES'] + +def copy_non_reserved_keywords(dict): + result = our_deepcopy(dict) + for k in result.keys(): + if k in reserved_construction_var_names: + SCons.Warnings.warn(SCons.Warnings.ReservedVariableWarning, + "Ignoring attempt to set reserved variable `%s'" % k) + del result[k] + return result + class BuilderWrapper: """Wrapper class that associates an environment with a Builder at instantiation.""" @@ -274,7 +289,7 @@ class Base: return self._dict[key] def __setitem__(self, key, value): - if key in ['TARGET', 'TARGETS', 'SOURCE', 'SOURCES']: + if key in reserved_construction_var_names: SCons.Warnings.warn(SCons.Warnings.ReservedVariableWarning, "Ignoring attempt to set reserved variable `%s'" % key) elif key == 'BUILDERS': @@ -473,7 +488,7 @@ class Base: """Append values to existing construction variables in an Environment. """ - kw = our_deepcopy(kw) + kw = copy_non_reserved_keywords(kw) for key, val in kw.items(): # It would be easier on the eyes to write this using # "continue" statements whenever we finish processing an item, @@ -541,7 +556,7 @@ class Base: """Append values to existing construction variables in an Environment, if they're not already there. """ - kw = our_deepcopy(kw) + kw = copy_non_reserved_keywords(kw) for key, val in kw.items(): if not self._dict.has_key(key): self._dict[key] = val @@ -582,6 +597,7 @@ class Base: apply_tools(clone, tools, toolpath) # Apply passed-in variables after the new tools. + kw = copy_non_reserved_keywords(kw) new = {} for key, value in kw.items(): new[key] = SCons.Util.scons_subst_once(value, self, key) @@ -641,6 +657,7 @@ class Base: env = copy.copy(self) env._dict = copy.copy(self._dict) env['__env__'] = env + overrides = copy_non_reserved_keywords(overrides) new = {} for key, value in overrides.items(): new[key] = SCons.Util.scons_subst_once(value, self, key) @@ -704,7 +721,7 @@ class Base: """Prepend values to existing construction variables in an Environment. """ - kw = our_deepcopy(kw) + kw = copy_non_reserved_keywords(kw) for key, val in kw.items(): # It would be easier on the eyes to write this using # "continue" statements whenever we finish processing an item, @@ -772,7 +789,7 @@ class Base: """Append values to existing construction variables in an Environment, if they're not already there. """ - kw = our_deepcopy(kw) + kw = copy_non_reserved_keywords(kw) for key, val in kw.items(): if not self._dict.has_key(key): self._dict[key] = val @@ -803,6 +820,7 @@ class Base: self.__setitem__('BUILDERS', kwbd) except KeyError: pass + kw = copy_non_reserved_keywords(kw) self._dict.update(our_deepcopy(kw)) def ReplaceIxes(self, path, old_prefix, old_suffix, new_prefix, new_suffix): diff --git a/src/engine/SCons/EnvironmentTests.py b/src/engine/SCons/EnvironmentTests.py index 3f0aab6..3447e7b 100644 --- a/src/engine/SCons/EnvironmentTests.py +++ b/src/engine/SCons/EnvironmentTests.py @@ -2366,6 +2366,105 @@ class EnvironmentTestCase(unittest.TestCase): f = env.xxx('$FOO') assert f == 'foo', f + def test_bad_keywords(type): + """Test trying to use reserved keywords in an Environment""" + reserved = ['TARGETS','SOURCES', 'SOURCE','TARGET'] + added = [] + + env = SCons.Environment.Environment(TARGETS = 'targets', + SOURCES = 'sources', + SOURCE = 'source', + TARGET = 'target', + INIT = 'init') + added.append('INIT') + for x in reserved: + assert not env.has_key(x), env[x] + for x in added: + assert env.has_key(x), \ + '%s is not reserved, but got omitted; see Environment.construction_var_name_ok'%x + + env.Append(TARGETS = 'targets', + SOURCES = 'sources', + SOURCE = 'source', + TARGET = 'target', + APPEND = 'append') + added.append('APPEND') + for x in reserved: + assert not env.has_key(x), env[x] + for x in added: + assert env.has_key(x), \ + '%s is not reserved, but got omitted; see Environment.construction_var_name_ok'%x + + env.AppendUnique(TARGETS = 'targets', + SOURCES = 'sources', + SOURCE = 'source', + TARGET = 'target', + APPENDUNIQUE = 'appendunique') + added.append('APPENDUNIQUE') + for x in reserved: + assert not env.has_key(x), env[x] + for x in added: + assert env.has_key(x), \ + '%s is not reserved, but got omitted; see Environment.construction_var_name_ok'%x + + env.Prepend(TARGETS = 'targets', + SOURCES = 'sources', + SOURCE = 'source', + TARGET = 'target', + PREPEND = 'prepend') + added.append('PREPEND') + for x in reserved: + assert not env.has_key(x), env[x] + for x in added: + assert env.has_key(x), \ + '%s is not reserved, but got omitted; see Environment.construction_var_name_ok'%x + + env.Prepend(TARGETS = 'targets', + SOURCES = 'sources', + SOURCE = 'source', + TARGET = 'target', + PREPENDUNIQUE = 'prependunique') + added.append('PREPENDUNIQUE') + for x in reserved: + assert not env.has_key(x), env[x] + for x in added: + assert env.has_key(x), \ + '%s is not reserved, but got omitted; see Environment.construction_var_name_ok'%x + + env.Replace(TARGETS = 'targets', + SOURCES = 'sources', + SOURCE = 'source', + TARGET = 'target', + REPLACE = 'replace') + added.append('REPLACE') + for x in reserved: + assert not env.has_key(x), env[x] + for x in added: + assert env.has_key(x), \ + '%s is not reserved, but got omitted; see Environment.construction_var_name_ok'%x + + copy = env.Copy(TARGETS = 'targets', + SOURCES = 'sources', + SOURCE = 'source', + TARGET = 'target', + COPY = 'copy') + for x in reserved: + assert not copy.has_key(x), env[x] + for x in added + ['COPY']: + assert copy.has_key(x), \ + '%s is not reserved, but got omitted; see Environment.construction_var_name_ok'%x + + over = env.Override({'TARGETS' : 'targets', + 'SOURCES' : 'sources', + 'SOURCE' : 'source', + 'TARGET' : 'target', + 'OVERRIDE' : 'override'}) + for x in reserved: + assert not over.has_key(x), over[x] + for x in added + ['OVERRIDE']: + assert over.has_key(x), \ + '%s is not reserved, but got omitted; see Environment.construction_var_name_ok'%x + class NoSubstitutionProxyTestCase(unittest.TestCase): diff --git a/src/engine/SCons/Script/__init__.py b/src/engine/SCons/Script/__init__.py index a536366..cac2fcf 100644 --- a/src/engine/SCons/Script/__init__.py +++ b/src/engine/SCons/Script/__init__.py @@ -752,6 +752,8 @@ def _main(args, parser): SCons.Warnings.enableWarningClass(SCons.Warnings.DuplicateEnvironmentWarning) SCons.Warnings.enableWarningClass(SCons.Warnings.MissingSConscriptWarning) SCons.Warnings.enableWarningClass(SCons.Warnings.NoParallelSupportWarning) + # This is good for newbies, and hopefully most everyone else too. + SCons.Warnings.enableWarningClass(SCons.Warnings.MisleadingKeywordsWarning) global ssoptions ssoptions = SConscriptSettableOptions(options) diff --git a/src/engine/SCons/Warnings.py b/src/engine/SCons/Warnings.py index 2e2e525..123f36b 100644 --- a/src/engine/SCons/Warnings.py +++ b/src/engine/SCons/Warnings.py @@ -60,6 +60,9 @@ class NoParallelSupportWarning(Warning): class ReservedVariableWarning(Warning): pass +class MisleadingKeywordsWarning(Warning): + pass + _warningAsException = 0 # The below is a list of 2-tuples. The first element is a class object. diff --git a/test/option--warn.py b/test/option--warn.py index ac510bc..0bf9a7e 100644 --- a/test/option--warn.py +++ b/test/option--warn.py @@ -140,4 +140,44 @@ File "SConstruct", line \d+, in .+ test.run(arguments='--warn=no-duplicate-environment file1.out') + +test.write('SConstruct', """ +def build(env, target, source): + file = open(str(target[0]), 'wb') + for s in source: + file.write(open(str(s), 'rb').read()) + +B = Builder(action=build, multi=1) +env = Environment(BUILDERS = { 'B' : B }) +env.B(targets = 'file3a.out', source = 'file3a.in') +env.B(target = 'file3b.out', sources = 'file3b.in') +""") + +test.write('file3a.in', 'file3a.in\n') +test.write('file3b.out', 'file3b.out\n') + +test.run(arguments='.', + stderr=r""" +scons: warning: Did you mean to use `target' instead of `targets'\? +File "SConstruct", line \d+, in .+ + +scons: warning: Did you mean to use `source' instead of `sources'\? +File "SConstruct", line \d+, in .+ +""") + +test.must_match(['file3a'], 'file3a.in\n') +test.must_match(['file3b'], 'file3b.out\n') + +test.run(arguments='--warn=misleading-keywords .', + stderr=r""" +scons: warning: Did you mean to use `target' instead of `targets'\? +File "SConstruct", line \d+, in .+ + +scons: warning: Did you mean to use `source' instead of `sources'\? +File "SConstruct", line \d+, in .+ +""") + +test.run(arguments='--warn=no-misleading-keywords .') + + test.pass_test() diff --git a/test/overrides.py b/test/overrides.py index a247f7d..eb43ced 100644 --- a/test/overrides.py +++ b/test/overrides.py @@ -92,4 +92,41 @@ assert test.read('hello.not_exe') == 'this is not a program!' test.up_to_date(arguments='hello.not_exe') + + +test.write('SConstruct', """\ +env = Environment() +env.Program('goodbye', 'goodbye.c', + CC=r'%s mycc.py', + LINK=r'%s mylink.py', + OBJSUFFIX='.not_obj', + PROGSUFFIX='.not_exe', + targets='ttt', + sources='sss') +""" % (python, python)) + +test.write('goodbye.c',"this ain't no c file!\n") + +test.write('mycc.py',""" +open('goodbye.not_obj', 'wt').write('this is no object file!') +""") + +test.write('mylink.py',""" +open('goodbye.not_exe', 'wt').write('this is not a program!') +""") + +test.run(arguments='goodbye.not_exe', stderr="""\ + +scons: warning: Did you mean to use `target' instead of `targets'? +File "SConstruct", line 8, in ? + +scons: warning: Did you mean to use `source' instead of `sources'? +File "SConstruct", line 8, in ? +""") + +assert test.read('goodbye.not_obj') == 'this is no object file!' +assert test.read('goodbye.not_exe') == 'this is not a program!' + + + test.pass_test() |