Add a recipe to identify culprits for chromium compile failures.

scripts/slave/recipes/findit/chromium/compile.py

+# Copyright 2015 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.
+
+import json
+
+from recipe_engine.config import List
+from recipe_engine.recipe_api import Property
+
+
+DEPS = [
+  'chromium',
+  'chromium_tests',
+  'findit',
+  'gclient',
+  'json',
+  'path',
+  'properties',
+  'python',
+  'step',
+]
+
+
+PROPERTIES = {
+    'target_mastername': Property(
+        kind=str, help='The target master to match compile config to.'),
+    'target_buildername': Property(
+        kind=str, help='The target builder to match compile config to.'),
+    'target_solution': Property(
+        kind=str, help='The gclient solution, eg: src for chromium.'),
+    'target_solution_revisions': Property(
+        kind=List(basestring),
+        help='The revisions to be tested for compile failure, '
+             'ordered from older revisions to newer revisions.'),
+    'compile_targets': Property(
+        kind=List(basestring), default=None, param_name='compile_targets',

[iannucci, 2015/11/18 03:57:54]

yay! real types!

Please note that in order for this to work with `run_recipe.py` or buildbucket
that this will need to be an actual list of strings. So:

  ["list", "of", "strings"]

and not

  list,of,strings

But other than that this is awesome :)

[stgao, 2015/11/19 22:06:34]

Yes, it will be a list of strings in the build request through Buildbucket.

Another note here is that List(str) won't work, because a string in a list is
turned into a unicode string somehow...Not sure why and exactly where...

+        help='The failed compile targets, eg: browser_tests, a/b.o'),

[Dirk Pranke, 2015/11/17 22:38:51]

nit: it's not clear what "a/b.o" is here; is that supposed to be an arbitrary
object file? 

Are you planning to support bisecting into compile failures for single object
files? that seems like it
will be fragile.

[stgao, 2015/11/19 22:06:34]

Yes, it's supposed to be an arbitrary object file.
I removed it for now.

The idea is: if a source file failed to compile on the waterfall, recompiling it
only will speed up the culprit finding because much less compile units have to
be run.

However, I still have to confirm one thing: target "a" depends on target "b",
the source file a.cc included in target "a" depends on a header file b.h
generated in target "b", not sure whether compiling a.o for a.cc only will make
the whole target "b" to be compiled first and b.h is generated as expected.

Another possible issue is that: even if the source file a.cc passes compile, the
target "a" might still fail to compile, or other targets impacted by the commit
might also fail to compile. However, with the assumption that a compile failure
is caused by a single commit which is the most common case except conflicting
commits, this approach could still identify the culprit.
Is this the case you thought fragile?

+}
+
+
+def RunSteps(api, target_mastername, target_buildername,
+             target_solution, target_solution_revisions,
+             compile_targets):
+  api.chromium_tests.configure_build(
+      target_mastername, target_buildername, override_bot_type='builder_tester')
+
+  results = []
+  try:
+    for current_revision in target_solution_revisions:
+      with api.step.nest('test %s' % str(current_revision)):
+        # Configure the revision to recompile.
+        cfg = api.gclient.c
+        if not cfg.revisions:
+          cfg.revisions = {}
+        cfg.revisions.update({
+            target_solution: current_revision,
+        })

[iannucci, 2015/11/18 03:57:54]

This is mutating `api.gclient.c`.

I think a more concise way to express this section would be:

  api.gclient.c.revisions[target_solution] = current_revision

which should work even if cfg.revisions is empty (if not, let me know)

[stgao, 2015/11/19 22:06:34]

Yes, it works!

+
+        bot_update_step, master_dict, test_spec = \
+            api.chromium_tests.prepare_checkout(
+                target_mastername,
+                target_buildername)
+
+        # If no compile target is provided, use analyze to filter out the
+        # impacted compile targets by the current revision.
+        compile_targets = sorted(set(compile_targets or []))
+        if not compile_targets:
+          affected_files = \
+              api.findit.get_files_affected_by_revision(current_revision)
+
+          all_compile_targets, _ = \
+              api.chromium_tests.get_compile_targets_and_tests(
+                target_mastername,
+                target_buildername,
+                master_dict,
+                test_spec,
+                override_bot_type='builder_tester',
+                override_tests=[])
+
+          requires_compile, matching_exes, compile_targets = \

