diagnose_apk_bloat.py: handle more rev options.

tools/binary_size/diagnose_apk_bloat.py

 #!/usr/bin/env python
 # Copyright 2017 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.
 
 """Tool for finding the cause of APK bloat.
 
 Run diagnose_apk_bloat.py -h for detailed usage help.
 """
 
 import argparse
 import collections
 import distutils.spawn
 import itertools
 import json
 import multiprocessing
 import os
 import shutil
 import subprocess
 import sys
 import tempfile
 import zipfile
 
+_ALLOWED_CONSECUTIVE_BUILDS = 15

[agrieve, 2017/04/21 16:57:31]

Limit seems a bit small. I've seen rolls go in that have 100s of commits. I
think the main important thing is just to have a clear log message saying how
many commits will be built, and maybe a "hit enter to continue") when the number
of commits is large.

[estevenson, 2017/04/21 20:15:49]

Done.

+_ALLOWED_CONSECUTIVE_FAILURES = 2
 _BUILDER_URL = \
     'https://build.chromium.org/p/chromium.perf/builders/Android%20Builder'
 _CLOUD_OUT_DIR = os.path.join('out', 'Release')
 _SRC_ROOT = os.path.abspath(
     os.path.join(os.path.dirname(__file__), os.pardir, os.pardir))
 _DEFAULT_ARCHIVE_DIR = os.path.join(_SRC_ROOT, 'binary-size-bloat')
 _DEFAULT_OUT_DIR = os.path.join(_SRC_ROOT, 'out', 'diagnose-apk-bloat')
 _DEFAULT_TARGET = 'monochrome_public_apk'
 
 # Global variable for storing the initial branch before the script was launched
 # so that it doesn't need to be passed everywhere in case we fail and exit.
 _initial_branch = None
 
+# Global variable for storing the subrepo directory.
+_subrepo = None

[agrieve, 2017/04/21 16:57:31]

Having the global makes the code a bit harder to follow... How about instead
have a single global called _restore_checkout_func, that gets set to a lambda at
the start and will do the restoring when called.

+
 
 class BaseDiff(object):
   """Base class capturing binary size diffs."""
   def __init__(self, name):
     self.name = name
     self.banner = '\n' + '*' * 30 + name + '*' * 30
-    self.RunDiff()
 
   def AppendResults(self, logfile):
     """Print and write diff results to an open |logfile|."""
     _PrintAndWriteToFile(logfile, self.banner)
     _PrintAndWriteToFile(logfile, 'Summary:')
     _PrintAndWriteToFile(logfile, self.Summary())
     _PrintAndWriteToFile(logfile, '\nDetails:')
     for l in self.DetailedResults():
       _PrintAndWriteToFile(logfile, l)
 
   def Summary(self):
     """A short description that summarizes the source of binary size bloat."""
     raise NotImplementedError()
 
   def DetailedResults(self):
     """An iterable description of the cause of binary size bloat."""
     raise NotImplementedError()
 
-  def ProduceDiff(self):
+  def ProduceDiff(self, archive_dirs):
     """Prepare a binary size diff with ready to print results."""
     raise NotImplementedError()
 
-  def RunDiff(self):
+  def RunDiff(self, logfile, archive_dirs):
     _Print('Creating {}', self.name)
-    self.ProduceDiff()
+    self.ProduceDiff(archive_dirs)
+    self.AppendResults(logfile)
 
 
 _ResourceSizesDiffResult = collections.namedtuple(
     'ResourceSizesDiffResult', ['section', 'value', 'units'])
 
 
 class ResourceSizesDiff(BaseDiff):
   _RESOURCE_SIZES_PATH = os.path.join(
       _SRC_ROOT, 'build', 'android', 'resource_sizes.py')
 
-  def __init__(self, archive_dirs, apk_name, slow_options=False):
-    self._archive_dirs = archive_dirs
+  def __init__(self, apk_name, slow_options=False):
     self._apk_name = apk_name
     self._slow_options = slow_options
     self._diff = None  # Set by |ProduceDiff()|
     super(ResourceSizesDiff, self).__init__('Resource Sizes Diff')
 
   def DetailedResults(self):
     for section, value, units in self._diff:
       yield '{:>+10,} {} {}'.format(value, units, section)
 
   def Summary(self):
     for s in self._diff:
       if 'normalized' in s.section:
         return 'Normalized APK size: {:+,} {}'.format(s.value, s.units)
     return ''
 
