Archive Linux perf builds for manual bisect

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 d6de5df167846b3f08b3fcb55e773e8f8aa0b47b..78ef7f82301511757bedec46647579559f2458eb 100644
--- a/scripts/slave/recipe_modules/chromium_tests/api.py
+++ b/scripts/slave/recipe_modules/chromium_tests/api.py
@@ -406,6 +406,24 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
       if not bot_config.get('cf_archive_build'):
         master_config = bot_db.get_master_settings(mastername)
         build_revision = update_step.presentation.properties['got_revision']
+
+        # For archiving 'chromium.perf', the builder also archives a version 
+        # without perf test files for manual bisect. 
+        # (https://bugs.chromium.org/p/chromium/issues/detail?id=604452)
+        if (mastername.startswith('chromium.perf') and
+            buildername == 'Linux Builder'):

[dtu, 2016/08/05 21:06:19]

This condition could be brittle -- prefer not to do things based on the name of
the builder. Maybe add an option to the master_config that enables this
behavior?

[miimnk, 2016/08/08 22:41:26]

Done.
Added an option in master_config. 

+          self.m.archive.zip_and_upload_build(
+              'package build for bisect',
+              self.m.chromium.c.build_config_fs,
+              build_url=self._build_gs_archive_url(
+                  mastername, master_config, buildername, strip_symbol = True),

[dtu, 2016/08/05 21:06:19]

style nit: no space around =

style nit: this line is very long. it may be more readable to assign the result
of self._build_gs_archive_url) to a variable beforehand.

[miimnk, 2016/08/08 22:41:26]

Done.

+              build_revision=build_revision,
+              cros_board=self.m.chromium.c.TARGET_CROS_BOARD,
+              update_properties=update_step.presentation.properties,
+              includePerfTestFiles=False,
+              store_by_hash=False
+          )
+
         self.m.archive.zip_and_upload_build(
             'package build',
             self.m.chromium.c.build_config_fs,
@@ -762,7 +780,8 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
       self.m.swarming.add_default_tag('purpose:post-commit')
       self.m.swarming.add_default_tag('purpose:CI')
 
-  def _build_gs_archive_url(self, mastername, master_config, buildername):
+  def _build_gs_archive_url(self, mastername, master_config, buildername,
+       strip_symbol = False):

[dtu, 2016/08/05 21:06:19]

style nit: no space around =

[miimnk, 2016/08/08 22:41:27]

Done.

     """Returns the archive URL to pass to self.m.archive.zip_and_upload_build.
 
     Most builders on most masters use a standard format for the build archive
@@ -776,6 +795,10 @@ class ChromiumTestsApi(recipe_api.RecipeApi):
         (as set on the master's configuration) is used instead.
     """
     if mastername.startswith('chromium.perf'):
+      if strip_symbol:

[dtu, 2016/08/05 21:06:19]

This function has completely separate code paths when this argument is True vs
when it's False. Maybe it doesn't belong in this function at all? On the other
hand, maybe this function provides an abstraction layer that shouldn't be
violated? Up to you.

[miimnk, 2016/08/08 22:41:27]

I see that the part of the code is indeed unrelated to the function. I made a
new function with that code. 

+        return self.m.archive.legacy_upload_url(
+          master_config.get('bisect_build_gs_bucket'),
+          extra_url_components=master_config.get('bisect_build_gs_extra'))
       return self.m.archive.legacy_upload_url(
           master_config.get('build_gs_bucket'),
           extra_url_components=None)