[Dirk Pranke, 2015/11/17 22:38:51]

You should update this logic to the latest version of chromium_tests.analyze:
you can
(and should) just look at the returned compile_targets list, and not need any of
lines 88-95.

[stgao, 2015/11/19 22:06:34]

Done.

Also refactored chromium_tests.get_compile_targets_and_tests into a new function
chromuim_tests.get_test_targets_compile_targets_and_tests to return test_targets
and compile_targets.

+              api.chromium_tests.analyze(
+                  affected_files,
+                  all_compile_targets,
+                  all_compile_targets,
+                  'trybot_analyze_config.json')
+
+
+          if not requires_compile:
+            compile_targets = []
+          else:
+            # Note that compile_targets doesn't necessarily include
+            # matching_exes, and for correctness we need to add them. Otherwise
+            # it's possible we'd only build foo_unittests but not
+            # foo_unittests_run.
+            compile_targets = sorted(set(compile_targets + matching_exes))
+
+        if not compile_targets:
+          # No compile target is impacted by the current revision.
+          results.append([current_revision, 'skip'])
+          continue

[iannucci, 2015/11/18 03:57:54]

I would maybe pull this loop body into a function of current_revision, and have
it return the result. Then you can do the results.append in the loop, and the
control flow will be a bit clearer (e.g. that results is only ever appended to
once per loop).

[stgao, 2015/11/19 22:06:34]

Done.

+
+        try:
+          api.chromium_tests.compile_specific_targets(
+              target_mastername,
+              target_buildername,
+              bot_update_step,
+              master_dict,
+              compile_targets,
+              [], # Not run any test.
+              override_bot_type='builder_tester')
+          results.append([current_revision, 'pass'])
+        except api.step.InfraFailure:
+          results.append([current_revision, 'unknown'])
+          raise
+        except api.step.StepFailure:
+          # TODO: if compile targets are specified in the build request, compile
+          # may fail because those targets are added in a later revision.
+          results.append([current_revision, 'fail'])
+          break # Found the culprit, no need to check later revisions.
+  finally:
+    # Report the result.
+    step_result = api.python.inline(
+        'report', 'import sys; sys.exit(0)', add_python_log=False)
+    step_result.presentation.logs.setdefault('result', []).append(
+        json.dumps(results, indent=2))

[iannucci, 2015/11/18 03:57:54]

I would consider summarizing the culprit (if we found one) on the step_text too.

[stgao, 2015/11/19 22:06:34]

Good idea.
Done.

+
+  return results
+
+
+def GenTests(api):
+  def props(compile_targets=None):
+    properties = {
+        'mastername': 'tryserver.chromium.linux',
+        'buildername': 'linux_variable',
+        'slavename': 'build1-a1',
+        'buildnumber': '1',
+        'target_mastername': 'chromium.linux',
+        'target_buildername': 'Linux Builder',
+        'target_solution': 'src',
+        'target_solution_revisions': ['r1'],
+    }
+    if compile_targets:
+      properties['compile_targets'] = compile_targets
+    return api.properties(**properties)
+
+  yield (
+      api.test('compile_skipped') +
+      props() +
+      api.override_step_data(
+          'test r1.analyze',
+          api.json.output({
+              'build_targets': [],
+              'status': 'No dependencies',
+              'targets': [],
+          })
+      )
+  )
+
+  yield (
+      api.test('compile_targets_returned_by_analyze') +
+      props() +
+      api.override_step_data(
+          'test r1.analyze',
+          api.json.output({
+              'build_targets': ['test_target'],
+              'status': 'Found dependency',
+              'targets': ['test_target_run'],
+          })
+      )
+  )
+
+  yield (
+      api.test('compile_specified_targets') +
+      props(compile_targets=['obj/a/b/c.o'])
+  )
+
+  yield (
+      api.test('compile_failed') +
+      props(compile_targets=['obj/a/b/c.o']) +
+      api.override_step_data('test r1.compile', retcode=1)
+  )
+
+  yield (
+      api.test('compile_failed_infra') +
+      props(compile_targets=['obj/a/b/c.o']) +
+      api.override_step_data(
+          'test r1.compile',
+          api.json.output({
+              'notice': [
+                  {
+                      'infra_status': {
+                          'ping_status_code': 408,
+                      },
+                  },
+              ],
+          }),
+          retcode=1)
+  )