-  def ProduceDiff(self):
-    chartjsons = self._RunResourceSizes()
+  def ProduceDiff(self, archive_dirs):
+    chartjsons = self._RunResourceSizes(archive_dirs)
     diff = []
     with_patch = chartjsons[0]['charts']
     without_patch = chartjsons[1]['charts']
     for section, section_dict in with_patch.iteritems():
       for subsection, v in section_dict.iteritems():
         # Ignore entries when resource_sizes.py chartjson format has changed.
         if (section not in without_patch or
             subsection not in without_patch[section] or
             v['units'] != without_patch[section][subsection]['units']):
           _Print('Found differing dict structures for resource_sizes.py, '
                  'skipping {} {}', section, subsection)
         else:
           diff.append(
               _ResourceSizesDiffResult(
                   '%s %s' % (section, subsection),
                   v['value'] - without_patch[section][subsection]['value'],
                   v['units']))
     self._diff = sorted(diff, key=lambda x: abs(x.value), reverse=True)
 
-  def _RunResourceSizes(self):
+  def _RunResourceSizes(self, archive_dirs):
     chartjsons = []
-    for archive_dir in self._archive_dirs:
+    for archive_dir in archive_dirs:
       apk_path = os.path.join(archive_dir, self._apk_name)
       chartjson_file = os.path.join(archive_dir, 'results-chart.json')
       cmd = [self._RESOURCE_SIZES_PATH, apk_path,'--output-dir', archive_dir,
              '--no-output-dir', '--chartjson']
       if self._slow_options:
         cmd += ['--estimate-patch-size']
       else:
         cmd += ['--no-static-initializer-check']
       _RunCmd(cmd)
       with open(chartjson_file) as f:
         chartjsons.append(json.load(f))
     return chartjsons
 
 
 class _BuildHelper(object):
   """Helper class for generating and building targets."""
   def __init__(self, args):
+    self.cloud = args.cloud
     self.enable_chrome_android_internal = args.enable_chrome_android_internal
     self.extra_gn_args_str = ''
     self.max_jobs = args.max_jobs
     self.max_load_average = args.max_load_average
     self.output_directory = args.output_directory
     self.target = args.target
     self.target_os = args.target_os
     self.use_goma = args.use_goma
     self._SetDefaults()
 
@@ skipping 53 matching lines @@
     return ['gn', 'gen', self.output_directory, '--args=%s' % gn_args]
 
   def _GenNinjaCmd(self):
     cmd = ['ninja', '-C', self.output_directory]
     cmd += ['-j', self.max_jobs] if self.max_jobs else []
     cmd += ['-l', self.max_load_average] if self.max_load_average else []
     cmd += [self.target]
     return cmd
 
   def Run(self):
+    """Run GN gen/ninja build and return the process returncode."""
     _Print('Building: {}.', self.target)
-    _RunCmd(self._GenGnCmd(), print_stdout=True)
-    _RunCmd(self._GenNinjaCmd(), print_stdout=True)
+    retcode = _RunCmd(
+        self._GenGnCmd(), print_stdout=True, exit_on_failure=False)[1]
+    if retcode:
+      return retcode
+    return _RunCmd(
+        self._GenNinjaCmd(), print_stdout=True, exit_on_failure=False)[1]
 
   def IsAndroid(self):
     return self.target_os == 'android'
 
   def IsLinux(self):
     return self.target_os == 'linux'
 
+  def IsCloud(self):
+    return self.cloud
 
-def _RunCmd(cmd, print_stdout=False):
+
+class _BuildArchive(object):
+  """Class for managing a directory with build results and build metadata."""
+  def __init__(self, rev, base_archive_dir, build):
+    self.build = build
+    self.dir = os.path.join(base_archive_dir, rev)
+    _EnsureDirsExist(self.dir)

[agrieve, 2017/04/21 16:57:31]

nit: Generally best to make constructors as light-weight as possible. Should
move any IO to a separate function.

[estevenson, 2017/04/21 20:15:49]

Done.

