Add Validation for BuildFactory Configuration

master/skia_master_scripts/factory.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.
 
 
 """Utility class to build the Skia master BuildFactory's.
 
 Based on gclient_factory.py and adds Skia-specific steps."""
 
 
 from buildbot.process.properties import WithProperties
 from config_private import AUTOGEN_SVN_BASEURL, SKIA_SVN_BASEURL
 from master.factory import gclient_factory
 from master.factory.build_factory import BuildFactory
 from skia_master_scripts import commands as skia_commands
 import config
 import config_private
 import ntpath
+import os
 import posixpath
+import utils
 
 
 # TODO(epoger): My intent is to make the build steps identical on all platforms
 # and thus remove the need for the whole target_platform parameter.
 # For now, these must match the target_platform values used in
 # third_party/chromium_buildbot/scripts/master/factory/gclient_factory.py ,
 # because we pass these values into GClientFactory.__init__() .
 TARGET_PLATFORM_LINUX = 'linux'
 TARGET_PLATFORM_MAC = 'mac'
 TARGET_PLATFORM_WIN32 = 'win32'
@@ skipping 76 matching lines @@
 
     # Set _default_clobber based on config.Master
     self._default_clobber = getattr(config.Master, 'default_clobber', False)
 
     self._do_upload_results = do_upload_results
     self._do_upload_bench_results = do_upload_results and \
         perf_output_basedir != None
     self._do_patch_step = do_patch_step
 
     if not environment_variables:
-      my_env_vars = {}
+      self._env_vars = {}

[borenet, 2013/04/25 15:50:28]

Adding this to self so that it gets recorded.

     else:
-      my_env_vars = dict(environment_variables)
-    gyp_defines = my_env_vars.get('GYP_DEFINES', '')
-    my_env_vars['GYP_DEFINES'] = gyp_defines + \
+      self._env_vars = dict(environment_variables)
+    gyp_defines = self._env_vars.get('GYP_DEFINES', '')
+    self._env_vars['GYP_DEFINES'] = gyp_defines + \
         ' skia_warnings_as_errors=%d' % int(compile_warnings_as_errors)
 
     # Get an implementation of SkiaCommands as appropriate for
     # this target_platform.
     workdir = self.TargetPathJoin('build', build_subdir)
     self._skia_cmd_obj = skia_commands.SkiaCommands(
         target_platform=target_platform, factory=self,
         configuration=configuration, workdir=workdir,
         target_arch=None, default_timeout=default_timeout,
-        environment_variables=my_env_vars)
+        environment_variables=self._env_vars)
 
     self._perf_output_basedir = perf_output_basedir
 
     self._configuration = configuration
     if self._configuration not in CONFIGURATIONS:
       raise ValueError('Invalid configuration %s.  Must be one of: %s' % (
           self._configuration, CONFIGURATIONS))
 
     self._skia_svn_username_file = '.skia_svn_username'
     self._skia_svn_password_file = '.skia_svn_password'
@@ skipping 36 matching lines @@
         '--make_flags', '"%s"' % ' '.join(self._make_flags),
         '--test_args', '"%s"' % ' '.join(test_args),
         '--gm_args', '"%s"' % ' '.join(gm_args),
         '--bench_args', '"%s"' % ' '.join(bench_args),
         '--num_cores', WithProperties('%(num_cores:-None)s'),
         '--is_try', str(self._do_patch_step),
         '--bench_pictures_cfg', bench_pictures_cfg,
         ]
     BuildFactory.__init__(self, build_factory_properties=properties)
 
+  def Validate(self):
+    """ Validate the Factory against the known good configuration. """
+    test_dir = os.path.join(os.path.dirname(__file__), os.pardir, os.pardir,
+                            'tools', 'tests', 'factory_configuration')
+    expected_dir = os.path.join(test_dir, 'expected')
+    if not os.path.exists(expected_dir):

[rmistry, 2013/04/25 17:31:50]

I think we can assume that the expected dir always exists.

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

Done.

+      os.makedirs(expected_dir)
+    actual_dir = os.path.join(test_dir, 'actual')
+    if not os.path.exists(actual_dir):
+      os.makedirs(actual_dir)
+    expectation = None
+    try:
+      expectation = open(os.path.join(expected_dir, self._builder_name)).read()
+    except IOError:
+      # Should we error out here?  We really want to have expectations...

[borenet, 2013/04/25 15:50:28]

I'm in favor of throwing an error.  That way, we are forced to add/modify
expectation files every time we add/modify a builder.

[rmistry, 2013/04/25 17:31:50]

Yes lets throw an error, else we will never be able to see the below warning
with all the logs that are outputted.

+      print 'Warning: No expected factory configuration for %s.' % \
+          self._builder_name
+    as_string = utils.ToString(self.__dict__)

[rmistry, 2013/04/25 17:31:50]

as_string -> vars_as_string  or attr_as_string ?

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

Changed to self_as_string, since it really is a representation of the entire
object as a string.

+    with open(os.path.join(actual_dir, self._builder_name), 'w') as f:
+      f.write(as_string)
+    if expectation and as_string != expectation:

[rmistry, 2013/04/25 17:31:50]

if we throw an error above then expecation should always exist and we can do:
if as_string != expectation:

+      raise ValueError('Factory configuration for %s does not match '
+                       'expectation!' % self._builder_name)
+
   def AddSlaveScript(self, script, description, args=None, timeout=None,
                      halt_on_failure=False, is_upload_step=False,
                      is_rebaseline_step=False, get_props_from_stdout=None,
                      workdir=None):
     """ Add a BuildStep consisting of a python script.
 
     script: which slave-side python script to run.
     description: string briefly describing the BuildStep.
     args: optional list of strings; arguments to pass to the script.
     timeout: optional integer; maximum time for the BuildStep to complete.
@@ skipping 293 matching lines @@
       self.UploadBenchGraphs()
 
   def Build(self, clobber=None):
     """Build and return the complete BuildFactory.
 
     clobber: boolean indicating whether we should clean before building
     """
     self.CommonSteps(clobber)
     self.NonPerfSteps()
     self.PerfSteps()
+    self.Validate()

[borenet, 2013/04/25 15:50:28]

Each builder only uses one of these Build functions, so this is safe.  Calling
Validate() here rather than at the end of the constructor causes all of the
build steps to be included in the dump.

     return self
 
   def BuildCompileOnly(self, clobber=None):
     """Build and return the complete BuildFactory, with only the compile step.
     Does not build in one step; the assumption is that if we're only performing
     the compile, we want as much information as possible about *which* compile
     steps are passing and failing.
 
     clobber: boolean indicating whether we should clean before building
     """
     self.UpdateSteps()
     self.Compile(clobber=clobber, build_in_one_step=False)
+    self.Validate()
     return self
 
   def BuildNoPerf(self, clobber=None):
     """Build and return the complete BuildFactory, without the benchmarking
     steps.
 
     clobber: boolean indicating whether we should clean before building
     """
     self.CommonSteps(clobber)
     self.NonPerfSteps()
+    self.Validate()
     return self
 
   def BuildPerfOnly(self, clobber=None):
     """Build and return the complete BuildFactory, with only the benchmarking
     steps.
 
     clobber: boolean indicating whether we should clean before building
     """
     if not self._perf_output_basedir:
       raise ValueError(
           'BuildPerfOnly requires perf_output_basedir to be defined.')
     if self._configuration != CONFIG_RELEASE:
       raise ValueError('BuildPerfOnly should run in %s configuration.' %
                        CONFIG_RELEASE)
     self.CommonSteps(clobber)
     self.PerfSteps()
+    self.Validate()
     return self