Add Validation for BuildFactory Configuration

master/skia_master_scripts/utils.py

 # Copyright (c) 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 
 """Miscellaneous utilities needed by the Skia buildbot master."""
 
 
 import httplib2
 import re
 
 # requires Google APIs client library for Python; see
 # https://code.google.com/p/google-api-python-client/wiki/Installation
 from apiclient.discovery import build
 from buildbot.scheduler import AnyBranchScheduler
 from buildbot.schedulers import timed
 from buildbot.schedulers.filter import ChangeFilter
 from buildbot.util import NotABranch
 from config_private import TRY_SVN_BASEURL
 from master import master_config
 from master import try_job_svn
 from master import try_job_rietveld
 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
 
 
 CATEGORY_BUILD = ' Build'
 TRYBOT_NAME_SUFFIX = '_Trybot'
 TRY_SCHEDULER_SVN = 'skia_try_svn'
 TRY_SCHEDULER_RIETVELD = 'skia_try_rietveld'
 TRY_SCHEDULERS = [TRY_SCHEDULER_SVN, TRY_SCHEDULER_RIETVELD]
 TRY_SCHEDULERS_STR = '|'.join(TRY_SCHEDULERS)
 
 
 def IsTrybot(builder_name):
   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."""
 
   def __init__(self, builders, **kwargs):
     self._builders = builders
     ChangeFilter.__init__(self, **kwargs)
 
   def filter_change(self, change):
     """Overrides ChangeFilter.filter_change to pass builders to filter_fn.
 
@@ skipping 321 matching lines @@
 
 def MakeCompileBuilderName(builder_base_name, release=False):
   if release:
     compile_name = 'Compile_Release'
   else:
     compile_name = 'Compile_Debug'
   return MakeBuilderName(builder_base_name, compile_name)
 
 
 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
   3. Release mode builder which runs ONLY benchmarks.
   """
   B = helper.Builder
   F = helper.Factory
 
   if not extra_branches:
     extra_branches = []
@@ skipping 13 matching lines @@
   if do_compile:
     compile_debug_builder_name = MakeCompileBuilderName(builder_base_name,
                                                         release=False)
     B(compile_debug_builder_name, 'f_%s' % compile_debug_builder_name,
         # Do not add gatekeeper for trybots.
         gatekeeper='GateKeeper' if try_schedulers is None else None,
         scheduler=scheduler_name, override_category=CATEGORY_BUILD)
     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,
         compile_warnings_as_errors=compile_bot_warnings_as_errors,
         **kwargs
         ).BuildCompileOnly())
     compile_release_builder_name = MakeCompileBuilderName(builder_base_name,
                                                           release=True)
     B(compile_release_builder_name, 'f_%s' % compile_release_builder_name,
         # Do not add gatekeeper for trybots.
         gatekeeper='GateKeeper' if try_schedulers is None else None,
         scheduler=scheduler_name, override_category=CATEGORY_BUILD)
     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,
         compile_warnings_as_errors=compile_bot_warnings_as_errors,
         **kwargs
         ).BuildCompileOnly())
 
   if do_debug:
     debug_builder_name = MakeDebugBuilderName(builder_base_name)
     B(debug_builder_name, 'f_%s' % debug_builder_name,
         scheduler=scheduler_name)
     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,
         compile_warnings_as_errors=False,
         **kwargs
         ).Build())
 
   if do_release:
     no_perf_builder_name = MakeReleaseBuilderName(builder_base_name)
     B(no_perf_builder_name, 'f_%s' % no_perf_builder_name,
         scheduler=scheduler_name)
     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,
         compile_warnings_as_errors=False,
         **kwargs
         ).BuildNoPerf())
 
   if do_bench:
     perf_builder_name = MakeBenchBuilderName(builder_base_name)
     B(perf_builder_name, 'f_%s' % perf_builder_name,
         scheduler=scheduler_name)
     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,
         compile_warnings_as_errors=False,
         **kwargs        
         ).BuildPerfOnly())
 
 
 def _MakeBuilderAndMaybeTrybotSet(do_trybots=True, **kwargs):
   _MakeBuilderSet(try_schedulers=None, **kwargs)
   if do_trybots:
     _MakeBuilderSet(try_schedulers=TRY_SCHEDULERS, **kwargs)
 
 
 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:
     # Add the corresponding trybot builders to the above list.
     builder_factory_scheduler.extend([
         (builder + TRYBOT_NAME_SUFFIX, factory, TRY_SCHEDULERS_STR)
         for (builder, factory, _scheduler) in builder_factory_scheduler])
 
   for (builder_name, factory, scheduler) in builder_factory_scheduler:
     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
   missing. This is because that method verifies that:
   1. req1.source.repository == req2.source.repository
   2. req1.source.project == req2.source.project
   3. req1.source.branch == req2.source.branch
   4. req1.patch == None and req2.patch = None
   5. (req1.source.changes and req2.source.changes) or \
      (not req1.source.changes and not req2.source.changes and \
@@ skipping 27 matching lines @@
   # request is associated with a change but the revisions match (#5 above).
   if req1.source.changes and not req2.source.changes:
     return False
   if not req1.source.changes and req2.source.changes:
     return False
   if not (req1.source.changes and req2.source.changes):
     if req1.source.revision != req2.source.revision:
       return False
   
   return True