Adds ability for builders to only compile targets affected by change

scripts/slave/recipes/chromium_trybot.py

Index: scripts/slave/recipes/chromium_trybot.py
diff --git a/scripts/slave/recipes/chromium_trybot.py b/scripts/slave/recipes/chromium_trybot.py
index 092b8b964e983a7bf00543d352b6fb34b234bb91..b33661d000040d7d51e79310fe1e3b520058fde6 100644
--- a/scripts/slave/recipes/chromium_trybot.py
+++ b/scripts/slave/recipes/chromium_trybot.py
@@ -490,6 +490,19 @@ def get_analyze_config(api, file_name):
   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',
@@ -656,6 +669,11 @@ def GenSteps(api):
         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', {})
 
@@ -669,14 +687,21 @@ def GenSteps(api):
       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.
@@ -706,12 +731,16 @@ def GenSteps(api):
     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
@@ -719,13 +748,8 @@ def GenSteps(api):
     #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
@@ -769,6 +793,8 @@ def GenSteps(api):
       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)')
@@ -1118,7 +1144,8 @@ def GenTests(api):
     ) +
     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
@@ -1146,7 +1173,8 @@ def GenTests(api):
     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
@@ -1174,5 +1202,64 @@ def GenTests(api):
     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']}))
+  )
+