Pass mb_mastername and mb_buildername as parameters to override the default.

scripts/slave/recipe_modules/chromium_tests/api.py

Index: scripts/slave/recipe_modules/chromium_tests/api.py
diff --git a/scripts/slave/recipe_modules/chromium_tests/api.py b/scripts/slave/recipe_modules/chromium_tests/api.py
index d628116a6caf786afa21c018d77b724286d89a12..360ca232525151f4cd4b51b8b9a79c020260e812 100644
--- a/scripts/slave/recipe_modules/chromium_tests/api.py
+++ b/scripts/slave/recipe_modules/chromium_tests/api.py
@@ -357,7 +357,7 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
 
 
   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(
@@ -366,13 +366,25 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
             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)
@@ -399,7 +411,9 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
       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
@@ -467,16 +481,10 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
           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)
 
@@ -598,7 +606,7 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
     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(
@@ -611,7 +619,8 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
         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)
@@ -631,7 +640,8 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
           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:
@@ -645,6 +655,8 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
     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,