display "latest GM failures" link in its own line under build step

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."""
 
 
@@ skipping 230 matching lines @@
         # b. All of the printing can noticeably slow down the master startup
         # c. The master prints so much output that it would be easy to miss the
         #    diff if we did print it.
         print 'Warning: Factory configuration for %s does not match ' \
               'expectation!' % self._builder_name
 
   # TODO(borenet): Can kwargs be used to simplify this function declaration?
   def AddSlaveScript(self, script, description, args=None, timeout=None,
                      halt_on_failure=False,
                      is_upload_render_step=False, is_upload_bench_step=False,
-                     is_rebaseline_step=False, get_props_from_stdout=None,
+                     is_rebaseline_step=False,
+                     get_links_from_stdout=None, get_props_from_stdout=None,
                      workdir=None, do_step_if=None, always_run=False,
                      flunk_on_failure=True, exception_on_failure=False):
     """ Add a BuildStep consisting of a python script.
 
     script: which slave-side python script to run.
     description: string briefly describing the BuildStep; if this description
         is in the self.skipsteps list, this BuildStep will be skipped--unless
         it's in the self.dontskipsteps list, in which case we run it!
     args: optional list of strings; arguments to pass to the script.
     timeout: optional integer; maximum time for the BuildStep to complete.
     halt_on_failure: boolean indicating whether to continue the build if this
         step fails.
     is_upload_render_step: boolean; if true, only run if
         self._do_upload_render_results is True
     is_upload_bench_step: boolean; if true, only run if
         self._do_upload_bench_results is True
     is_rebaseline_step: boolean indicating whether this step is required for
         rebaseline-only builds.
+    get_links_from_stdout: optional dictionary. Keys are strings indicating
+        link text to set based on the output of this step. Values are
+        strings containing regular expressions for parsing the linked URL from
+        the output of the step.
     get_props_from_stdout: optional dictionary. Keys are strings indicating
         build properties to set based on the output of this step. Values are
         strings containing regular expressions for parsing the property from
         the output of the step.
     workdir: optional string indicating the working directory in which to run
         the script. If this is provided, then the script must be given relative
         to this directory.
     do_step_if: optional, function which determines whether or not to run the
         step.  The function is not evaluated until runtime.
     always_run: boolean indicating whether this step should run even if a
@@ skipping 18 matching lines @@
     if args:
       arguments += args
     self._skia_cmd_obj.AddSlaveScript(
         script=script,
         args=arguments,
         description=description,
         timeout=timeout,
         halt_on_failure=halt_on_failure,
         is_upload_step=is_upload_render_step or is_upload_bench_step,
         is_rebaseline_step=is_rebaseline_step,
+        get_links_from_stdout=get_links_from_stdout,
         get_props_from_stdout=get_props_from_stdout,
         workdir=workdir,
         do_step_if=do_step_if,
         always_run=always_run,
         flunk_on_failure=flunk_on_failure,
         exception_on_failure=exception_on_failure)
 
   def AddFlavoredSlaveScript(self, script, args=None, **kwargs):
     """ Add a flavor-specific BuildStep.
 
@@ skipping 207 matching lines @@
   def PostBench(self):
     """ Step to run after the benchmarking steps. """
     self.AddFlavoredSlaveScript(script='postbench.py',
                                 description='PostBench',
                                 exception_on_failure=True)
 
   def CompareGMs(self):
     """Compare the actually-generated GM images to the checked-in baselines."""
     self.AddSlaveScript(script='compare_gms.py',
                         description='CompareGMs',
-                        get_props_from_stdout={
-                            'latest_gm_failures_url':
+                        get_links_from_stdout={
+                            'Latest GM failures on this builder':

[epoger, 2014/06/05 22:15:36]

For more flexibility, we could even allow the client-side script to set up the
number of links, and the text label for each one...

We could define a meta-tag like this:
BUILDBOT_DISPLAY_LINK('link name','http://www.cnn.com')

And every time it appears in the stdout, we would add a link to the buildbot
page.

I guess we would still want build steps to have to "opt in" to get it; otherwise
we would have to run regex matches over the stdout of every build step, just in
case...

[borenet, 2014/06/06 12:10:34]

This makes me wish we had better performance measurements for the master.  My
inclination is to request that we always scan for links and properties and then
add functions to skia_slave_scripts/utils/misc.py:

SetBuildProperty(k, v)
SetLink(name, url)

Each of these would print to stdout in a predictable format like you suggested
above, and then we'd no longer need to do a master restart to set build
properties from a slave-side script.

[epoger, 2014/06/09 13:33:28]

I think those functions sound like a good idea.  Although I could imagine
wanting our native tools (gm and its ilk) to be able to trigger the links also,
and those tools wouldn't be able to call into skia_slave_scripts/utils/misc.py .
 Perhaps the format for the special notations could be shared between buildbot
Python code and C++ code within the skia repo.

At any rate, like you say, it would be awfully helpful to have better
performance measurements for the master before making such a change.  We know
that there are some steps that have ridiculously long stdio logs; I'm afraid
that parsing all of the logs by default could be problematic.

Also... there may be some way we could log these notations into a different
stream, or make them appear at the top of the stdio log and thus only parse the
first part of the log.

                             '%s([^\n]*)\n' % skia_vars.GetGlobalVariable(
                                 'latest_gm_failures_preamble')},
                         is_rebaseline_step=True)
 
   def CompareRenderedSKPs(self):
     """Compare the actual image results of SKP rendering to expectations."""
     self.AddSlaveScript(script='compare_rendered_skps.py',
                         description='CompareRenderedSKPs',
                         is_rebaseline_step=True)
 
@@ skipping 265 matching lines @@
         # Perf-only builder.
         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.PerfSteps()
     self.Validate()
     return self