Pass mb_mastername and mb_buildername as parameters to override the default.

scripts/slave/recipe_modules/chromium_tests/api.py

 # Copyright 2014 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 ast
 import contextlib
 import copy
 import itertools
 import json
 
@@ skipping 339 matching lines @@
         command(lambda name: '%s (with patch)' % name)
       except self.m.step.StepFailure:
         self.deapply_patch(update_step)
         command(lambda name: '%s (without patch)' % name)
         raise
     else:
       command(lambda name: name)
 
 
   def compile(self, mastername, buildername, update_step, master_dict,
-              test_spec):
+              test_spec, mb_mastername, mb_buildername):

[Paweł Hajdan Jr., 2015/11/27 10:21:24]

Can we make mb_ args optional and have them default to non-mb ones? Applies to
entire CL.

[stgao, 2015/11/28 07:37:48]

I think we can not do that, assuming that "non-mb ones" refer to the other two
parameters `mastername` and `buildername` here. (I might be wrong, so please let
me know why we could default to those two.)

Reason: the original code in function `run_mb_and_compile` explicitly said not
to use the mastername and buildername passed in (that code was refactored from
function `compile_specific_targets` at
https://chromium.googlesource.com/chromium/tools/build/+blame/b5a369456d211dc...).


However, we could default to api.properties['mastername'] and
api.properties['buildername'] instead within function`compile_specific_targets`,
if that is preferred.
What do you think?

[Paweł Hajdan Jr., 2015/11/30 12:47:25]

Tentatively sounds good. Maybe that was a bit vague, but I did mean to default
to current behavior.

     """Runs compile and related steps for given builder."""
     compile_targets, tests_including_triggered = \
         self.get_compile_targets_and_tests(
             mastername,
             buildername,
             master_dict, test_spec)
     self.compile_specific_targets(
         mastername, buildername, update_step, master_dict,
-        compile_targets, tests_including_triggered)
+        compile_targets, tests_including_triggered,
+        mb_mastername, mb_buildername)
 
   def compile_specific_targets(
       self, mastername, buildername, update_step, master_dict,
-      compile_targets, tests_including_triggered, override_bot_type=None):
+      compile_targets, tests_including_triggered,
+      mb_mastername, mb_buildername, override_bot_type=None):
     """Runs compile and related steps for given builder.
 
+    We don't use the given `mastername` and `buildername` to run MB, because
+    they may be the values of the continuous builder the trybot may be
+    configured to match; instead we need to use the actual mastername and
+    buildername we're running on(given as `mb_mastername` and `mb_buildername`),
+    because it may be configured with different MB settings.
+
+    However, recipes used by Findit for culprit finding may still set
+    (mb_mastername, mb_buildername) = (mastername, buildername) to exactly match
+    a given continuous builder.
+
     Allows finer-grained control about exact compile targets used."""
 
     bot_config = master_dict.get('builders', {}).get(buildername)
     master_config = master_dict.get('settings', {})
     bot_type = override_bot_type or bot_config.get('bot_type', 'builder_tester')
 
     self.m.chromium.cleanup_temp()
     if self.m.chromium.c.TARGET_PLATFORM == 'android':
       self.m.chromium_android.clean_local_files()
       self.m.chromium_android.run_tree_truth()
 
     if bot_type in ['builder', 'builder_tester']:
       isolated_targets = [
         t.isolate_target for t in tests_including_triggered if t.uses_swarming
       ]
 
       if isolated_targets:
         self.m.isolate.clean_isolated_files(self.m.chromium.output_dir)
 
       if self.m.chromium.c.project_generator.tool == 'mb':
         if bot_config.get('chromium_config') == 'chromium_win_clang':
           self.m.chromium.update_clang()
 
       try:
         self.transient_check(update_step, lambda transform_name:
             self.run_mb_and_compile(compile_targets, isolated_targets,
-                                    name_suffix=transform_name('')))
+                                    name_suffix=transform_name(''),
+                                    mb_mastername=mb_mastername,
+                                    mb_buildername=mb_buildername))
       except self.m.step.StepFailure:
         self.m.tryserver.set_compile_failure_tryjob_result()
         raise
 
       if isolated_targets:
         self.m.isolate.remove_build_metadata()
         # 'compile' just prepares all information needed for the isolation,
         # and the isolation is a separate step.
         self.m.isolate.isolate_tests(
             self.m.chromium.output_dir,
@@ skipping 47 matching lines @@
           })
 
     if bot_config.get('archive_build') and not self.m.tryserver.is_tryserver:
       self.m.chromium.archive_build(
           'archive_build',
           bot_config['gs_bucket'],
           bot_config.get('gs_acl'),
           mode='dev'
       )
 
