trybot_commit_size_checker.py: Show resource_sizes.py diff inline

* Removes obsolete command-line flags.
* Moves resource_sizes.py diff as well as failing size checks inline

Bug: 914654
Change-Id: I444da444a2bbed4a154ce1d4a64bb99549e81e86
Reviewed-on: https://chromium-review.googlesource.com/c/1407375Reviewed-by: Eric Stevenson <estevenson@chromium.org>
Commit-Queue: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622156}

tools/binary_size/diagnose_bloat.py

@@ -148,13 +148,13 @@ class ResourceSizesDiff(BaseDiff):
     return self._ResultLines()
 
   def Summary(self):
-    header_lines = [
+    footer_lines = [
+        '',
         'For an explanation of these metrics, see:',
         ('https://chromium.googlesource.com/chromium/src/+/master/docs/speed/'
-         'binary_size/metrics.md#Metrics-for-Android'),
-        '']
-    return header_lines + self._ResultLines(
-        include_sections=ResourceSizesDiff._SUMMARY_SECTIONS)
+         'binary_size/metrics.md#Metrics-for-Android')]
+    return self._ResultLines(
+        include_sections=ResourceSizesDiff._SUMMARY_SECTIONS) + footer_lines
 
   def ProduceDiff(self, before_dir, after_dir):
     before = self._LoadResults(before_dir)

tools/binary_size/trybot_commit_size_checker.py

