Adding trybots for Periodic and PerCommit Housekeeper

master/skia_master_scripts/housekeeping_periodic_factory.py

Index: master/skia_master_scripts/housekeeping_periodic_factory.py
===================================================================
--- master/skia_master_scripts/housekeeping_periodic_factory.py	(revision 7935)
+++ master/skia_master_scripts/housekeeping_periodic_factory.py	(working copy)
@@ -30,30 +30,31 @@
     if clobber:
       self.AddSlaveScript(script='clean.py', description='Clean')
 
-    sanitize_script_path = self.TargetPathJoin('tools',
-                                               'sanitize_source_files.py')
-    skia_trunk_svn_baseurl = '%s/%s' % (
-        SKIA_SVN_BASEURL.replace('http', 'https'), 'trunk')
-    # Run the sanitization script.
-    self._skia_cmd_obj.AddRunCommand(
-        command='python %s' % sanitize_script_path,
-        description='RunSanitization')
-    if self._do_upload_results:
-      merge_dir_path = self.TargetPathJoin(tempfile.gettempdir(),
-                                           'sanitize-merge')
-      # Cleanup the previous (if any) sanitize merge dir.
+    if not self._do_patch_step:  # Do not run the sanitizer if it is a try job.

[borenet, 2013/03/01 20:53:00]

I think it's clearer from a user perspective to perform this check in the build
step script itself so that the step can throw a BuildStepWarning indicating that
the step doesn't get run on trybots.  This is what we currently do with
UploadGMResults.

[rmistry, 2013/03/01 21:05:07]

Yes this makes sense in the python scripts, but some build steps here (and in
percommit factory) are bash scripts and some are just bash commands. So to make
it consistent I used the same conditional stmt.
I think if we depend on one behavior for one case and another for other cases it
will be harder for us to maintain.

[borenet, 2013/03/01 21:18:49]

Fair enough.  Actually, now that I think about it, the best way to do it is with
doStepIf, which is a function pointer passed into factory.addStep.  By default,
all of our build steps are instances of SkiaBuildStep and doStepIf is set to
ShouldDoStep (see
https://code.google.com/p/skia/source/browse/buildbot/master/skia_master_scri...).

Maybe we should add a field to SkiaBuildStep which is "skip_on_trybots" or some
such, and add logic in ShouldDoStep to determine if the build is a try, and
return False if skip_on_trybots is True and the build is a try.  The nice thing
about "skipped" steps is that they still show up, but they are colored blue and
are clearly marked "skipped."  This would work no matter what the slave-side
script looks like, and would be consistent across the board.  Thoughts?

+      sanitize_script_path = self.TargetPathJoin('tools',
+                                                 'sanitize_source_files.py')
+      skia_trunk_svn_baseurl = '%s/%s' % (
+          SKIA_SVN_BASEURL.replace('http', 'https'), 'trunk')
+      # Run the sanitization script.
       self._skia_cmd_obj.AddRunCommand(
-        command='rm -rf %s' % merge_dir_path, description='Cleanup')
-      # Upload sanitized files.
-      self._skia_cmd_obj.AddMergeIntoSvn(
-          source_dir_path='.',
-          dest_svn_url=skia_trunk_svn_baseurl,
-          merge_dir_path=merge_dir_path,
-          svn_username_file=self._skia_svn_username_file,
-          svn_password_file=self._skia_svn_password_file,
-          commit_message=WithProperties(
-              'Sanitizing source files in %s' % self._builder_name),
-          description='UploadSanitizedFiles')
+          command='python %s' % sanitize_script_path,
+          description='RunSanitization')
+      if self._do_upload_results:
+        merge_dir_path = self.TargetPathJoin(tempfile.gettempdir(),
+                                             'sanitize-merge')
+        # Cleanup the previous (if any) sanitize merge dir.
+        self._skia_cmd_obj.AddRunCommand(
+          command='rm -rf %s' % merge_dir_path, description='Cleanup')
+        # Upload sanitized files.
+        self._skia_cmd_obj.AddMergeIntoSvn(
+            source_dir_path='.',
+            dest_svn_url=skia_trunk_svn_baseurl,
+            merge_dir_path=merge_dir_path,
+            svn_username_file=self._skia_svn_username_file,
+            svn_password_file=self._skia_svn_password_file,
+            commit_message=WithProperties(
+                'Sanitizing source files in %s' % self._builder_name),
+            description='UploadSanitizedFiles')
 
     # pylint: disable=W0212
     clang_static_analyzer_script_path = self.TargetPathJoin(
@@ -66,21 +67,22 @@
     self.AddSlaveScript(script='check_gs_timestamps.py',
                         description='CheckGoogleStorageTimestamps')
 
-    # pylint: disable=W0212
-    disk_usage_script_path = self.TargetPathJoin(
-        self._skia_cmd_obj._local_slave_script_dir,
-        'check_compute_engine_disk_usage.sh')
-    self._skia_cmd_obj.AddRunCommand(
-        command=('SKIA_COMPUTE_ENGINE_HOSTNAME=%s PERSISTENT_DISK_NAME='
-                 '/home/default/skia-master %s'
-                 % (SKIA_PUBLIC_MASTER, disk_usage_script_path)),
-        description='CheckMasterDiskUsage')
-    self._skia_cmd_obj.AddRunCommand(
-        command=(WithProperties('SKIA_COMPUTE_ENGINE_HOSTNAME=%(slavename)s '
-                                'PERSISTENT_DISK_NAME='
-                                '/home/default/skia-slave ' + \
-                                disk_usage_script_path)),
-        description='CheckHousekeepingSlaveDiskUsage')
+    if not self._do_patch_step:  # Do not run the checkers if it is a try job.
+      # pylint: disable=W0212
+      disk_usage_script_path = self.TargetPathJoin(
+          self._skia_cmd_obj._local_slave_script_dir,
+          'check_compute_engine_disk_usage.sh')
+      self._skia_cmd_obj.AddRunCommand(
+          command=('SKIA_COMPUTE_ENGINE_HOSTNAME=%s PERSISTENT_DISK_NAME='
+                   '/home/default/skia-master %s'
+                   % (SKIA_PUBLIC_MASTER, disk_usage_script_path)),
+          description='CheckMasterDiskUsage')
+      self._skia_cmd_obj.AddRunCommand(
+          command=(WithProperties('SKIA_COMPUTE_ENGINE_HOSTNAME=%(slavename)s '
+                                  'PERSISTENT_DISK_NAME='
+                                  '/home/default/skia-slave ' + \
+                                  disk_usage_script_path)),
+          description='CheckHousekeepingSlaveDiskUsage')
 
     return self