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(

[Paweł Hajdan Jr., 2015/11/24 13:20:41]

Why is this needed?

The mastername and buildername should identify the solution, right?

[stgao, 2015/11/24 23:19:20]

This is very helpful, and it could save me one parameter.

But how could I identify the solution for waterfall continuous builder specified
by the given target_mastername and target_buildername? Eg: how to derive "src"
from ("chromium.linux", "Linux Builder")?

+        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.'),
+    'requested_compile_targets': Property(
+        kind=List(basestring), default=None, param_name='compile_targets',
+        help='The failed compile targets, eg: browser_tests'),
+}
+
+
+class CompileResult(object):
+  SKIPPED = 'skipped'  # No compile is needed.
+  PASSED = 'passed'  # Compile passed.
+  FAILED = 'failed'  # Compile failed.
+
+
+def _run_compile_at_revision(api, target_mastername, target_buildername,
+                             target_solution, revision,
+                             compile_targets):
+  with api.step.nest('test %s' % str(revision)):
+    # Configure the revision to recompile.
+    api.gclient.c.revisions[target_solution] = revision
+
+    bot_update_step, master_dict, test_spec = \
+        api.chromium_tests.prepare_checkout(
+            target_mastername,
+            target_buildername)
+
+    # If no compile targets are provided, use analyze to filter out the

[Paweł Hajdan Jr., 2015/11/24 13:20:41]

Why do you want to support the case where no compile targets are provided?

[stgao, 2015/11/24 23:19:20]

For compile failures on the waterfall, now there is no reliable way for a
program to tell which targets failed to compile. Thus, no compile targets is the
common case.

For compile and link failures, we are now trying to use regex to extract the
failed targets. However, this approach is impossible for failures of ACTIONs.

+    # impacted compile targets by the given revision.
+    # TODO: if compile targets are provided, check if they exist and use analyze
+    # to filter out the impacted ones by the given revision.
+    compile_targets = sorted(set(compile_targets or []))
+    if not compile_targets:
+      affected_files = api.findit.get_files_affected_by_revision(
+          revision, target_solution)
+
+      test_targets, compile_targets, _ = \
+          api.chromium_tests.get_test_targets_compile_targets_and_tests(

[Dirk Pranke, 2015/11/24 00:17:41]

why are you splitting test_targets and compile_targets apart, 
only to pass them back in to analyze?

in theory the build targets listed as 'additional_compile_targets'
are treated differently from the build targets listed as 
'test_targets' in the call to analyze, but in practice for your 
purposes, they'll mostly be the same.

So I'm not sure why you couldn't use the older version of 
get_compile_targets_and_tests() as-is?

If we really do need to change things, I think that whole 
method is getting crufty and confusing and we probably 
need to scrap it completely; it's trying to do too many 
things. I would be more inclined to have it just return 
the compile targets for the tests, and move the 
'compile_targets' logic out of the function.

[stgao, 2015/11/24 06:15:29]

Yes, to match the changed API in chromium_tests.analyze.
I made the change, because I thought the following code doesn't match the api
description in filter.does_patch_require_compile.
IIUC, this is actually what you suggested, so I will go with it.

Before I change it back, I'd like to double check: to get the impacted compile
targets by the affected files, the code in this comment and the code in this
patchset are equivalent?

# `all_compile_targets` includes those specified as
# "additional_compile_targets" in the test spec.
all_compile_targets, _ = api.chromium_tests.get_compile_targets_and_tests(...) 
_, compile_targets = api.chromium_tests.analyze(
     affected_files,
     test_targets=[ ],
     additional_compile_targets=all_compile_targets,
     config_file_name='trybot_analyze_config.json')

[stgao, 2015/11/24 23:19:19]

Done.

+            target_mastername,
+            target_buildername,
+            master_dict,
+            test_spec,
+            override_bot_type='builder_tester',
+            override_tests=[])
+      additional_compile_targets = sorted(
+          set(compile_targets) - set(test_targets))
+
+      _, compile_targets = api.chromium_tests.analyze(

[Paweł Hajdan Jr., 2015/11/24 13:20:41]

Is analyze _really_ needed here? Why can't we rely on ninja to recompile targets
that need it?

[stgao, 2015/11/24 23:19:20]

We could do that, but it will be slow, because more targets have to be compiled.
With analyze, we could even skip compile at some commits.

The goal here is to speed up compile so that we could reduce the latency of
culprit finding.

[Paweł Hajdan Jr., 2015/11/30 16:19:05]

What's the reasoning behind this? I'm not convinced. What is the scenario where
a target is not fresh according to ninja, but is not affected by given revision?

[stgao, 2015/12/01 16:30:00]

In this smoke test, build in http://shortn/_d3dv1OGpgs after a clobber with "rm
-rf src/out/Debug", only a few targets were affected by the first tested
revision "6beae8c75e3a0300ced4c95cb0463cfe2f6b7e6e".

As discussed offline, I will move this out to a separate CL. "analyze" is needed
to reduce the number of targets to be compile, before ninja could support
outputting the failed compile targets to a json file or the like yet.

+          affected_files,
+          test_targets,
+          additional_compile_targets,
+          'trybot_analyze_config.json')
+
+      if not compile_targets:
+        # No compile target is impacted by the given revision.
+        return CompileResult.SKIPPED
+
+    assert compile_targets, 'compile targets should not be empty!'