+    metadata_path = os.path.join(self.dir, 'metadata.txt')
+    self.rev = rev
+    self.metadata = _GenerateMetadata([self], build, metadata_path)
+
+  def ArchiveBuildResults(self):
+    """Save build artifacts necessary for diffing."""
+    _Print('Saving build results to: {}', self.dir)
+    build = self.build
+    self._ArchiveFile(build.main_lib_path)
+    lib_name_noext = os.path.splitext(os.path.basename(build.main_lib_path))[0]
+    size_path = os.path.join(self.dir, lib_name_noext + '.size')
+    supersize_path = os.path.join(_SRC_ROOT, 'tools/binary_size/supersize')
+    tool_prefix = _FindToolPrefix(build.output_directory)
+    supersize_cmd = [supersize_path, 'archive', size_path, '--elf-file',
+                     build.main_lib_path, '--tool-prefix', tool_prefix,
+                     '--output-directory', build.output_directory,
+                     '--no-source-paths']
+    if build.IsAndroid():
+      supersize_cmd += ['--apk-file', build.abs_apk_path]
+      self._ArchiveFile(build.abs_apk_path)
+
+    _RunCmd(supersize_cmd)
+    _WriteMetadata(self.metadata)
+
+  def Exists(self):
+    return _MetadataExists(self.metadata)
+
+  def _ArchiveFile(self, filename):
+    if not os.path.exists(filename):
+      _Die('missing expected file: {}', filename)
+    shutil.copy(filename, self.dir)
+
+
+class _DiffArchiveManager(object):
+  """Class for maintaining BuildArchives and their related diff artifacts."""
+  def __init__(self, revs, archive_dir, diffs, build):
+    self.archive_dir = archive_dir
+    _EnsureDirsExist(archive_dir)
+    self.build = build
+    self.build_archives = [_BuildArchive(rev, archive_dir, build)
+                           for rev in revs]
+    self.diffs = diffs
+
+  def IterArchives(self):
+    return iter(self.build_archives)
+
+  def MaybeDiff(self, first_id, second_id):
+    """Perform diffs given two build archives."""
+    archives = [
+        self.build_archives[first_id], self.build_archives[second_id]]
+    diff_path = self._DiffFilePath(archives)
+    if not self._CanDiff(archives):
+      _Print('Skipping diff for {} due to missing build archives.', diff_path)
+      return
+
+    metadata_path = self._DiffMetadataPath(archives)
+    metadata = _GenerateMetadata(archives, self.build, metadata_path)
+    if _MetadataExists(metadata):
+      _Print('Skipping diff for {} and {}. Matching diff already exists: {}',
+             archives[0].rev, archives[1].rev, diff_path)
+    else:
+      archive_dirs = [archives[0].dir, archives[1].dir]
+      with open(diff_path, 'a') as diff_file:
+        for d in self.diffs:
+          d.RunDiff(diff_file, archive_dirs)
+      _WriteMetadata(metadata)
+
+  def _CanDiff(self, archives):
+    return all(a.Exists() for a in archives)
+
+  def _DiffFilePath(self, archives):
+    return os.path.join(self._DiffDir(archives), 'diff_results.txt')
+
+  def _DiffMetadataPath(self, archives):
+    return os.path.join(self._DiffDir(archives), 'metadata.txt')
+
+  def _DiffDir(self, archives):
+    diff_path = os.path.join(
+        self.archive_dir, 'diffs', '_'.join(a.rev for a in archives))
+    _EnsureDirsExist(diff_path)
+    return diff_path
+
+
+def _EnsureDirsExist(path):
+  if not os.path.exists(path):
+    os.makedirs(path)
+
+
+def _GenerateMetadata(archives, build, path):
+  return {
+    'revs': [a.rev for a in archives],
+    'archive_dirs': [a.dir for a in archives],
+    'target': build.target,
+    'target_os': build.target_os,
+    'is_cloud': build.IsCloud(),
+    'subrepo': _subrepo,
+    'path': path,
+    'gn_args': {
+      'extra_gn_args_str': build.extra_gn_args_str,
+      'enable_chrome_android_internal': build.enable_chrome_android_internal,
+    }
+  }
+
+
+def _WriteMetadata(metadata):
+  with open(metadata['path'], 'w') as f:
+    json.dump(metadata, f)
+
+
+def _MetadataExists(metadata):
+  old_metadata = {}
+  path = metadata['path']
+  if os.path.exists(path):
+    with open(path, 'r') as f:
+      old_metadata = json.load(f)
+      ret = len(metadata) == len(old_metadata)
+      ret &= all(v == old_metadata[k]
+                 for k, v in metadata.items() if k != 'gn_args')
+      if not metadata['is_cloud']:

[agrieve, 2017/04/21 16:57:31]

Can you add a comment saying why this check is necessary.

