Merge "releasetools: Remove the unconditional fallback to bsdiff."

tools/releasetools/blockimgdiff.py

@@ -264,9 +264,6 @@ class ImgdiffStats(object):
   reasons. The stats is only meaningful when imgdiff not being disabled by the
   caller of BlockImageDiff. In addition, only files with supported types
   (BlockImageDiff.FileTypeSupportedByImgdiff()) are allowed to be logged.
-
-  TODO: The info could be inaccurate due to the unconditional fallback from
-  imgdiff to bsdiff on errors. The fallbacks will be removed.
   """
 
   USED_IMGDIFF = "APK files diff'd with imgdiff"
@@ -275,6 +272,7 @@ class ImgdiffStats(object):
   # Reasons for not applying imgdiff on APKs.
   SKIPPED_TRIMMED = "Not used imgdiff due to trimmed RangeSet"
   SKIPPED_NONMONOTONIC = "Not used imgdiff due to having non-monotonic ranges"
+  SKIPPED_INCOMPLETE = "Not used imgdiff due to incomplete RangeSet"
 
   # The list of valid reasons, which will also be the dumped order in a report.
   REASONS = (
@@ -282,6 +280,7 @@ class ImgdiffStats(object):
       USED_IMGDIFF_LARGE_APK,
       SKIPPED_TRIMMED,
       SKIPPED_NONMONOTONIC,
+      SKIPPED_INCOMPLETE,
   )
 
   def  __init__(self):
@@ -415,6 +414,7 @@ class BlockImageDiff(object):
       - The file type is supported by imgdiff;
       - The source and target blocks are monotonic (i.e. the data is stored with
         blocks in increasing order);
+      - Both files have complete lists of blocks;
       - We haven't removed any blocks from the source set.
 
     If all these conditions are satisfied, concatenating all the blocks in the
@@ -437,6 +437,10 @@ class BlockImageDiff(object):
       self.imgdiff_stats.Log(name, ImgdiffStats.SKIPPED_NONMONOTONIC)
       return False
 
+    if tgt_ranges.extra.get('incomplete') or src_ranges.extra.get('incomplete'):
+      self.imgdiff_stats.Log(name, ImgdiffStats.SKIPPED_INCOMPLETE)
+      return False
+
     if tgt_ranges.extra.get('trimmed') or src_ranges.extra.get('trimmed'):
       self.imgdiff_stats.Log(name, ImgdiffStats.SKIPPED_TRIMMED)
       return False
@@ -851,7 +855,6 @@ class BlockImageDiff(object):
       diff_total = len(diff_queue)
       patches = [None] * diff_total
       error_messages = []
-      warning_messages = []
 
       # Using multiprocessing doesn't give additional benefits, due to the
       # pattern of the code. The diffing work is done by subprocess.call, which
@@ -899,23 +902,6 @@ class BlockImageDiff(object):
                       xf.tgt_name if xf.tgt_name == xf.src_name else
                       xf.tgt_name + " (from " + xf.src_name + ")",
                       xf.tgt_ranges, xf.src_ranges, e.message))
-              # TODO(b/68016761): Better handle the holes in mke2fs created
-              # images.
-              if imgdiff:
-                try:
-                  patch = compute_patch(src_file, tgt_file, imgdiff=False)
-                  message.append(
-                      "Fell back and generated with bsdiff instead for %s" % (
-                          xf.tgt_name,))
-                  xf.style = "bsdiff"
-                  with lock:
-                    warning_messages.extend(message)
-                  del message[:]
-                except ValueError as e:
-                  message.append(
-                      "Also failed to generate with bsdiff for %s:\n%s" % (
-                          xf.tgt_name, e.message))
-
             if message:
               with lock:
                 error_messages.extend(message)
@@ -933,11 +919,6 @@ class BlockImageDiff(object):
       if sys.stdout.isatty():
         print('\n')
 
-      if warning_messages:
-        print('WARNING:')
-        print('\n'.join(warning_messages))
-        print('\n\n\n')
-
       if error_messages:
         print('ERROR:')
         print('\n'.join(error_messages))
@@ -1518,11 +1499,6 @@ class BlockImageDiff(object):
       be valid because the block ranges of src-X & tgt-X will always stay the
       same afterwards; but there's a chance we don't use the patch if we
       convert the "diff" command into "new" or "move" later.
-
-      The split will be attempted by calling imgdiff, which expects the input
-      files to be valid zip archives. If imgdiff fails for some reason (i.e.
-      holes in the APK file), we will fall back to split the failed APKs into
-      fixed size chunks.
       """
 
       while True:
@@ -1544,16 +1520,11 @@ class BlockImageDiff(object):
                "--block-limit={}".format(max_blocks_per_transfer),
                "--split-info=" + patch_info_file,
                src_file, tgt_file, patch_file]
-        p = common.Run(cmd, stdout=subprocess.PIPE)
+        p = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
-        p.communicate()
+        imgdiff_output, _ = p.communicate()
-        if p.returncode != 0:
+        assert p.returncode == 0, \
-          print("Failed to create patch between {} and {},"
+            "Failed to create imgdiff patch between {} and {}:\n{}".format(
-                " falling back to bsdiff".format(src_name, tgt_name))
+                src_name, tgt_name, imgdiff_output)
-          with transfer_lock:
-            AddSplitTransfersWithFixedSizeChunks(tgt_name, src_name,
-                                                 tgt_ranges, src_ranges,
-                                                 "diff", self.transfers)
-          continue
 
         with open(patch_info_file) as patch_info:
           lines = patch_info.readlines()

tools/releasetools/test_blockimgdiff.py

@@ -236,11 +236,19 @@ class BlockImageDiffTest(unittest.TestCase):
         block_image_diff.CanUseImgdiff(
             "/vendor/app/app3.apk", RangeSet("10-15"), src_ranges))
 
+    # At least one of the ranges is incomplete.
+    src_ranges = RangeSet("0-5")
+    src_ranges.extra['incomplete'] = True
+    self.assertFalse(
+        block_image_diff.CanUseImgdiff(
+            "/vendor/app/app4.apk", RangeSet("10-15"), src_ranges))
+
     # The stats are correctly logged.
     self.assertDictEqual(
         {
             ImgdiffStats.SKIPPED_NONMONOTONIC : {'/system/app/app2.apk'},
             ImgdiffStats.SKIPPED_TRIMMED : {'/vendor/app/app3.apk'},
+            ImgdiffStats.SKIPPED_INCOMPLETE: {'/vendor/app/app4.apk'},
         },
         block_image_diff.imgdiff_stats.stats)