[Dirk Pranke, 2015/11/24 00:17:41]

nit: I'm not convinced this assert is really useful.

[stgao, 2015/11/24 23:19:20]

Done.

+
+    try:
+      api.chromium_tests.compile_specific_targets(
+          target_mastername,
+          target_buildername,
+          bot_update_step,
+          master_dict,
+          compile_targets,
+          [], # Not run any test.

[Dirk Pranke, 2015/11/24 00:17:41]

nit: I'd just say tests_including_triggered=[] instead

[stgao, 2015/11/24 23:19:20]

Done.

+          override_bot_type='builder_tester')

[Paweł Hajdan Jr., 2015/11/24 13:20:41]

Why is this needed?

[stgao, 2015/11/24 23:19:20]

Just want to make it explicit. I could remove it if that is the preferred way.
But it seems chromium_trybot.py does the same like this.

[Paweł Hajdan Jr., 2015/11/30 16:19:05]

That's actually a behavior change. chromium_trybot does this because it merges
builder+tester pairs into single trybots.

What this recipe is doing is just compiling things. Is this because of avoiding
to archive the build artifacts?

[stgao, 2015/12/01 16:30:00]

Oh, I see. I thought it was the same as a "builder" bot type.
Removed all override.
Put up https://codereview.chromium.org/1484293002/ for this purpose instead.

[stgao, 2015/12/01 17:15:02]

As discussion in the meeting, I will add this bot-type override back.

+      return CompileResult.PASSED
+    except api.step.InfraFailure:
+      raise
+    except api.step.StepFailure:
+      return CompileResult.FAILED
+
+
+def RunSteps(api, target_mastername, target_buildername,
+             target_solution, target_solution_revisions,
+             requested_compile_targets):
+  api.chromium_tests.configure_build(
+      target_mastername, target_buildername, override_bot_type='builder_tester')

[Paweł Hajdan Jr., 2015/11/24 13:20:41]

Why is override_bot_type needed here?

[stgao, 2015/11/24 23:19:20]

Just want to make it explicit. I could remove it if that is the preferred way.
But it seems chromium_trybot.py does the same like this.

+
+  results = []
+  try:
+    for current_revision in target_solution_revisions:
+      try:
+        compile_result = _run_compile_at_revision(
+            api, target_mastername, target_buildername, target_solution,
+            current_revision, requested_compile_targets)
+      except api.step.InfraFailure:
+        # Retry once only on infra failure.

[Paweł Hajdan Jr., 2015/11/24 13:20:41]

Is this retry really needed/useful here?

[stgao, 2015/11/24 23:19:19]