[estevenson, 2017/04/21 20:15:49]

Done.

+        ret &= metadata['gn_args'] == old_metadata['gn_args']
+      return ret
+  return False
+
+
+def _RunCmd(cmd, print_stdout=False, exit_on_failure=True):
   """Convenience function for running commands.
 
   Args:
     cmd: the command to run.
     print_stdout: if this is True, then the stdout of the process will be
-        printed, otherwise stdout will be returned.
+        printed instead of returned.
+    exit_on_failure: die if an error occurs when this is True.
 
   Returns:
-    Command stdout if |print_stdout| is False otherwise ''.
+    Tuple of (process stdout, process returncode).
   """
   cmd_str = ' '.join(c for c in cmd)
   _Print('Running: {}', cmd_str)
-  if print_stdout:
-    proc_stdout = sys.stdout
-  else:
-    proc_stdout = subprocess.PIPE
+  proc_stdout = sys.stdout if print_stdout else subprocess.PIPE
 
   proc = subprocess.Popen(cmd, stdout=proc_stdout, stderr=subprocess.PIPE)
   stdout, stderr = proc.communicate()
 
-  if proc.returncode:
+  if proc.returncode and exit_on_failure:
     _Die('command failed: {}\nstderr:\n{}', cmd_str, stderr)
 
-  return stdout.strip() if stdout else ''
+  stdout = stdout.strip() if stdout else ''
+  return stdout, proc.returncode
 
 
-def _GitCmd(args):
-  return _RunCmd(['git', '-C', _SRC_ROOT] + args)
+def _GitCmd(args, retcode=False):

[agrieve, 2017/04/21 16:57:31]

I don't see retcode being used anywhere. You also don't pass
exit_on_failure=False. If you do need this functionality, might be a nicer
interface to return None for an error.

[estevenson, 2017/04/21 20:15:49]

Oops, meant to use it in _GenerateRevList. Changed _GenerateRevList to use
_RunCmd directly.

+  return _RunCmd(['git', '-C', _subrepo] + args)[int(retcode)]
 
 
 def _GclientSyncCmd(rev):
   cwd = os.getcwd()
   os.chdir(_SRC_ROOT)
   _RunCmd(['gclient', 'sync', '-r', 'src@' + rev], print_stdout=True)
   os.chdir(cwd)
 
 
-def _ArchiveBuildResult(archive_dir, build):
-  """Save build artifacts necessary for diffing.
-
-  Expects |build.output_directory| to be correct.
-  """
-  _Print('Saving build results to: {}', archive_dir)
-  if not os.path.exists(archive_dir):
-    os.makedirs(archive_dir)
-
-  def ArchiveFile(filename):
-    if not os.path.exists(filename):
-      _Die('missing expected file: {}', filename)
-    shutil.copy(filename, archive_dir)
-
-  ArchiveFile(build.main_lib_path)
-  lib_name_noext = os.path.splitext(os.path.basename(build.main_lib_path))[0]
-  size_path = os.path.join(archive_dir, lib_name_noext + '.size')
-  supersize_path = os.path.join(_SRC_ROOT, 'tools/binary_size/supersize')
-  tool_prefix = _FindToolPrefix(build.output_directory)
-  supersize_cmd = [supersize_path, 'archive', size_path, '--elf-file',
-                   build.main_lib_path, '--tool-prefix', tool_prefix,
-                   '--output-directory', build.output_directory,
-                   '--no-source-paths']
-  if build.IsAndroid():
-    supersize_cmd += ['--apk-file', build.abs_apk_path]
-    ArchiveFile(build.abs_apk_path)
-
-  _RunCmd(supersize_cmd)
-
-
 def _FindToolPrefix(output_directory):
   build_vars_path = os.path.join(output_directory, 'build_vars.txt')
   if os.path.exists(build_vars_path):
     with open(build_vars_path) as f:
       build_vars = dict(l.rstrip().split('=', 1) for l in f if '=' in l)
     # Tool prefix is relative to output dir, rebase to source root.
     tool_prefix = build_vars['android_tool_prefix']
     while os.path.sep in tool_prefix:
       rebased_tool_prefix = os.path.join(_SRC_ROOT, tool_prefix)
       if os.path.exists(rebased_tool_prefix + 'readelf'):
         return rebased_tool_prefix
       tool_prefix = tool_prefix[tool_prefix.find(os.path.sep) + 1:]
   return ''
 
 
