Add Validation for BuildFactory Configuration

master/skia_master_scripts/utils.py

Index: master/skia_master_scripts/utils.py
===================================================================
--- master/skia_master_scripts/utils.py	(revision 8776)
+++ master/skia_master_scripts/utils.py	(working copy)
@@ -23,14 +23,6 @@
 from master.builders_pools import BuildersPools
 from oauth2client.client import SignedJwtAssertionCredentials
 
-from skia_master_scripts import android_factory
-from skia_master_scripts import chromeos_factory
-from skia_master_scripts import factory as skia_factory
-from skia_master_scripts import housekeeping_percommit_factory, \
-                                housekeeping_periodic_factory
-from skia_master_scripts import ios_factory
-from skia_master_scripts import nacl_factory
-
 import config_private
 
 
@@ -46,6 +38,62 @@
   return builder_name.endswith(TRYBOT_NAME_SUFFIX)
 
 
+def _IndentStr(indent):
+  string = ''

[epoger, 2013/04/25 18:12:29]

I think you can implement this as:

return '    ' * (indent + 1)

[borenet, 2013/04/25 18:28:09]

Done.

+  for _ in range(indent + 1):
+    string += '    '
+  return string
+
+
+def ToString(o, indent=0):
+  """ Returns a string representation of the given object. This differs from the
+  built-in string function in that it does not give memory locations.
+z

[epoger, 2013/04/25 18:12:29]

z ???

[borenet, 2013/04/25 18:28:09]

Done.

+  o: the object to print.

[epoger, 2013/04/25 18:12:29]

suggest o -> obj

[borenet, 2013/04/25 18:28:09]

Done.

+  """
+  if isinstance(o, list):
+    return _ListToString(o, indent)
+  elif isinstance(o, dict):
+    return _DictToString(o, indent)
+  elif isinstance(o, tuple):
+    return _ListToString(o, indent)
+  elif isinstance(o, str):
+    return '\'%s\'' % o
+  elif o is None:
+    return 'None'
+  else:
+    return '<Object>'
+
+
+def _ListToString(l, indent):

[epoger, 2013/04/25 18:12:29]

suggest "list" or a variable name like that; "l" looks too much like "1"

[borenet, 2013/04/25 18:28:09]

Changed to list_var to avoid collision with built-in "list".

+  if not l:
+    return '[]'
+  indent_str = _IndentStr(indent)
+  val = '[\n'
+  indent += 1
+  val += ''.join(['%s%s,\n' % (indent_str, ToString(elem, indent)) \
+                  for elem in l])
+  indent -= 1
+  indent_str = _IndentStr(indent - 1)
+  val += indent_str + ']'
+  return val
+
+
+def _DictToString(d, indent):
+  if not d:
+    return '{}'
+  indent_str = _IndentStr(indent)
+  val = '{\n'
+  indent += 1
+  val += ''.join(['%s%s: %s,\n' % (indent_str, ToString(k, indent),
+                                   ToString(d[k], indent)) \
+                  for k in sorted(d.keys())])
+  indent -= 1
+  indent_str = _IndentStr(indent - 1)
+  val += indent_str + '}'
+  return val
+
+
 class SkiaChangeFilter(ChangeFilter):
   """Skia specific subclass of ChangeFilter."""
 
@@ -387,27 +435,26 @@
 
 
 def MakeDebugBuilderName(builder_base_name):
-  return MakeBuilderName(builder_base_name, skia_factory.CONFIG_DEBUG)
+  return MakeBuilderName(builder_base_name, 'Debug')
 
 
 def MakeReleaseBuilderName(builder_base_name):
-  return MakeBuilderName(builder_base_name, skia_factory.CONFIG_RELEASE)
+  return MakeBuilderName(builder_base_name, 'Release')
 
 
 def MakeBenchBuilderName(builder_base_name):
-  return MakeBuilderName(builder_base_name, skia_factory.CONFIG_BENCH)
+  return MakeBuilderName(builder_base_name, 'Bench')
 
 
 def MakeSchedulerName(builder_base_name):
   return MakeBuilderName(builder_base_name, 'Scheduler')
 
 
