[NaCl SDK] update_nacl_manifest: fix bundle reordering bug.

When running update_nacl_manifest, if a bundle version is identical to the
online version, it would use that bundle instead. This had the side-effect of
moving the bundle to the end of the list.

BUG=none
R=sbc@chromium.org
NOTRY=true


Review URL: https://chromiumcodereview.appspot.com/12385091

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@185972 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 36 insertions, 9 deletions

diff --git a/native_client_sdk/src/build_tools/manifest_util.py b/native_client_sdk/src/build_tools/manifest_util.py
index 4d7c7c7..9091453 100644
--- a/native_client_sdk/src/build_tools/manifest_util.py
+++ b/native_client_sdk/src/build_tools/manifest_util.py
@@ -454,20 +454,23 @@ class SDKManifest(object):
     return self._manifest_data[BUNDLES_KEY]
 
   def SetBundle(self, new_bundle):
-    """Replace named bundle.  Add if absent.
+    """Add or replace a bundle in the manifest.
+
+    Note: If a bundle in the manifest already exists with this name, it will be
+    overwritten with a copy of this bundle, at the same index as the original.
 
     Args:
       bundle: The bundle.
     """
     name = new_bundle[NAME_KEY]
-    if not BUNDLES_KEY in self._manifest_data:
-      self._manifest_data[BUNDLES_KEY] = []
-    bundles = self._manifest_data[BUNDLES_KEY]
-    # Delete any bundles from the list, then add the new one.  This has the
-    # effect of replacing the bundle if it already exists.  It also removes all
-    # duplicate bundles.
-    self.RemoveBundle(name)
-    bundles.append(copy.deepcopy(new_bundle))
+    bundles = self.GetBundles()
+    new_bundle_copy = copy.deepcopy(new_bundle)
+    for i, bundle in enumerate(bundles):
+      if bundle[NAME_KEY] == name:
+        bundles[i] = new_bundle_copy
+        return
+    # Bundle not already in list, append it.
+    bundles.append(new_bundle_copy)
 
   def RemoveBundle(self, name):
     """Remove a bundle by name.
diff --git a/native_client_sdk/src/build_tools/tests/update_nacl_manifest_test.py b/native_client_sdk/src/build_tools/tests/update_nacl_manifest_test.py
index 38b8068..9cccfcb 100755
--- a/native_client_sdk/src/build_tools/tests/update_nacl_manifest_test.py
+++ b/native_client_sdk/src/build_tools/tests/update_nacl_manifest_test.py
@@ -648,6 +648,30 @@ mac,canary,21.0.1156.0,2012-05-30 12:14:21.305090"""
     uploaded_bundle = self.uploaded_manifest.GetBundle('pepper_26')
     self.assertEqual(2, len(uploaded_bundle.GetHostOSArchives()))
 
+  def testKeepBundleOrder(self):
+    # This is a regression test: when a bundle is skipped (because it isn't
+    # newer than the online bundle), it was added to the end of the list.
+
+    # Make an online manifest that already has B18.
+    online_manifest = MakeManifest(B18_0_1025_163_R1_MLW)
+    self.files.AddOnlineManifest(online_manifest.GetDataAsString())
+
+    self.manifest = MakeManifest(B18_R1_NONE, B19_R1_NONE)
+    self.history.Add(OS_MLW, STABLE, V18_0_1025_163)
+    self.history.Add(OS_MLW, STABLE, V19_0_1084_41)
+    self.files.Add(B18_0_1025_163_R1_MLW)
+    self.files.Add(B19_0_1084_41_R1_MLW)
+
+    self._MakeDelegate()
+    self._Run(OS_MLW)
+    self._ReadUploadedManifest()
+
+    # Bundle 18 should be before bundle 19.
+    bundles = self.uploaded_manifest.GetBundles()
+    self.assertEqual(2, len(bundles))
+    self.assertEqual('pepper_18', bundles[0].name)
+    self.assertEqual('pepper_19', bundles[1].name)
+
 
 class TestUpdateVitals(unittest.TestCase):
   def setUp(self):