Make allow_subannotations more robust.

scripts/slave/annotated_run.py

 #!/usr/bin/env python
 # 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.
 
 """Entry point for fully-annotated builds.
 
 This script is part of the effort to move all builds to annotator-based
 systems. Any builder configured to use the AnnotatorFactory.BaseFactory()
 found in scripts/master/factory/annotator_factory.py executes a single
@@ skipping 187 matching lines @@
     self.abort_reason = None
 
   @property
   def step(self):
     return copy.deepcopy(self._step)
 
   @property
   def retcode(self):
     return self._retcode
 
+  @retcode.setter
+  def retcode(self, val):
+    assert self._retcode is None, 'Can\'t override already-defined retcode'
+    self._retcode = val
+
   @property
   def presentation(self):
     return self._presentation
 
 # TODO(martiniss) update comment
 # Result of 'render_step', fed into 'step_callback'.
 Placeholders = collections.namedtuple(
     'Placeholders', ['cmd', 'stdout', 'stderr', 'stdin'])
 
 
@@ skipping 214 matching lines @@
   return engine.run(steps, api)
 
 
 class RecipeEngine(object):
   """Knows how to execute steps emitted by a recipe, holds global state such as
   step history and build properties. Each recipe module API has a reference to
   this object.
 
   Recipe modules that are aware of the engine:
     * properties - uses engine.properties.
-    * step_history - uses engine.step_history.
     * step - uses engine.create_step(...).
 
   This class acts mostly as a documentation of expected public engine interface.
   """
 
   @staticmethod
   def create(stream, properties, test_data):
     """Create a new instance of RecipeEngine based on 'engine' property."""
     engine_cls_name = properties.get('engine', 'SequentialRecipeEngine')
     for cls in RecipeEngine.__subclasses__():
@@ skipping 27 matching lines @@
     Args:
       step: ConfigGroup object with information about the step, see
         recipe_modules/step/config.py.
 
     Returns:
       Opaque engine specific object that is understood by 'run_steps' method.
     """
     raise NotImplementedError
 
 
+def _merge(*dicts):
+  result = {}
+  for d in dicts:
+    result.update(d)
+  return result
+
+
 class SequentialRecipeEngine(RecipeEngine):
   """Always runs step sequentially. Currently the engine used by default."""
   def __init__(self, stream, properties, test_data):
     super(SequentialRecipeEngine, self).__init__()
     self._stream = stream
     self._properties = properties
     self._test_data = test_data
-    self._step_history = collections.OrderedDict()
+    self._step_results = collections.OrderedDict()
+    self._step_disambiguation_index = {}
 
-    self._previous_step_annotation = None
-    self._previous_step_result = None
+    self._annotation = None
+    self._step_result = None
     self._api = None
 
   @property
   def properties(self):
     return self._properties
 
   @property
   def previous_step_result(self):
     """Allows api.step to get the active result from any context."""
-    return self._previous_step_result
+    return self._step_result
 
   def _emit_results(self):
-    annotation = self._previous_step_annotation
-    step_result = self._previous_step_result
+    annotation = self._annotation
+    step_result = self._step_result
 
-    self._previous_step_annotation = None
-    self._previous_step_result = None
+    self._annotation = None
+    self._step_result = None
 
     if not annotation or not step_result:
       return
 
     step_result.presentation.finalize(annotation)
     if self._test_data.enabled:
       val = annotation.stream.getvalue()
       lines = filter(None, val.splitlines())
       if lines:
         # note that '~' sorts after 'z' so that this will be last on each
         # step. also use _step to get access to the mutable step
         # dictionary.
         # pylint: disable=w0212
         step_result._step['~followup_annotations'] = lines
     annotation.step_ended()
 
-  def run_step(self, step):
-    ok_ret = step.pop('ok_ret')
-    infra_step = step.pop('infra_step')
+  def _disambiguate_name(self, step_name):
+    if step_name in self._step_disambiguation_index:
+      self._step_disambiguation_index[step_name] += 1
+      step_name += ' (%s)' % self._step_disambiguation_index[step_name]
+    else:
+      self._step_disambiguation_index[step_name] = 1
+    return step_name
 