-def _SyncAndBuild(revs, archive_dirs, build):
-  # Move to a detached state since gclient sync doesn't work with local commits
-  # on a branch.
-  _GitCmd(['checkout', '--detach'])
-  for rev, archive_dir in itertools.izip(revs, archive_dirs):
-    _GclientSyncCmd(rev)
-    build.Run()
-    _ArchiveBuildResult(archive_dir, build)
+def _SyncAndBuild(archive, build, use_subrepo):
+  if use_subrepo:
+    _GitCmd(['checkout',  archive.rev])
+  else:
+    # Move to a detached state since gclient sync doesn't work with local
+    # commits on a branch.
+    _GitCmd(['checkout', '--detach'])
+    _GclientSyncCmd(archive.rev)
+  retcode = build.Run()
+  return retcode == 0
 
 
-def _NormalizeRev(rev):
-  """Use actual revs instead of HEAD, HEAD^, etc."""
-  return _GitCmd(['rev-parse', rev])
+def _GenerateRevList(with_patch, without_patch, all_in_range):
+  """Normalize and optionally generate a list of commits in the given range.
+
+  Returns a list of revisions ordered from newest to oldest.
+  """
+  retcode = _GitCmd(['merge-base', '--is-ancestor', without_patch, with_patch])
+  assert not retcode and with_patch != without_patch, (
+      'Invalid revision arguments, rev_without_patch (%s) is newer than '
+      'rev_with_patch (%s)' % (without_patch, with_patch))
+
+  rev_seq = '%s^..%s' % (without_patch, with_patch)
+  stdout = _GitCmd(['rev-list', rev_seq])
+  all_revs = stdout.splitlines()
+  if all_in_range:
+    revs = all_revs
+  else:
+    revs = [all_revs[0], all_revs[-1]]
+
+  assert len(revs) <= _ALLOWED_CONSECUTIVE_BUILDS, (
+      'Too many commits in range %s..%s, allowed: %d, found %d' % (
+      without_patch, with_patch, _ALLOWED_CONSECUTIVE_BUILDS, len(revs)))
+  return revs
 
 
 def _EnsureDirectoryClean():
   _Print('Checking source directory')
   stdout = _GitCmd(['status', '--porcelain'])
   # Ignore untracked files.
   if stdout and stdout[:2] != '??':
     _Die('please ensure working directory is clean.')
 
 
 def _SetInitialBranch():
   global _initial_branch
   _initial_branch = _GitCmd(['rev-parse', '--abbrev-ref', 'HEAD'])
 
 
 def _RestoreInitialBranch():
   if _initial_branch:
     _GitCmd(['checkout', _initial_branch])
 
 
+def _SetSubrepo(subrepo):
+  global _subrepo
+  _subrepo = subrepo or _SRC_ROOT
+
+
 def _Die(s, *args, **kwargs):
   _Print('Failure: ' + s, *args, **kwargs)
   _RestoreInitialBranch()
   sys.exit(1)
 
 
-def _DownloadBuildArtifacts(revs, archive_dirs, build, depot_tools_path=None):
+def _DownloadBuildArtifacts(archive, build, depot_tools_path=None):
   """Download artifacts from arm32 chromium perf builder."""
   if depot_tools_path:
     gsutil_path = os.path.join(depot_tools_path, 'gsutil.py')
   else:
     gsutil_path = distutils.spawn.find_executable('gsutil.py')
 
   if not gsutil_path:
     _Die('gsutil.py not found, please provide path to depot_tools via '
          '--depot-tools-path or add it to your PATH')
 
   download_dir = tempfile.mkdtemp(dir=_SRC_ROOT)
   try:
-    for rev, archive_dir in itertools.izip(revs, archive_dirs):
-      _DownloadAndArchive(gsutil_path, rev, archive_dir, download_dir, build)
+    _DownloadAndArchive(gsutil_path, archive, download_dir, build)
   finally:
     shutil.rmtree(download_dir)
 
 
-def _DownloadAndArchive(gsutil_path, rev, archive_dir, dl_dir, build):
-  dl_file = 'full-build-linux_%s.zip' % rev
+def _DownloadAndArchive(gsutil_path, archive, dl_dir, build):
+  dl_file = 'full-build-linux_%s.zip' % archive.rev
   dl_url = 'gs://chrome-perf/Android Builder/%s' % dl_file
   dl_dst = os.path.join(dl_dir, dl_file)