-def _MakeBuilderSet(helper, builder_base_name, gm_image_subdir,
+def _MakeBuilderSet(helper, builder_base_name, gm_image_subdir, factory_type,
                     perf_output_basedir=None, extra_branches=None,
-                    factory_type=None, do_compile=True, do_debug=True,
-                    do_release=True, do_bench=True, try_schedulers=None,
-                    compile_bot_warnings_as_errors=True,
-                    **kwargs):
+                    do_compile=True, do_debug=True, do_release=True,
+                    do_bench=True, try_schedulers=None,
+                    compile_bot_warnings_as_errors=True, **kwargs):
   """ Creates a trio of builders for a given platform:
   1. Debug mode builder which runs all steps
   2. Release mode builder which runs all steps EXCEPT benchmarks
@@ -441,7 +488,7 @@
     F('f_%s' % compile_debug_builder_name, factory_type(
         builder_name=compile_debug_builder_name,
         other_subdirs=subdirs_to_checkout,
-        configuration=skia_factory.CONFIG_DEBUG,
+        configuration='Debug',

[epoger, 2013/04/25 18:12:29]

I think there's value in using constants to hold these strings, to avoid a
mistaken "Deubg" somewhere along the line

[borenet, 2013/04/25 18:28:09]

Agreed.  Unfortunately, I can't use the constants directly from SkiaFactory,
since that is a circular import.  However, SkiaFactory validates the
configuration, so we're relatively safe.  Keep in mind, this code will all
disappear soon.

         gm_image_subdir=gm_image_subdir,
         do_patch_step=(try_schedulers is not None),
         perf_output_basedir=None,
@@ -457,7 +504,7 @@
     F('f_%s' % compile_release_builder_name, factory_type(
         builder_name=compile_release_builder_name,
         other_subdirs=subdirs_to_checkout,
-        configuration=skia_factory.CONFIG_RELEASE,
+        configuration='Release',
         gm_image_subdir=gm_image_subdir,
         do_patch_step=(try_schedulers is not None),
         perf_output_basedir=None,
@@ -472,7 +519,7 @@
     F('f_%s' % debug_builder_name, factory_type(
         builder_name=debug_builder_name,
         other_subdirs=subdirs_to_checkout,
-        configuration=skia_factory.CONFIG_DEBUG,
+        configuration='Debug',
         gm_image_subdir=gm_image_subdir,
         do_patch_step=(try_schedulers is not None),
         perf_output_basedir=None,
@@ -487,7 +534,7 @@
     F('f_%s' % no_perf_builder_name,  factory_type(
         builder_name=no_perf_builder_name,
         other_subdirs=subdirs_to_checkout,
-        configuration=skia_factory.CONFIG_RELEASE,
+        configuration='Release',
         gm_image_subdir=gm_image_subdir,
         do_patch_step=(try_schedulers is not None),
         perf_output_basedir=None,
@@ -502,7 +549,7 @@
     F('f_%s' % perf_builder_name, factory_type(
         builder_name=perf_builder_name,
         other_subdirs=subdirs_to_checkout,
-        configuration=skia_factory.CONFIG_RELEASE,
+        configuration='Release',
         gm_image_subdir=gm_image_subdir,
         do_patch_step=(try_schedulers is not None),
         perf_output_basedir=perf_output_basedir,
@@ -518,21 +565,22 @@
 
 
 def MakeBuilderSet(**kwargs):
-  _MakeBuilderAndMaybeTrybotSet(factory_type=skia_factory.SkiaFactory, **kwargs)
+  _MakeBuilderAndMaybeTrybotSet(**kwargs)
 
 
-def MakeHousekeeperBuilderSet(helper, do_trybots, do_upload_results):
+def MakeHousekeeperBuilderSet(helper, percommit_factory_type,
+                              periodic_factory_type, do_trybots, **kwargs):
   B = helper.Builder
   F = helper.Factory
 
   builder_factory_scheduler = [
     # The Percommit housekeeper
     ('Skia_PerCommit_House_Keeping',
-     housekeeping_percommit_factory.HouseKeepingPerCommitFactory,
+     percommit_factory_type,
      'skia_rel'),
     # The Periodic housekeeper
     ('Skia_Periodic_House_Keeping',
-     housekeeping_periodic_factory.HouseKeepingPeriodicFactory,
+     periodic_factory_type,
      'skia_periodic'),
   ]
   if do_trybots:
@@ -545,35 +593,10 @@
     B(builder_name, 'f_%s' % builder_name, scheduler=scheduler)
     F('f_%s' % builder_name,
       factory(
-        do_upload_results=do_upload_results,
-        target_platform=skia_factory.TARGET_PLATFORM_LINUX,
         builder_name=builder_name,
         do_patch_step=(scheduler == TRY_SCHEDULERS_STR),
       ).Build())
 
-
-def MakeAndroidBuilderSet(extra_branches=None, **kwargs):
-  if not extra_branches:
-    extra_branches = []
-  extra_branches.append('android')
-  _MakeBuilderAndMaybeTrybotSet(factory_type=android_factory.AndroidFactory,
-                                extra_branches=extra_branches,
-                                **kwargs)
-
-
-def MakeChromeOSBuilderSet(**kwargs):
-  _MakeBuilderAndMaybeTrybotSet(factory_type=chromeos_factory.ChromeOSFactory,
-                                **kwargs)
-
-
-def MakeIOSBuilderSet(**kwargs):
-  _MakeBuilderAndMaybeTrybotSet(factory_type=ios_factory.iOSFactory, **kwargs)
-
-
-def MakeNaClBuilderSet(**kwargs):
-  _MakeBuilderAndMaybeTrybotSet(factory_type=nacl_factory.NaClFactory, **kwargs)
-
-
 def CanMergeBuildRequests(req1, req2):
   """ Determine whether or not two BuildRequests can be merged. Note that the
   call to buildbot.sourcestamp.SourceStamp.canBeMergedWith() is conspicuously