-  def run_mb_and_compile(self, compile_targets, isolated_targets, name_suffix):
+  def run_mb_and_compile(self, compile_targets, isolated_targets, name_suffix,
+                         mb_mastername, mb_buildername):
     if self.m.chromium.c.project_generator.tool == 'mb':
-      # We don't use the mastername and buildername passed in, because
-      # those may be the values of the continuous builder the trybot may
-      # be configured to match; we need to use the actual mastername
-      # and buildername we're running on, because it may be configured
-      # with different MB settings.
-      real_mastername = self.m.properties['mastername']
-      real_buildername = self.m.properties['buildername']
-      self.m.chromium.run_mb(real_mastername, real_buildername,
+      self.m.chromium.run_mb(mb_mastername, mb_buildername,
                              isolated_targets=isolated_targets,
                              name='generate_build_files%s' % name_suffix)
 
     self.m.chromium.compile(compile_targets, name='compile%s' % name_suffix)
 
   def tests_for_builder(self, mastername, buildername, update_step, master_dict,
                         override_bot_type=None):
     got_revision = update_step.presentation.properties['got_revision']
 
     bot_config = master_dict.get('builders', {}).get(buildername)
@@ skipping 101 matching lines @@
     # mapping for their own purposes.
     first_solution_name = self.m.gclient.c.solutions[0].name
     rev_property = self.m.gclient.c.got_revision_mapping[first_solution_name]
     self.m.gclient.c.revisions[first_solution_name] = str(
         bot_update_json['properties'][rev_property])
     self.m.bot_update.ensure_checkout(
         force=True, patch=False, update_presentation=False)
     self.m.chromium.runhooks(name='runhooks (without patch)')
 
   def run_tests_on_tryserver(self, mastername, api, tests, bot_update_step,
-                             affected_files):
+                             affected_files, mb_mastername, mb_buildername):
     def deapply_patch_fn(failing_tests):
       self.deapply_patch(bot_update_step)
       compile_targets = list(itertools.chain(
           *[t.compile_targets(api) for t in failing_tests]))
       if compile_targets:
         # Remove duplicate targets.
         compile_targets = sorted(set(compile_targets))
         failing_swarming_tests = [
             t.isolate_target for t in failing_tests if t.uses_swarming]
         if failing_swarming_tests:
           self.m.isolate.clean_isolated_files(self.m.chromium.output_dir)
         self.run_mb_and_compile(compile_targets, failing_swarming_tests,
-                                ' (without patch)')
+                                ' (without patch)',
+                                mb_mastername, mb_buildername)
         if failing_swarming_tests:
           self.m.isolate.isolate_tests(self.m.chromium.output_dir,
                                        verbose=True)
 
     deapply_patch = True
     for path in RECIPE_CONFIG_PATHS:
       if any([f.startswith(path) for f in affected_files]):
         deapply_patch = False
         break
 
     with self.wrap_chromium_tests(mastername, tests):
       if deapply_patch:
         self.m.test_utils.determine_new_failures(api, tests, deapply_patch_fn)
       else:
         failing_tests = self.m.test_utils.run_tests_with_patch(api, tests)
         if failing_tests:
           self.m.python.failing_step('test results', 'TESTS FAILED')
 
   def analyze(self, affected_files, test_targets, additional_compile_targets,
-              config_file_name, additional_names=None):
+              config_file_name, mb_mastername, mb_buildername,
+              additional_names=None):
     """Runs "analyze" step to determine targets affected by the patch.
 
     Returns a tuple of:
       - list of targets that are needed to run tests (see filter recipe module)
       - list of targets that need to be compiled (see filter recipe module)"""
 
     if additional_names is None:
       additional_names = ['chromium']
 
     use_mb = (self.m.chromium.c.project_generator.tool == 'mb')
     build_output_dir = '//out/%s' % self.m.chromium.c.build_config_fs
     self.m.filter.does_patch_require_compile(
         affected_files,
+        mb_mastername,
+        mb_buildername,
         test_targets=test_targets,
         additional_compile_targets=additional_compile_targets,
         additional_names=additional_names,
         config_file_name=config_file_name,
         use_mb=use_mb,
         build_output_dir=build_output_dir,
         cros_board=self.m.chromium.c.TARGET_CROS_BOARD)
 
     compile_targets = self.m.filter.compile_targets[:]
 
@@ skipping 156 matching lines @@
       result_text = 'MB is enabled for this builder at this revision.'
       log_name = 'Builder MB-ready'
       self.m.step.active_result.presentation.logs[log_name] = [result_text]
       return False
     except (self.m.step.StepFailure, KeyError):
       result_text = 'MB is not enabled for this builder at this revision.'
       log_name = 'Builder NOT MB-ready'
       self.m.step.active_result.presentation.logs[log_name] = [result_text]
       self.m.step.active_result.presentation.status = self.m.step.WARNING
       return True