-  _Print('Downloading build artifacts for {}', rev)
+  _Print('Downloading build artifacts for {}', archive.rev)
   # gsutil writes stdout and stderr to stderr, so pipe stdout and stderr to
   # sys.stdout.
   retcode = subprocess.call([gsutil_path, 'cp', dl_url, dl_dir],
                              stdout=sys.stdout, stderr=subprocess.STDOUT)
   if retcode:
       _Die('unexpected error while downloading {}. It may no longer exist on '
            'the server or it may not have been uploaded yet (check {}). '
            'Otherwise, you may not have the correct access permissions.',
            dl_url, _BUILDER_URL)
 
   # Files needed for supersize and resource_sizes. Paths relative to out dir.
   to_extract = [build.main_lib_name, build.map_file_name, 'args.gn',
                 'build_vars.txt', build.apk_path]
   extract_dir = os.path.join(os.path.splitext(dl_dst)[0], 'unzipped')
   # Storage bucket stores entire output directory including out/Release prefix.
   _Print('Extracting build artifacts')
   with zipfile.ZipFile(dl_dst, 'r') as z:
     _ExtractFiles(to_extract, _CLOUD_OUT_DIR, extract_dir, z)
     dl_out = os.path.join(extract_dir, _CLOUD_OUT_DIR)
     build.output_directory, output_directory = dl_out, build.output_directory
-    _ArchiveBuildResult(archive_dir, build)
+    archive.ArchiveBuildResults()
     build.output_directory = output_directory
 
 
 def _ExtractFiles(to_extract, prefix, dst, z):
   zip_infos = z.infolist()
   assert all(info.filename.startswith(prefix) for info in zip_infos), (
       'Storage bucket folder structure doesn\'t start with %s' % prefix)
   to_extract = [os.path.join(prefix, f) for f in to_extract]
   for f in to_extract:
     z.extract(f, path=dst)
@@ skipping 15 matching lines @@
       formatter_class=argparse.ArgumentDefaultsHelpFormatter)
   parser.add_argument('--archive-dir',
                       default=_DEFAULT_ARCHIVE_DIR,
                       help='Where results are stored.')
   parser.add_argument('--rev-with-patch',
                       default='HEAD',
                       help='Commit with patch.')
   parser.add_argument('--rev-without-patch',
                       help='Older patch to diff against. If not supplied, '
                       'the previous commit to rev_with_patch will be used.')
+  parser.add_argument('--all',
+                      action='store_true',
+                      help='Build all revs from rev_with_patch to '

[agrieve, 2017/04/21 16:57:31]

I've been finding the flags a bit unintuitive, I think mainly because "with
patch" does not always mean "commit comes after" in my head. It's also a bit
confusing that commits are built in reverse order (although makes sense when you
think about it).

I've also found it a bit surprising when running without flags does things.

Suggestion:

Add a single positional argument that is the commit range. E.g. HEAD,
ababab..232323, HEAD^^^^..HEAD

git rev-parse will return a single hash for HEAD, and return start/end for a
range. e.g.:

$ git rev-parse bfce95b70e45..5ed39274a52c
5ed39274a52c6e34e35e7a9c95aff9ef64a5ca32
^bfce95b70e45ee513b3735e27660fbecc1eeea5f

[estevenson, 2017/04/21 20:15:49]

Spoke offline a bit about this, I've change it so that:
..bloat.py -> shows help
..bloat.py bfce95b70e45 -> builds/diffs bfce95b70e45^ and bfce95b70e45
..bloat.py bfce95b70e45 --reference-rev 5ed39274a52c -> builds/diffs
5ed39274a52c and bfce95b70e45
..bloat.py bfce95b70e45 --reference-rev 5ed39274a52c --all -> builds/diffs
5ed39274a52c..bfce95b70e45

Didn't update code references to --without and --with-patch since that's still
pretty clear to me (and using just rev would be confusing). Let me know if you'd
like me to.

+                      'rev_without_patch and diff contiguous revisions.')
   parser.add_argument('--include-slow-options',
                       action='store_true',
                       help='Run some extra steps that take longer to complete. '
                       'This includes apk-patch-size estimation and '
                       'static-initializer counting')
   parser.add_argument('--cloud',
                       action='store_true',
                       help='Download build artifacts from perf builders '
                       '(Android only, Googlers only).')
   parser.add_argument('--depot-tools-path',
                       help='Custom path to depot tools. Needed for --cloud if '
                       'depot tools isn\'t in your PATH')
