Adds ability for builders to only compile targets affected by change

scripts/slave/recipes/chromium_trybot.py

 # Copyright 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.
 
 import re
 
 DEPS = [
   'bot_update',
   'chromium',
   'chromium_tests',
@@ skipping 472 matching lines @@
   config_path = api.path.join('testing', 'buildbot', file_name)
   step_result = api.json.read(
     'read analyze test spec',
     api.path['checkout'].join(config_path),
     step_test_data=lambda: api.json.test_api.output({'exclusions': []})
     )
   step_result.presentation.step_text = 'path: %r' % config_path
   return step_result.json.output
 
 
+def append_test_if_in_compile_targets(test, tests, compile_targets):
+  """Adds |test| to |tests| if at least one of the test's compile target is in
+  |compile_targets|."""
+  # The target all builds everything.
+  if 'all' in compile_targets:
+    tests.append(test)
+    return

[iannucci, 2014/08/20 17:54:40]

I would generally recommend returning something (probably a list of tests) and
then extend the list in the caller, rather than passing a list as a mutate-able
entity to the function. e.g.

tests.extend(tests_in_compile_targets(test, compile_targets))

[sky, 2014/08/20 20:26:33]

Done.

+  for target in test.compile_targets(None):
+    if target in compile_targets:
+      tests.append(test)
+      return
+
+
 add_swarming_builder('linux_chromium_rel', 'linux_chromium_rel_swarming',
                      'tryserver.chromium.linux')
 add_swarming_builder('linux_chromium_chromeos_rel',
                      'linux_chromium_chromeos_rel_swarming',
                      'tryserver.chromium.linux')
 add_swarming_builder('win_chromium_rel', 'win_chromium_rel_swarming',
                      'tryserver.chromium.win')
 add_swarming_builder('win_chromium_x64_rel', 'win_chromium_x64_rel_swarming',
                      'tryserver.chromium.win')
 add_swarming_builder('mac_chromium_rel', 'mac_chromium_rel_swarming',
@@ skipping 146 matching lines @@
       if 'exclude_builders' in test:
         if '%s:%s' % (mastername, buildername) in test['exclude_builders']:
           return False
       return True
 
     # Parse test spec file into list of Test instances.
     compile_targets, gtest_tests, swarming_tests = parse_test_spec(
         test_spec,
         bot_config.get('enable_swarming'),
         should_use_test)
+    compile_targets.extend(bot_config.get('compile_targets', []))
+    # TODO(phajdan.jr): Also compile 'all' on win, http://crbug.com/368831 .
+    # Disabled for now because it takes too long and/or fails on Windows.
+    if not api.platform.is_win and not bot_config.get('exclude_compile_all'):

[iannucci, 2014/08/20 17:54:40]

TODO: the bot_config stuff has gotten excessively complex with lots of
hard-coded strings in lots of places. It needs to be documented and/or
symbolized.

[sky, 2014/08/20 20:26:33]

No argument there.

+      compile_targets = ['all'] + compile_targets
 
     runhooks_env = bot_config.get('runhooks_env', {})
 
     # See if the patch needs to compile on the current platform.
     if isinstance(test_spec, dict) and should_filter_builder(
         buildername, test_spec.get('non_filter_builders', []),
         api.properties.get('root')):
       analyze_config = get_analyze_config(
           api, bot_config['testing'].get('analyze_config_file',
                                          'trybot_analyze_config.json'))
       api.filter.does_patch_require_compile(
           exclusions=analyze_config.get('exclusions', []),
           exes=get_test_names(gtest_tests, swarming_tests),
+          compile_targets=compile_targets,
           env=runhooks_env)
       if not api.filter.result:
         return [], swarming_tests, bot_update_step
-      # Patch needs compile. Filter the list of test targets.
+      # Patch needs compile. Filter the list of test and compile targets.
       if should_filter_tests(buildername,
                              test_spec.get('non_filter_tests_builders', [])):
         gtest_tests = filter_tests(gtest_tests, api.filter.matching_exes)
         swarming_tests = filter_tests(swarming_tests, api.filter.matching_exes)
+      if buildername in test_spec.get('filter_compile_builders', []):
+        if 'all' in compile_targets:

[iannucci, 2014/08/20 17:54:40]

TBH, I would rather 'magic' compile targets like 'all' be prefixed with
something (like '$all', or '!all') to indicate their specialness at a glance.

[sky, 2014/08/20 20:26:33]

The specialness is handled by the build system. Are you suggesting we some how
preprocess these?

+          compile_targets = api.filter.compile_targets
+        else:
+          compile_targets = list(set(compile_targets) &
+                                 set(api.filter.compile_targets))
 
     # Swarming uses Isolate to transfer files to swarming bots.
     # set_isolate_environment modifies GYP_DEFINES to enable test isolation.
     if bot_config.get('use_isolate') or swarming_tests:
       api.isolate.set_isolate_environment(api.chromium.c)
 
     # If going to use swarming_client (pinned in src/DEPS), ensure it is
     # compatible with what recipes expect.
     if swarming_tests:
       api.swarming.check_client_version()
       # Decide the task priority.
       api.swarming.task_priority = build_to_priority(api.properties)
 
     api.chromium.runhooks(env=runhooks_env)
 
     tests = []
     # TODO(phajdan.jr): Re-enable checkdeps on Windows when it works with git.
     if not api.platform.is_win:
       tests.append(api.chromium.steps.CheckdepsTest())
     if api.platform.is_linux:
       tests.extend([
           api.chromium.steps.CheckpermsTest(),
           api.chromium.steps.ChecklicensesTest(),
       ])
     tests.append(api.chromium.steps.Deps2GitTest())
 
     if (bot_config['chromium_config'] not in ['chromium_chromeos',
                                              'chromium_chromeos_clang']

[iannucci, 2014/08/20 17:54:40]

this should be

if bot_config.get('do_telemetry_tests'):  # or some equally obvious string
  # add telemetry tests

What's the point of a config with orthogonal properties if you conflate property
meanings? :(

In fact, on the 'magic values' note, could this just be '$telemetry_tests' in
the test list or something? (and similarly for every explicitly named block of
tests in this function)

[sky, 2014/08/20 20:26:33]

Agreed. I didn't add these. I'm happy to change in a followup patch. How does
that sound?

         and not buildername.startswith('win8')):

[iannucci, 2014/08/20 17:54:40]

This is exactly the sort of logic that configs should prevent...

-      tests.append(api.chromium.steps.TelemetryUnitTests())
-      tests.append(api.chromium.steps.TelemetryPerfUnitTests())
+      append_test_if_in_compile_targets(api.chromium.steps.TelemetryUnitTests(),
+                                        tests, compile_targets)
+      append_test_if_in_compile_targets(
+          api.chromium.steps.TelemetryPerfUnitTests(), tests, compile_targets)
 
     tests.extend(gtest_tests)
     tests.extend(swarming_tests)
-    tests.append(api.chromium.steps.NaclIntegrationTest())
+    append_test_if_in_compile_targets(api.chromium.steps.NaclIntegrationTest(),
+                                      tests, compile_targets)
+    # MojoPythonTests don't require anything to be compiled.
     tests.append(api.chromium.steps.MojoPythonTests())
 
     # test_installer only works on 32-bit builds; http://crbug.com/399643
     # Disabled due to retry issues (http://crbug.com/402081.
     #if api.platform.is_win and api.chromium.c.TARGET_BITS == 32:
     #  tests.append(api.chromium.steps.MiniInstallerTest())
 
-    compile_targets.extend(bot_config.get('compile_targets', []))
     compile_targets.extend(api.itertools.chain(
         *[t.compile_targets(api) for t in tests]))
-    # TODO(phajdan.jr): Also compile 'all' on win, http://crbug.com/368831 .
-    # Disabled for now because it takes too long and/or fails on Windows.
-    if not api.platform.is_win and not bot_config.get('exclude_compile_all'):
-      compile_targets = ['all'] + compile_targets
     api.chromium.compile(compile_targets, name='compile (with patch)')
 
     # Collect *.isolated hashes for all isolated targets, used when triggering
     # tests on swarming.
     if bot_config.get('use_isolate') or swarming_tests:
       api.isolate.find_isolated_tests(api.chromium.output_dir)
 
     if bot_config['compile_only']:
       tests = []
       swarming_tests = []
@@ skipping 23 matching lines @@
         bot_update_json['properties']['got_revision'])
     api.bot_update.ensure_checkout(force=True,
                                    patch=False,
                                    update_presentation=False)
     try:
       api.chromium.runhooks()
     finally:
       compile_targets = list(api.itertools.chain(
           *[t.compile_targets(api) for t in failing_tests]))
       if compile_targets:
+        # Remove duplicate targets.
+        compile_targets = list(set(compile_targets))
         try:
           api.chromium.compile(
                   compile_targets, name='compile (without patch)')
         except api.step.StepFailure:
           api.chromium.compile(compile_targets,
                                name='compile (without patch, clobber)',
                                force_clobber=True)
         # Search for *.isolated only if enabled in bot config or if some
         # swarming test is being recompiled.
         bot_config = get_bot_config(mastername, buildername)
@@ skipping 329 matching lines @@
   # analyze result returning true. This should result in a compile.
   yield (
     api.test('compile_because_of_analyze') +
     props(buildername='linux_chromium_rel') +
     api.platform.name('linux') +
     api.override_step_data('read test spec', api.json.output({
       })
     ) +
     api.override_step_data(
       'analyze',
-      api.json.output({'status': 'Found dependency', 'targets': []}))
+      api.json.output({'status': 'Found dependency', 'targets': [],
+                       'build_targets': []}))
   )
 
   # Tests analyze module by way of specifying non_filter_builders and
   # analyze result returning true along with a smaller set of tests.
   yield (
     api.test('compile_because_of_analyze_with_filtered_tests_no_builder') +
     props(buildername='linux_chromium_rel') +
     api.platform.name('linux') +
     api.override_step_data('read test spec', api.json.output({
         'non_filter_tests_builders': ['linux_chromium_rel'],
         'gtest_tests': [
           {
             'test': 'base_unittests',
             'swarming': {'can_use_on_swarming_builders': True},
           },
           {
             'test': 'browser_tests',
           },
           {
             'test': 'unittests',
           },
         ],
       })
     ) +
     api.override_step_data(
       'analyze',
       api.json.output({'status': 'Found dependency',
-                       'targets': ['browser_tests', 'base_unittests']}))
+                       'targets': ['browser_tests', 'base_unittests'],
+                       'build_targets': ['browser_tests', 'base_unittests']}))
   )
 
   # Tests analyze module by way of not specifying non_filter_builders and
   # analyze result returning true along with a smaller set of tests. This
   # specifices a 'filter_test_builder', so that this bot uses the filtered set.
   yield (
     api.test('compile_because_of_analyze_with_filtered_tests') +
     props(buildername='linux_chromium_rel') +
     api.platform.name('linux') +
     api.override_step_data('read test spec', api.json.output({
         'gtest_tests': [
           {
             'test': 'base_unittests',
             'swarming': {'can_use_on_swarming_builders': True},
           },
           {
             'test': 'browser_tests',
           },
           {
             'test': 'unittests',
           },
         ],
       })
     ) +
     api.override_step_data(
       'analyze',
       api.json.output({'status': 'Found dependency',
-                       'targets': ['browser_tests', 'base_unittests']}))
+                       'targets': ['browser_tests', 'base_unittests'],
+                       'build_targets': ['browser_tests', 'base_unittests']}))
   )