-    test_data_fn = step.pop('step_test_data', recipe_test_api.StepTestData)
-    step_test = self._test_data.pop_step_test_data(step['name'],
-                                                   test_data_fn)
-    placeholders = render_step(step, step_test)
+  def _disambiguate_step(self, step):
+    """Disambiguates step (destructively) by adding an index afterward. E.g.
 
-    self._step_history[step['name']] = step
+    gclient sync
+    gclient sync (2)
+    ...
+    """
+    step['name'] = self._disambiguate_name(step['name'])
+
+  def _subannotator(self):
+    class Subannotator(object):
+      # We use ann as the self argument because we are closing over the
+      # SequentialRecipeEngine self.

[iannucci, 2015/04/03 20:46:00]

is that a good idea? Can we just hold a presentation object in here and then
assign it after the completion of the step? Then this could be defined at the
module level instead of inside this function.

edit: oh, it's because we need to support BUILD_STEP. bleh.

P.S. BUILD_STEP is deprecated... Should really switch to STEP_STARTED instead.

[luqui, 2015/04/07 23:26:54]

Yeah I noticed that. The script I'm trying support uses it though, and we need
it from the past.

+      # pylint: disable=e0213
+      def BUILD_STEP(ann, name):
+        # TODO(luqui): disambiguate

[iannucci, 2015/04/03 20:46:00]

still a todo?

[luqui, 2015/04/07 23:26:54]

Done.

+        self._open_step({'name': self._disambiguate_name(name)})
+
+      def STEP_WARNINGS(ann):
+        self._step_result.presentation.status = 'WARNING'
+      def STEP_FAILURE(ann):
+        self._step_result.presentation.status = 'FAILURE'
+      def STEP_EXCEPTION(ann):
+        self._step_result.presentation.status = 'EXCEPTION'
+
+      def STEP_TEXT(ann, msg):
+        self._step_result.presentation.step_text = msg
+
+      def STEP_LINK(ann, link_label, link_url):
+        self._step_result.presentation.links[link_label] = link_url
+
+      def STEP_LOG_LINE(ann, log_label, log_line):
+        self._step_result.presentation.logs[log_label] += log_line
+      def STEP_LOG_END(ann, log_label):
+        # We do step finalization all at once.
+        pass
+
+      def SET_BUILD_PROPERTY(ann, name, value):
+        self._step_result.presentation.properties[name] = value
+
+      def STEP_SUMMARY_TEXT(ann, msg):
+        self._step_result.presentation.step_summary_text = msg
+
+    return Subannotator()
+
+  def _open_step(self, step):
     self._emit_results()
+    step_result = StepData(step, None)
+    self._step_results[step['name']] = step_result
+    self._step_result = step_result
+    self._annotation = self._stream.step(step['name'])
+    self._annotation.step_started()
+    if self._test_data.enabled:
+      self._annotation.stream = cStringIO.StringIO()
 
-    step_result = None
-
+  def _step_kernel(self, step, step_test, subannotator=None):
     if not self._test_data.enabled:
-      self._previous_step_annotation, retcode = annotator.run_step(
-        self._stream, **step)
-
-      step_result = StepData(step, retcode)
-      self._previous_step_annotation.annotation_stream.step_cursor(step['name'])
+      # Warning: run_step cah change the current self._annotation and

[iannucci, 2015/04/03 20:46:00]

typo cah?

[luqui, 2015/04/07 23:26:54]

Done.

+      # self._step_result if it uses a subannotator.
+      retcode = annotator.run_step(
+        self._stream,
+        step_annotation=self._annotation,
+        subannotator=subannotator,
+        **step)
+      self._step_result.retcode = retcode
+      # TODO(luqui): What is the purpose of this line?
+      self._annotation.annotation_stream.step_cursor(
+          self._step_result.step['name'])

[iannucci, 2015/04/03 20:46:00]

it was a bandaid to try to fix subannoations from crashing the master, iirc.
Check git blame.

     else:
-      self._previous_step_annotation = annotation = self._stream.step(
-              step['name'])
-      annotation.step_started()
       try:
-        annotation.stream = cStringIO.StringIO()
-
-        step_result = StepData(step, step_test.retcode)
+        self._step_result.retcode = step_test.retcode
       except OSError:
         exc_type, exc_value, exc_tb = sys.exc_info()
         trace = traceback.format_exception(exc_type, exc_value, exc_tb)
         trace_lines = ''.join(trace).split('\n')
-        annotation.write_log_lines('exception', filter(None, trace_lines))
-        annotation.step_exception()
+        self._annotation.write_log_lines(
+            'exception', filter(None, trace_lines))
+        self._annotation.step_exception()
 