+  parser.add_argument('--subrepo',
+                      help='Specify a subrepo directory to use. Gclient sync '
+                      'will be skipped if this option is used and all git '
+                      'commands will be executed from the subrepo directory. '
+                      'This option doesn\'t work with --cloud.')
 
   build_group = parser.add_argument_group('ninja', 'Args to use with ninja/gn')
   build_group.add_argument('-j',
                            dest='max_jobs',
                            help='Run N jobs in parallel.')
   build_group.add_argument('-l',
                            dest='max_load_average',
                            help='Do not start new jobs if the load average is '
                            'greater than N.')
   build_group.add_argument('--no-goma',
                            action='store_false',
                            dest='use_goma',
                            default=True,
                            help='Use goma when building with ninja.')
   build_group.add_argument('--target-os',
                            default='android',
                            choices=['android', 'linux'],
                            help='target_os gn arg.')
   build_group.add_argument('--output-directory',
                            default=_DEFAULT_OUT_DIR,
                            help='ninja output directory.')
-  build_group.add_argument('--enable_chrome_android_internal',
+  build_group.add_argument('--enable-chrome-android-internal',
                            action='store_true',
                            help='Allow downstream targets to be built.')
   build_group.add_argument('--target',
                            default=_DEFAULT_TARGET,
                            help='GN APK target to build.')
   args = parser.parse_args()
   build = _BuildHelper(args)
-  if args.cloud and build.IsLinux():
-    parser.error('--cloud only works for android')
+  _SetSubrepo(args.subrepo)
+  use_subrepo = bool(args.subrepo)
+  if build.IsCloud():
+    if build.IsLinux():
+      parser.error('--cloud only works for android')
+    if use_subrepo:
+      parser.error('--subrepo doesn\'t work with --cloud')
 
   _EnsureDirectoryClean()
   _SetInitialBranch()
-  revs = [args.rev_with_patch,
-          args.rev_without_patch or args.rev_with_patch + '^']
-  revs = [_NormalizeRev(r) for r in revs]
-  archive_dirs = [os.path.join(args.archive_dir, '%d-%s' % (len(revs) - i, rev))
-                  for i, rev in enumerate(revs)]
-  if args.cloud:
-    _DownloadBuildArtifacts(revs, archive_dirs, build,
-                            depot_tools_path=args.depot_tools_path)
-  else:
-    _SetInitialBranch()
-    _SyncAndBuild(revs, archive_dirs, build)
-    _RestoreInitialBranch()
-
-  output_file = os.path.join(args.archive_dir,
-                             'diff_result_{}_{}.txt'.format(*revs))
-  if os.path.exists(output_file):
-    os.remove(output_file)
+  revs = _GenerateRevList(args.rev_with_patch,
+                          args.rev_without_patch or args.rev_with_patch + '^',
+                          args.all)
   diffs = []
   if build.IsAndroid():
     diffs +=  [
-        ResourceSizesDiff(archive_dirs, build.apk_name,
-                          slow_options=args.include_slow_options)
+        ResourceSizesDiff(
+            build.apk_name, slow_options=args.include_slow_options)
     ]
-  with open(output_file, 'a') as logfile:
-    for d in diffs:
-      d.AppendResults(logfile)
+  diff_mngr = _DiffArchiveManager(revs, args.archive_dir, diffs, build)
+  consecutive_failures = 0
+  for i, archive in enumerate(diff_mngr.IterArchives()):
+    if archive.Exists():
+      _Print('Found matching metadata for {}, skipping build step.',
+             archive.rev)
+    else:
+      if build.IsCloud():
+        _DownloadBuildArtifacts(archive, build,
+                                depot_tools_path=args.depot_tools_path)
+      else:
+        build_success = _SyncAndBuild(archive, build, use_subrepo)
+        if not build_success:
+          consecutive_failures += 1
+          if consecutive_failures > _ALLOWED_CONSECUTIVE_FAILURES:
+            _Die('{} builds failed in a row, last failure was {}.',
+                 consecutive_failures, archive.rev)
+        else:
+          archive.ArchiveBuildResults()
+          consecutive_failures = 0
+
+    if i != 0:
+      diff_mngr.MaybeDiff(i - 1, i)
+
+  _RestoreInitialBranch()
 
 if __name__ == '__main__':
   sys.exit(main())