+
+  # Tests compile_target portion of analyze module.
+  yield (
+    api.test('compile_because_of_analyze_with_filtered_compile_targets') +
+    props(buildername='linux_chromium_rel') +
+    api.platform.name('linux') +
+    api.override_step_data('read test spec', api.json.output({
+        'filter_compile_builders': 'linux_chromium_rel',
+        'gtest_tests': [
+          {
+            'test': 'base_unittests',
+            'swarming': {'can_use_on_swarming_builders': True},
+          },
+          {
+            'test': 'browser_tests',
+          },
+          {
+            'test': 'unittests',
+          },
+        ],
+      })
+    ) +
+    api.override_step_data(
+      'analyze',
+      api.json.output({'status': 'Found dependency',
+                       'targets': ['browser_tests', 'base_unittests'],
+                       'build_targets': ['chrome', 'browser_tests',
+                                         'base_unittests']}))
+  )
+
+  # Tests compile_targets portion of analyze with a bot that doesn't include the
+  # 'all' target.
+  yield (
+    api.test(
+      'compile_because_of_analyze_with_filtered_compile_targets_exclude_all') +
+    props(buildername='linux_browser_asan') +
+    api.platform.name('linux') +
+    api.override_step_data('read test spec', api.json.output({
+        'compile_targets': ['base_unittests'],
+        'gtest_tests': [
+          {
+            'test': 'browser_tests',
+            'args': '--gtest-filter: *NaCl*',
+          }, {
+            'test': 'base_tests',
+            'args': ['--gtest-filter: *NaCl*'],
+          },
+        ],
+        'filter_compile_builders': 'linux_browser_asan',
+      })
+    ) +
+    api.override_step_data(
+      'analyze',
+      api.json.output({'status': 'Found dependency',
+                       'targets': ['browser_tests', 'base_unittests'],
+                       'build_targets': ['base_unittests']}))
+  )
+