-    get_placeholder_results(step_result, placeholders)
-    self._previous_step_result = step_result
+    return self._step_result.retcode
 
-    if step_result.retcode in ok_ret:
-      step_result.presentation.status = 'SUCCESS'
-      return step_result
+  def run_step(self, step):
+    self._disambiguate_step(step)
+    ok_ret = step.pop('ok_ret')
+    infra_step = step.pop('infra_step')
+    allow_subannotations = step.get('allow_subannotations', False)
+
+    test_data_fn = step.pop('step_test_data', recipe_test_api.StepTestData)
+    step_test = self._test_data.pop_step_test_data(step['name'], test_data_fn)
+    placeholders = render_step(step, step_test)
+
+    if allow_subannotations:
+      # TODO(luqui) Make this hierarchical.
+      self._open_step(step)
+      start_annotation = self._annotation
+      retcode = self._step_kernel(step, step_test,
+                                  subannotator=self._subannotator())
+
+      # Open a closing step for presentation modifications.
+      if self._annotation != start_annotation:
+        self._open_step({ 'name': step['name'] + ' (end)' })
+        self._step_result.retcode = retcode
+    else:
+      self._open_step(step)
+      self._step_kernel(step, step_test)
+
+    get_placeholder_results(self._step_result, placeholders)
+
+    if self._step_result.retcode in ok_ret:
+      self._step_result.presentation.status = 'SUCCESS'
+      return self._step_result
     else:
       if not infra_step:
         state = 'FAILURE'
         exc = recipe_api.StepFailure
       else:
         state = 'EXCEPTION'
         exc = recipe_api.InfraFailure
 
-      step_result.presentation.status = state
+      self._step_result.presentation.status = state
       if step_test.enabled:
         # To avoid cluttering the expectations, don't emit this in testmode.
-        self._previous_step_annotation.emit(
-            'step returned non-zero exit code: %d' % step_result.retcode)
+        self._annotation.emit(
+            'step returned non-zero exit code: %d' % self._step_result.retcode)
 
-      raise exc(step['name'], step_result)
+      raise exc(step['name'], self._step_result)
 
 
   def run(self, steps_function, api):
     self._api = api
     retcode = None
     final_result = None
 
     try:
       try:
         retcode = steps_function(api)
         assert retcode is None, (
-        "Non-None return from GenSteps is not supported yet")
+        'Non-None return from GenSteps is not supported yet')
 
         assert not self._test_data.enabled or not self._test_data.step_data, (
-        "Unconsumed test data! %s" % (self._test_data.step_data,))
+        'Unconsumed test data! %s' % (self._test_data.step_data,))
       finally:
         self._emit_results()
     except recipe_api.StepFailure as f:
       retcode = f.retcode or 1
       final_result = {
-        "name": "$final_result",
-        "reason": f.reason,
-        "status_code": retcode
+        'name': '$final_result',
+        'reason': f.reason,
+        'status_code': retcode
       }
 
     except Exception as ex:
       unexpected_exception = self._test_data.is_unexpected_exception(ex)
 
       retcode = -1
       final_result = {
-        "name": "$final_result",
-        "reason": "Uncaught Exception: %r" % ex,
-        "status_code": retcode
+        'name': '$final_result',
+        'reason': 'Uncaught Exception: %r' % ex,
+        'status_code': retcode
       }
 
       with self._stream.step('Uncaught Exception') as s:
         s.step_exception()
         s.write_log_lines('exception', traceback.format_exc().splitlines())
 
       if unexpected_exception:
         raise
 
     if final_result is not None:
-      self._step_history[final_result['name']] = final_result
+      self._step_results[final_result['name']] = (
+        StepData(final_result, final_result['status_code']))
 
-    return RecipeExecutionResult(retcode, self._step_history)
+    return RecipeExecutionResult(retcode, self._step_results)
 
   def create_step(self, step):  # pylint: disable=R0201
     # This version of engine doesn't do anything, just converts step to dict
     # (that is consumed by annotator engine).
     return step.as_jsonish()
 
 
 class ParallelRecipeEngine(RecipeEngine):
   """New engine that knows how to run steps in parallel.
 
@@ skipping 51 matching lines @@
 
 def shell_main(argv):
   if update_scripts():
     return subprocess.call([sys.executable] + argv)
   else:
     return main(argv)
 
 
 if __name__ == '__main__':
   sys.exit(shell_main(sys.argv))