Archive Linux perf builds for manual bisect

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 collections
 import contextlib
 import copy
 import itertools
 import json
 
@@ skipping 388 matching lines @@
             verbose=True,
             set_swarm_hashes=False)
 
   def archive_build(self, mastername, buildername, update_step, bot_db):
     bot_config = bot_db.get_bot_config(mastername, buildername)
 
     if bot_config.get('bot_type') == 'builder':
       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,
             build_url=self._build_gs_archive_url(
                 mastername, master_config, buildername),
             build_revision=build_revision,
             cros_board=self.m.chromium.c.TARGET_CROS_BOARD,
             # TODO(machenbach): Make asan a configuration switch.
             package_dsym_files=(
                 self.m.chromium.c.gyp_env.GYP_DEFINES.get('asan') and
@@ skipping 336 matching lines @@
       self.m.swarming.default_user = requester
 
       patch_project = self.m.properties.get('patch_project')
       if patch_project:
         self.m.swarming.add_default_tag('patch_project:%s' % patch_project)
     else:
       self.m.swarming.default_priority = MASTER_SWARMING_PRIORITIES[mastername]
       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
     URL, but some builders on some masters may specify custom places to upload
     builds to. These special cases include:
       'chromium.perf' or 'chromium.perf.fyi':
         Exclude the name of the master from the url.
       'tryserver.chromium.perf', or
           linux_full_bisect_builder on 'tryserver.chromium.linux':
         Return None so that the archive url specified in factory_properties
         (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)
     elif (mastername == 'tryserver.chromium.perf' or
           (mastername == 'tryserver.chromium.linux' and
            buildername == 'linux_full_bisect_builder')):
       return None
     else:
       return self.m.archive.legacy_upload_url(
           master_config.get('build_gs_bucket'),
@@ skipping 40 matching lines @@
   def get_compile_targets_for_scripts(self):
     return self.m.python(
         name='get compile targets for scripts',
         script=self.m.path['checkout'].join(
             'testing', 'scripts', 'get_compile_targets.py'),
         args=[
             '--output', self.m.json.output(),
             '--',
         ] + self.get_common_args_for_scripts(),
         step_test_data=lambda: self.m.json.test_api.output({}))