Revert of Make allow_subannotations more robust.

scripts/slave/annotated_run.py

Index: scripts/slave/annotated_run.py
diff --git a/scripts/slave/annotated_run.py b/scripts/slave/annotated_run.py
index 769977c66d12643f6e302eb61aa0862ee4d7cc3e..1c0f7bb69d9f84db336fdb70d86d4b07dc35b157 100755
--- a/scripts/slave/annotated_run.py
+++ b/scripts/slave/annotated_run.py
@@ -204,11 +204,6 @@
   @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):
@@ -452,6 +447,7 @@
 
   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.
@@ -499,13 +495,6 @@
     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):
@@ -513,11 +502,10 @@
     self._stream = stream
     self._properties = properties
     self._test_data = test_data
-    self._step_results = collections.OrderedDict()
-    self._step_disambiguation_index = {}
-
-    self._annotation = None
-    self._step_result = None
+    self._step_history = collections.OrderedDict()
+
+    self._previous_step_annotation = None
+    self._previous_step_result = None
     self._api = None
 
   @property
@@ -527,14 +515,14 @@
   @property
   def previous_step_result(self):
     """Allows api.step to get the active result from any context."""
-    return self._step_result
+    return self._previous_step_result
 
   def _emit_results(self):
-    annotation = self._annotation
-    step_result = self._step_result
-
-    self._annotation = None
-    self._step_result = None
+    annotation = self._previous_step_annotation
+    step_result = self._previous_step_result
+
+    self._previous_step_annotation = None
+    self._previous_step_result = None
 
     if not annotation or not step_result:
       return
@@ -551,124 +539,47 @@
         step_result._step['~followup_annotations'] = lines
     annotation.step_ended()
 
-  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]
+  def run_step(self, step):
+    ok_ret = step.pop('ok_ret')
+    infra_step = step.pop('infra_step')
+
+    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)
+
+    self._step_history[step['name']] = step
+    self._emit_results()
+
+    step_result = 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'])
     else:
-      self._step_disambiguation_index[step_name] = 1
-    return step_name
-
-  def _disambiguate_step(self, step):
-    """Disambiguates step (destructively) by adding an index afterward. E.g.
-
-    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.
-      # pylint: disable=e0213
-      def BUILD_STEP(ann, name):
-        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()
-
-  def _step_kernel(self, step, step_test, subannotator=None):
-    if not self._test_data.enabled:
-      # Warning: run_step can change the current self._annotation and
-      # 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'])
-    else:
+      self._previous_step_annotation = annotation = self._stream.step(
+              step['name'])
+      annotation.step_started()
       try:
-        self._step_result.retcode = step_test.retcode
+        annotation.stream = cStringIO.StringIO()
+
+        step_result = StepData(step, 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')
-        self._annotation.write_log_lines(
-            'exception', filter(None, trace_lines))
-        self._annotation.step_exception()
-
-    return self._step_result.retcode
-
-  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
+        annotation.write_log_lines('exception', filter(None, trace_lines))
+        annotation.step_exception()
+
+    get_placeholder_results(step_result, placeholders)
+    self._previous_step_result = step_result
+
+    if step_result.retcode in ok_ret:
+      step_result.presentation.status = 'SUCCESS'
+      return step_result
     else:
       if not infra_step:
         state = 'FAILURE'
@@ -677,13 +588,13 @@
         state = 'EXCEPTION'
         exc = recipe_api.InfraFailure
 
-      self._step_result.presentation.status = state
+      step_result.presentation.status = state
       if step_test.enabled:
         # To avoid cluttering the expectations, don't emit this in testmode.
-        self._annotation.emit(
-            'step returned non-zero exit code: %d' % self._step_result.retcode)
-
-      raise exc(step['name'], self._step_result)
+        self._previous_step_annotation.emit(
+            'step returned non-zero exit code: %d' % step_result.retcode)
+
+      raise exc(step['name'], step_result)
 
 
   def run(self, steps_function, api):
@@ -695,18 +606,18 @@
       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:
@@ -714,9 +625,9 @@
 
       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:
@@ -727,10 +638,9 @@
         raise
 
     if final_result is not None:
-      self._step_results[final_result['name']] = (
-        StepData(final_result, final_result['status_code']))
-
-    return RecipeExecutionResult(retcode, self._step_results)
+      self._step_history[final_result['name']] = final_result
+
+    return RecipeExecutionResult(retcode, self._step_history)
 
   def create_step(self, step):  # pylint: disable=R0201
     # This version of engine doesn't do anything, just converts step to dict