SCons
Document setting BUILDERS value to a callable wrapper
This issue was originally created at: 2004-03-11 13:48:42.
This issue was reported by: bwarsaw.
bwarsaw said at 2004-03-11 13:48:42
For example:
def mkdir(env, target, source):
os.makedirs(str(target))
env = Environment(BUILDERS={'mkdir': mkdir})
env.mkdir(env.Dir('src'), None)
The second time you run this, the os.makedirs() call will fail because SCons won't recognize that the 'src' directory already exists and is up-to-date, so it will call the mkdir Builder unnecessarily.
stevenknight said at 2005-11-17 20:36:22
This stems from a problem in the SConscript file, you haven't actually made the mkdir() function into a Builder. The right code would be:
env = Environment(BUILDERS={'mkdir':Builder(action=mkdir)})
SCons should provide a more helpful error message in this case, though, probably an error message to the effect that the value in the BUILDERS dictionary is not a legitimate Builder object. I'm going to change the Summary to reflect that.
--SK
issues@scons said at 2005-11-17 20:36:22
Converted from SourceForge tracker item 914490
gregnoel said at 2009-04-15 20:47:13
Bug party triage. Steven sez, "to characterize whether it is or isn't easy or to fix the doc."
stevenknight said at 2009-11-10 18:00:19
stevenknight => issues@scons
stevenknight said at 2010-02-16 08:39:52
Created an attachment (id=692)
patch to generate an error when setting a non-Builder or non-callable BUILDER
stevenknight said at 2010-02-16 10:31:51
Reassigning and reprioritizing per bug party triage.
The attached patch implements an error message when someone attempts to add an entry to the BUILDERS dict that isn't a Builder object, a proxy Builder object, or a callable().
Note that because the patch accepts callable() as a valid BUILDERS entry, it wouldn't have actually caught this test case.
The reason that this patch accepts callable() is that we have a test case (test/Builder/wrapper.py) that tests for an undocumented feature: it's okay to have a callable function in the BUILDERS dict so long as it returns an actual Builder object. This can be used to add pseudo-Builder "wrappers" that have SCons enforce the normal calling signature for Builders (that is, target, source, with interpretation of strings into Nodes, etc.). In contrast, adding pseudo-Builders "directly" using AddMethod() allows the user complete freedom to invent a custom calling signature with custom semantics.
I think we should integrate the patch I've attached to make our behavior a little more informative, and also update the pseudo-Builder chapter in the User's Guide with descriptions of using callable() functions in BUILDERS as wrappers, and the distinction and trade-offs between that and using AddMethod().
stevenknight said at 2010-02-17 06:15:59
Don't listen to me. I was looking at the wrong code. The wrappers don't have to return a Builder object. They should, however, call a Builder object to add dependencies to the DAG, not to implement the action directly. I can't think of any way to tell, either at SConscript read time or when we walk the DAG, whether or not the wrapper function actually called or calls a Builder.
(We talked about this on IRC yesterday, when I was incorrectly stating that the wrappers needed to return a Builder, and the suggestion was made that we could detect the return of a non-Builder and issue a message then.)
I think we have to stick with just documenting this behavior, most likely in the pseudo-Builders chapter of the User's Guide. I'm going to check in the patch, and then change the Subcomponent to "documentation" and the Summary to reflect that.
stevenknight said at 2010-02-17 06:22:23
Error message added in r4675:
http://scons.tigris.org/source/browse/scons?view=rev&revision=4675 [dead link]
Changing summary to reflect the need to document the wrapper behavior.
stevenknight attached not-a-builder.patch at 2010-02-16 08:39:52.
patch to generate an error when setting a non-Builder or non-callable BUILDER
The code portion of this change is indeed done - I added the "has patch" label then took it back off, as the remaining piece is indeed documentation only.