@@ -34,9 +34,8 @@ _NORMALIZED_APK_SIZE_DETAILS = (
     'for an explanation of Normalized APK Size')
 
 _FAILURE_GUIDANCE = """
-Please look at the symbol diffs from the "Show Resource Sizes Diff",
-"Show Supersize Diff", and "Dex Method Count", and "Supersize HTML Report" bot
-steps. Try and understand the growth and see if it can be mitigated.
+Please look at size breakdowns, try to understand the growth, and see if it can
+be mitigated.
 
 There is guidance at:
 
@@ -78,7 +77,7 @@ class _SizeDelta(collections.namedtuple(
     return cmp(self.name, other.name)
 
 
-def _CreateAndWriteMethodCountDelta(symbols, output_path):
+def _CreateAndWriteMethodCountDelta(symbols):
   dex_symbols = symbols.WhereInSection(models.SECTION_DEX_METHOD)
   dex_added = dex_symbols.WhereDiffStatusIs(models.DIFF_STATUS_ADDED)
   dex_removed = dex_symbols.WhereDiffStatusIs(models.DIFF_STATUS_REMOVED)
@@ -91,31 +90,21 @@ def _CreateAndWriteMethodCountDelta(symbols, output_path):
   lines.append('Removed: {}'.format(dex_removed_count))
   lines.extend(sorted(s.full_name for s in dex_removed))
 
-  if output_path:
-    with open(output_path, 'w') as f:
-      f.writelines(l + '\n' for l in lines)
-
   return lines, _SizeDelta('Dex Methods', 'methods',
                            _MAX_DEX_METHOD_COUNT_INCREASE, dex_net_added,
                            _DEX_DETAILS)
 
 
-def _CreateAndWriteResourceSizesDelta(apk_name, before_dir, after_dir,
-                                      output_path):
+def _CreateAndWriteResourceSizesDelta(apk_name, before_dir, after_dir):
   sizes_diff = diagnose_bloat.ResourceSizesDiff(apk_name)
   sizes_diff.ProduceDiff(before_dir, after_dir)
 
-  lines = sizes_diff.Summary()
-  if output_path:
-    with open(output_path, 'w') as f:
-      f.writelines(l + '\n' for l in lines)
-
-  return lines, _SizeDelta(
+  return sizes_diff.Summary(), _SizeDelta(
       'Normalized APK Size', 'bytes', _MAX_NORMALIZED_INCREASE,
       sizes_diff.summary_stat.value, _NORMALIZED_APK_SIZE_DETAILS)
 
 
-def _CreateAndWriteSupersizeDiff(apk_name, before_dir, after_dir, output_path):
+def _CreateAndWriteSupersizeDiff(apk_name, before_dir, after_dir):
   before_size_path = os.path.join(before_dir, apk_name + '.size')
   after_size_path = os.path.join(after_dir, apk_name + '.size')
   before = archive.LoadAndPostProcessSizeInfo(before_size_path)
@@ -123,11 +112,6 @@ def _CreateAndWriteSupersizeDiff(apk_name, before_dir, after_dir, output_path):
   size_info_delta = diff.Diff(before, after, sort=True)
 
   lines = list(describe.GenerateLines(size_info_delta))
-
-  if output_path:
-    with open(output_path, 'w') as f:
-      f.writelines(l + '\n' for l in lines)
-
   return lines, size_info_delta
 
 
@@ -156,36 +140,23 @@ def main():
       '--after-dir',
       required=True,
       help='Directory containing APK for the new build.')
-  parser.add_argument(
-      '--resource-sizes-diff-path',
-      help='Output path for the resource_sizes.py diff.')
-  parser.add_argument(
-      '--supersize-diff-path', help='Output path for the Supersize diff.')
-  parser.add_argument(
-      '--dex-method-count-diff-path',
-      help='Output path for the dex method count diff.')
-  parser.add_argument(
-      '--ndjson-path', help='Output path for the Supersize HTML report.')
   parser.add_argument(
       '--results-path',
       required=True,
       help='Output path for the trybot result .json file.')
   parser.add_argument(
-      '--staging-dir', help='Directory to write summary files to.')
+      '--staging-dir',
+      required=True,
+      help='Directory to write summary files to.')
   parser.add_argument('-v', '--verbose', action='store_true')
   args = parser.parse_args()
 
   if args.verbose:
     logging.basicConfig(level=logging.INFO)
 
-  ndjson_path = args.ndjson_path
-  # TODO(agrieve): Remove above args once recipe is updated.
-  if args.staging_dir:
-    ndjson_path = os.path.join(args.staging_dir, _NDJSON_FILENAME)
-
   logging.info('Creating Supersize diff')
   supersize_diff_lines, delta_size_info = _CreateAndWriteSupersizeDiff(
-      args.apk_name, args.before_dir, args.after_dir, args.supersize_diff_path)
+      args.apk_name, args.before_dir, args.after_dir)
 
   changed_symbols = delta_size_info.raw_symbols.WhereDiffStatusIs(
       models.DIFF_STATUS_UNCHANGED).Inverted()
@@ -193,8 +164,7 @@ def main():
   # Monitor dex method growth since this correlates closely with APK size and
   # may affect our dex file structure.
   logging.info('Checking dex symbols')
-  dex_delta_lines, dex_delta = _CreateAndWriteMethodCountDelta(
-      changed_symbols, args.dex_method_count_diff_path)
+  dex_delta_lines, dex_delta = _CreateAndWriteMethodCountDelta(changed_symbols)
   size_deltas = {dex_delta}
 
   # Check for uncompressed .pak file entries being added to avoid unnecessary
@@ -206,66 +176,49 @@ def main():
   logging.info('Creating sizes diff')
   resource_sizes_lines, resource_sizes_delta = (
       _CreateAndWriteResourceSizesDelta(args.apk_name, args.before_dir,
-                                        args.after_dir,
-                                        args.resource_sizes_diff_path))
+                                        args.after_dir))
   size_deltas.add(resource_sizes_delta)
 
   # .ndjson can be consumed by the html viewer.
   logging.info('Creating HTML Report')
-  html_report.BuildReportFromSizeInfo(
-      ndjson_path, delta_size_info, all_symbols=True)
+  ndjson_path = os.path.join(args.staging_dir, _NDJSON_FILENAME)
+  html_report.BuildReportFromSizeInfo(ndjson_path, delta_size_info)
 
   passing_deltas = set(m for m in size_deltas if m.IsAllowable())
   failing_deltas = size_deltas - passing_deltas
 
   is_roller = '-autoroll' in args.author
-  checks_text = """
-
-Binary size checks {}.
-
-*******************************************************************************
+  failing_checks_text = '\n'.join(d.explanation for d in sorted(failing_deltas))
+  passing_checks_text = '\n'.join(d.explanation for d in sorted(passing_deltas))
+  checks_text = """\
 FAILING:
-
 {}
 
-*******************************************************************************
-
 PASSING:
-
 {}
-
-*******************************************************************************
-
-""".format('failed' if failing_deltas else 'passed', '\n\n'.join(
-      d.explanation for d in sorted(failing_deltas)), '\n\n'.join(
-          d.explanation for d in sorted(passing_deltas)))
+""".format(failing_checks_text, passing_checks_text)
 
   if failing_deltas:
     checks_text += _FAILURE_GUIDANCE
 
   status_code = 1 if failing_deltas and not is_roller else 0
-  summary = '<br>'.join([
-      '',
-      'Normalized apk size delta: {}'.format(resource_sizes_delta.actual),
-      'Dex method count delta: {}'.format(dex_delta.actual),
-  ])
+  summary = '<br>' + '<br>'.join(resource_sizes_lines)
+  if 'Empty Resource Sizes Diff' in summary:
+    summary = '<br>No size metrics were affected.'
+  if failing_deltas:
+    summary += '<br><br>Failed Size Checks:<br>'
+    summary += failing_checks_text.replace('\n', '<br>')
+    summary += '<br>Look at "Size Assertion Results" for guidance.'
 
-  # TODO(agrieve): Remove once recipe is updated: details, normalized_apk_size
   results_json = {
       'status_code': status_code,
       'summary': summary,
-      'details': checks_text,
-      'normalized_apk_size': resource_sizes_delta.actual,
       'archive_filenames': [_NDJSON_FILENAME],
       'links': [
           {
               'name': '>>> Size Assertion Results <<<',
               'lines': checks_text.splitlines(),
           },
-          {
-              'name': '>>> Resource Sizes Diff (high-level metrics) <<<',
-              'lines': resource_sizes_lines,
-          },
           {
               'name': '>>> Dex Method Diff <<<',
               'lines': dex_delta_lines,