For compile failures on the waterfall, I saw a lot of flaky failures in August
and September
(https://docs.google.com/a/google.com/spreadsheets/d/1MxOZtL8JAQo2KW6kAouxvcd-...).
Thus, I added this retry to make the culprit-finding process more reliable.

However, I don't have data for trybots. But as the under-lying infra is the
same, I think the situation might be similar.

[Paweł Hajdan Jr., 2015/11/30 16:19:05]

Can we start with a version of this recipe which doesn't have this logic, and
possibly add it later if needed?

[stgao, 2015/12/01 16:30:00]

OK. Removed.

+        compile_result = _run_compile_at_revision(
+            api, target_mastername, target_buildername, target_solution,
+            current_revision, requested_compile_targets)
+
+      results.append([current_revision, compile_result])
+      if compile_result == CompileResult.FAILED:
+        # TODO: if compile targets are specified, compile may fail because those

[Paweł Hajdan Jr., 2015/11/24 13:20:41]

Please follow the style guide's TODO(...) format.

[stgao, 2015/11/24 23:19:20]

Done.

+        # targets are added in a later revision.
+        break # Found the culprit, no need to check later revisions.
+  finally:
+    # Report the result.
+    step_result = api.python.inline(

[Paweł Hajdan Jr., 2015/11/24 13:20:41]

Why not use api.python.result_step (or succeeding/failing_step)?

[stgao, 2015/11/24 23:19:20]

I'm afraid I can't do that.

Because they don't support adding both step text and step log at the same time.
And as they don't return the step_result either, I can't set either step text or
step log like the code here.

[Paweł Hajdan Jr., 2015/11/30 16:19:05]

Could you at the very least add a TODO, and eventually make the python recipe
module support that?

[stgao, 2015/12/01 16:30:00]

Done.

+        'report', 'import sys; sys.exit(0)', add_python_log=False)
+    if (not requested_compile_targets and
+        results and results[-1][1] == CompileResult.FAILED):
+      step_result.presentation.step_text = '<br/>Culprit: %s' % results[-1][0]
+    step_result.presentation.logs.setdefault('result', []).append(
+        json.dumps(results, indent=2))
+
+  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({
+              'status': 'No dependencies',
+              'compile_targets': [],
+              'test_targets': [],
+          })
+      )
+  )
+
+  yield (
+      api.test('successful_compile_targets_returned_by_analyze') +
+      props() +
+      api.override_step_data(
+          'test r1.analyze',
+          api.json.output({
+              'status': 'Found dependency',
+              'compile_targets': ['test_target', 'test_target_run'],
+              'test_targets': ['test_target', 'test_target_run'],
+          })
+      )
+  )
+
+  yield (
+      api.test('failed_compile_targets_returned_by_analyze') +
+      props() +
+      api.override_step_data(
+          'test r1.analyze',
+          api.json.output({
+              'status': 'Found dependency',
+              'compile_targets': ['test_target', 'test_target_run'],
+              'test_targets': ['test_target', 'test_target_run'],
+          })
+      ) +
+      api.override_step_data('test r1.compile', retcode=1)
+  )
+
+  yield (
+      api.test('compile_specified_targets') +
+      props(compile_targets=['target_name'])
+  )
+
+  yield (
+      api.test('compile_failed') +
+      props(compile_targets=['target_name']) +
+      api.override_step_data('test r1.compile', retcode=1)
+  )
+
+  yield (
+      api.test('successful_retry_compile_upon_infra_failure') +
+      props(compile_targets=['target_name']) +
+      api.override_step_data(
+          'test r1.compile',
+          api.json.output({
+              'notice': [
+                  {
+                      'infra_status': {
+                          'ping_status_code': 408,
+                      },
+                  },
+              ],
+          }),
+          retcode=1) +
+      api.override_step_data(
+          'test r1.compile (2)',
+          api.json.output({
+              'notice': [
+                  {
+                      'infra_status': {
+                          'ping_status_code': 200,
+                      },
+                  },
+              ],
+          }),
+          retcode=1)
+  )
+
+  yield (
+      api.test('failed_retry_compile_upon_infra_failure') +
+      props(compile_targets=['target_name']) +
+      api.override_step_data(
+          'test r1.compile',
+          api.json.output({
+              'notice': [
+                  {
+                      'infra_status': {
+                          'ping_status_code': 408,
+                      },
+                  },
+              ],
+          }),
+          retcode=1) +
+      api.override_step_data(
+          'test r1.compile (2)',
+          api.json.output({
+              'notice': [
+                  {
+                      'infra_status': {
+                          'ping_status_code': 408,
+                      },
+                  },
+              ],
+          }),
+          retcode=1)
+  )