SDK Manager: handle platform-tools preview + final.

One issue is that when there was only one instance of platform-tools possible,
the computeUpdates() code would pick the first one. But now there can be 2 of
them (preview, non-preview) and thus we need to pick up the higher one even if
it's not the first choice.

Same issue with tools: if a platform depends on tools and there are none
installed, we need to pick the highest version of the available preview
or final package that satisfies the dependency.

Note that in both cases the issue does not arise if there's already
a tools or platform-tools installed.

Change-Id: I61db2881274dafa32c5c303c0b3569fc336b8e92

4 files changed, 274 insertions, 53 deletions

diff --git a/sdkmanager/libs/sdklib/tests/src/com/android/sdklib/internal/repository/packages/MockPlatformPackage.java b/sdkmanager/libs/sdklib/tests/src/com/android/sdklib/internal/repository/packages/MockPlatformPackage.java
index 1d70ba9..dd744ec 100755
--- a/sdkmanager/libs/sdklib/tests/src/com/android/sdklib/internal/repository/packages/MockPlatformPackage.java
+++ b/sdkmanager/libs/sdklib/tests/src/com/android/sdklib/internal/repository/packages/MockPlatformPackage.java
@@ -47,7 +47,7 @@ public class MockPlatformPackage extends PlatformPackage {
      * Creates a {@link MockPlatformTarget} with the requested API and revision
      * and then a {@link MockPlatformPackage} wrapping it.
      *
-     * Also sets the min-tools-rev of the platform.
+     * Also sets the min-tools-rev of the platform using a major revision integer.
      *
      * By design, this package contains one and only one archive.
      */
@@ -57,6 +57,20 @@ public class MockPlatformPackage extends PlatformPackage {
              createProps(min_tools_rev));
     }
 
+    /**
+     * Creates a {@link MockPlatformTarget} with the requested API and revision
+     * and then a {@link MockPlatformPackage} wrapping it.
+     *
+     * Also sets the min-tools-rev of the platform using a {@link FullRevision}.
+     *
+     * By design, this package contains one and only one archive.
+     */
+    public MockPlatformPackage(int apiLevel, int revision, FullRevision min_tools_rev) {
+        this(null /*source*/,
+             new MockPlatformTarget(apiLevel, revision),
+             createProps(min_tools_rev));
+    }
+
     public MockPlatformPackage(SdkSource source, int apiLevel, int revision, int min_tools_rev) {
         this(source, new MockPlatformTarget(apiLevel, revision), createProps(min_tools_rev));
     }
@@ -73,6 +87,12 @@ public class MockPlatformPackage extends PlatformPackage {
         return props;
     }
 
+    private static Properties createProps(FullRevision min_tools_rev) {
+        Properties props = new Properties();
+        props.setProperty(PkgProps.MIN_TOOLS_REV, min_tools_rev.toShortString());
+        return props;
+    }
+
     public IAndroidTarget getTarget() {
         return mTarget;
     }
diff --git a/sdkmanager/libs/sdklib/tests/src/com/android/sdklib/internal/repository/packages/MockToolPackage.java b/sdkmanager/libs/sdklib/tests/src/com/android/sdklib/internal/repository/packages/MockToolPackage.java
index 32711d6..3c39194 100755
--- a/sdkmanager/libs/sdklib/tests/src/com/android/sdklib/internal/repository/packages/MockToolPackage.java
+++ b/sdkmanager/libs/sdklib/tests/src/com/android/sdklib/internal/repository/packages/MockToolPackage.java
@@ -84,10 +84,40 @@ public class MockToolPackage extends ToolPackage {
                 );
     }
 
+    /**
+     * Creates a {@link MockToolPackage} with the given revision and hardcoded defaults
+     * for everything else.
+     * <p/>
+     * By design, this creates a package with one and only one archive.
+     */
+    public MockToolPackage(
+            SdkSource source,
+            FullRevision revision,
+            FullRevision minPlatformToolsRev) {
+        super(
+                source, // source,
+                createProps(revision, minPlatformToolsRev), // props,
+                revision.getMajor(),
+                null, // license,
+                "desc", // description,
+                "url", // descUrl,
+                Os.getCurrentOs(), // archiveOs,
+                Arch.getCurrentArch(), // archiveArch,
+                "foo" // archiveOsPath
+                );
+    }
+
     private static Properties createProps(FullRevision revision, int minPlatformToolsRev) {
         Properties props = FullRevisionPackageTest.createProps(revision);
         props.setProperty(PkgProps.MIN_PLATFORM_TOOLS_REV,
                           Integer.toString((minPlatformToolsRev)));
         return props;
     }
+
+    private static Properties createProps(FullRevision revision, FullRevision minPlatformToolsRev) {
+        Properties props = FullRevisionPackageTest.createProps(revision);
+        props.setProperty(PkgProps.MIN_PLATFORM_TOOLS_REV,
+                          minPlatformToolsRev.toShortString());
+        return props;
+    }
 }
diff --git a/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/SdkUpdaterLogic.java b/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/SdkUpdaterLogic.java
index da71115..25e6553 100755
--- a/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/SdkUpdaterLogic.java
+++ b/sdkmanager/libs/sdkuilib/src/com/android/sdkuilib/internal/repository/SdkUpdaterLogic.java
@@ -738,25 +738,34 @@ class SdkUpdaterLogic {
 
         // Finally nothing matched, so let's look at all available remote packages
         fetchRemotePackages(remotePkgs, remoteSources);
+        FullRevision localRev = rev;
+        Archive localArch = null;
         for (Package p : remotePkgs) {
             if (p instanceof ToolPackage) {
-                if (((ToolPackage) p).getRevision().compareTo(rev) >= 0) {
+                FullRevision r = ((ToolPackage) p).getRevision();
+                if (r.compareTo(localRev) >= 0) {
                     // It's not already in the list of things to install, so add the
                     // first compatible archive we can find.
                     for (Archive a : p.getArchives()) {
                         if (a.isCompatible()) {
-                            return insertArchive(a,
-                                    outArchives,
-                                    selectedArchives,
-                                    remotePkgs,
-                                    remoteSources,
-                                    localArchives,
-                                    true /*automated*/);
+                            localRev = r;
+                            localArch = a;
+                            break;
                         }
                     }
                 }
             }
         }
+        if (localArch != null) {
+            return insertArchive(localArch,
+                    outArchives,
+                    selectedArchives,
+                    remotePkgs,
+                    remoteSources,
+                    localArchives,
+                    true /*automated*/);
+
+        }
 
         // We end up here if nothing matches. We don't have a good platform to match.
         // We need to indicate this extra depends on a missing platform archive
@@ -782,6 +791,7 @@ class SdkUpdaterLogic {
         // This is the requirement to match.
         FullRevision rev = pkg.getMinPlatformToolsRevision();
         boolean findMax = false;
+        int compareThreshold = 0;
         ArchiveInfo aiMax = null;
         Archive aMax = null;
 
@@ -793,6 +803,9 @@ class SdkUpdaterLogic {
             // So instead we parse all the existing and remote packages and try to find
             // the max available revision and we'll use it.
             findMax = true;
+            // When findMax is false, we want r.compareTo(rev) >= 0.
+            // When findMax is true, we want r.compareTo(rev) > 0 (so >= 1).
+            compareThreshold = 1;
         }
 
         // First look in locally installed packages.
@@ -802,10 +815,10 @@ class SdkUpdaterLogic {
                 Package p = a.getParentPackage();
                 if (p instanceof PlatformToolPackage) {
                     FullRevision r = ((PlatformToolPackage) p).getRevision();
-                    if (findMax && r.compareTo(rev) > 0) {
+                    if (findMax && r.compareTo(rev) > compareThreshold) {
                         rev = r;
                         aiMax = ai;
-                    } else if (!findMax && r.compareTo(rev) >= 0) {
+                    } else if (!findMax && r.compareTo(rev) >= compareThreshold) {
                         // We found one already installed.
                         return null;
                     }
@@ -813,6 +826,9 @@ class SdkUpdaterLogic {
             }
         }
 
+        // Because of previews, we can have more than 1 choice, so get the local max.
+        FullRevision localRev = rev;
+        ArchiveInfo localAiMax = null;
         // Look in archives already scheduled for install
         for (ArchiveInfo ai : outArchives) {
             Archive a = ai.getNewArchive();
@@ -820,43 +836,61 @@ class SdkUpdaterLogic {
                 Package p = a.getParentPackage();
                 if (p instanceof PlatformToolPackage) {
                     FullRevision r = ((PlatformToolPackage) p).getRevision();
-                    if (findMax && r.compareTo(rev) > 0) {
-                        rev = r;
-                        aiMax = ai;
-                    } else if (!findMax && r.compareTo(rev) >= 0) {
-                        // The dependency is already scheduled for install, nothing else to do.
-                        return ai;
+                    if (r.compareTo(localRev) >= compareThreshold) {
+                        localRev = r;
+                        localAiMax = ai;
                     }
                 }
             }
         }
+        if (localAiMax != null) {
+            if (findMax) {
+                rev = localRev;
+                aiMax = localAiMax;
+            } else {
+                // The dependency is already scheduled for install, nothing else to do.
+                return localAiMax;
+            }
+        }
+
 
         // Otherwise look in the selected archives.
+        localRev = rev;
+        Archive localAMax = null;
         if (selectedArchives != null) {
             for (Archive a : selectedArchives) {
                 Package p = a.getParentPackage();
                 if (p instanceof PlatformToolPackage) {
                     FullRevision r = ((PlatformToolPackage) p).getRevision();
-                    if (findMax && r.compareTo(rev) > 0) {
-                        rev = r;
-                        aiMax = null;
-                        aMax = a;
-                    } else if (!findMax && r.compareTo(rev) >= 0) {
-                        // It's not already in the list of things to install, so add it now
-                        return insertArchive(a,
-                                outArchives,
-                                selectedArchives,
-                                remotePkgs,
-                                remoteSources,
-                                localArchives,
-                                true /*automated*/);
+                    if (r.compareTo(localRev) >= compareThreshold) {
+                        localRev = r;
+                        localAiMax = null;
+                        localAMax = a;
                     }
                 }
             }
+            if (localAMax != null) {
+                if (findMax) {
+                    rev = localRev;
+                    aiMax = null;
+                    aMax = localAMax;
+                } else {
+                    // It's not already in the list of things to install, so add it now
+                    return insertArchive(localAMax,
+                            outArchives,
+                            selectedArchives,
+                            remotePkgs,
+                            remoteSources,
+                            localArchives,
+                            true /*automated*/);
+                }
+            }
         }
 
         // Finally nothing matched, so let's look at all available remote packages
         fetchRemotePackages(remotePkgs, remoteSources);
+        localRev = rev;
+        localAMax = null;
         for (Package p : remotePkgs) {
             if (p instanceof PlatformToolPackage) {
                 FullRevision r = ((PlatformToolPackage) p).getRevision();
@@ -864,26 +898,34 @@ class SdkUpdaterLogic {
                     // Make sure there's at least one valid archive here
                     for (Archive a : p.getArchives()) {
                         if (a.isCompatible()) {
-                            if (findMax && r.compareTo(rev) > 0) {
-                                rev = r;
-                                aiMax = null;
-                                aMax = a;
-                            } else if (!findMax && r.compareTo(rev) >= 0) {
-                                // It's not already in the list of things to install, so add the
-                                // first compatible archive we can find.
-                                return insertArchive(a,
-                                        outArchives,
-                                        selectedArchives,
-                                        remotePkgs,
-                                        remoteSources,
-                                        localArchives,
-                                        true /*automated*/);
+                            if (r.compareTo(localRev) >= compareThreshold) {
+                                localRev = r;
+                                localAiMax = null;
+                                localAMax = a;
+                                break;
                             }
                         }
                     }
                 }
             }
         }
+        if (localAMax != null) {
+            if (findMax) {
+                rev = localRev;
+                aiMax = null;
+                aMax = localAMax;
+            } else {
+                // It's not already in the list of things to install, so add the
+                // first compatible archive we can find.
+                return insertArchive(localAMax,
+                        outArchives,
+                        selectedArchives,
+                        remotePkgs,
+                        remoteSources,
+                        localArchives,
+                        true /*automated*/);
+            }
+        }
 
         if (findMax) {
             if (aMax != null) {
diff --git a/sdkmanager/libs/sdkuilib/tests/com/android/sdkuilib/internal/repository/UpdaterLogicTest.java b/sdkmanager/libs/sdkuilib/tests/com/android/sdkuilib/internal/repository/SdkUpdaterLogicTest.java
index 7918885..6a93914 100755
--- a/sdkmanager/libs/sdkuilib/tests/com/android/sdkuilib/internal/repository/UpdaterLogicTest.java
+++ b/sdkmanager/libs/sdkuilib/tests/com/android/sdkuilib/internal/repository/SdkUpdaterLogicTest.java
@@ -21,6 +21,7 @@ import com.android.sdklib.internal.avd.AvdManager;
 import com.android.sdklib.internal.repository.DownloadCache;
 import com.android.sdklib.internal.repository.ITaskFactory;
 import com.android.sdklib.internal.repository.archives.Archive;
+import com.android.sdklib.internal.repository.packages.FullRevision;
 import com.android.sdklib.internal.repository.packages.MockAddonPackage;
 import com.android.sdklib.internal.repository.packages.MockBrokenPackage;
 import com.android.sdklib.internal.repository.packages.MockPlatformPackage;
@@ -41,7 +42,7 @@ import java.util.List;
 
 import junit.framework.TestCase;
 
-public class UpdaterLogicTest extends TestCase {
+public class SdkUpdaterLogicTest extends TestCase {
 
     private static class NullUpdaterData implements IUpdaterData {
 
@@ -87,10 +88,10 @@ public class UpdaterLogicTest extends TestCase {
 
     }
 
-    private static class MockUpdaterLogic extends SdkUpdaterLogic {
+    private static class MockSdkUpdaterLogic extends SdkUpdaterLogic {
         private final Package[] mRemotePackages;
 
-        public MockUpdaterLogic(IUpdaterData updaterData, Package[] remotePackages) {
+        public MockSdkUpdaterLogic(IUpdaterData updaterData, Package[] remotePackages) {
             super(updaterData);
             mRemotePackages = remotePackages;
         }
@@ -112,7 +113,7 @@ public class UpdaterLogicTest extends TestCase {
      * can find the base platform for a given addon.
      */
     public void testFindAddonDependency() {
-        MockUpdaterLogic mul = new MockUpdaterLogic(new NullUpdaterData(), null);
+        MockSdkUpdaterLogic mul = new MockSdkUpdaterLogic(new NullUpdaterData(), null);
 
         MockPlatformPackage p1 = new MockPlatformPackage(1, 1);
         MockPlatformPackage p2 = new MockPlatformPackage(2, 1);
@@ -154,7 +155,7 @@ public class UpdaterLogicTest extends TestCase {
      * platform package for a given broken add-on package.
      */
     public void testFindExactApiLevelDependency() {
-        MockUpdaterLogic mul = new MockUpdaterLogic(new NullUpdaterData(), null);
+        MockSdkUpdaterLogic mul = new MockSdkUpdaterLogic(new NullUpdaterData(), null);
 
         MockPlatformPackage p1 = new MockPlatformPackage(1, 1);
         MockPlatformPackage p2 = new MockPlatformPackage(2, 1);
@@ -202,7 +203,7 @@ public class UpdaterLogicTest extends TestCase {
      * tool package for a given platform package.
      */
     public void testFindPlatformDependency() {
-        MockUpdaterLogic mul = new MockUpdaterLogic(new NullUpdaterData(), null);
+        MockSdkUpdaterLogic mul = new MockSdkUpdaterLogic(new NullUpdaterData(), null);
 
         MockPlatformToolPackage pt1 = new MockPlatformToolPackage(1);
 
@@ -245,7 +246,7 @@ public class UpdaterLogicTest extends TestCase {
      * platform-tool package for a given tool package.
      */
     public void testFindPlatformToolDependency() {
-        MockUpdaterLogic mul = new MockUpdaterLogic(new NullUpdaterData(), null);
+        MockSdkUpdaterLogic mul = new MockSdkUpdaterLogic(new NullUpdaterData(), null);
 
         MockPlatformToolPackage t1 = new MockPlatformToolPackage(1);
         MockPlatformToolPackage t2 = new MockPlatformToolPackage(2);
@@ -306,7 +307,7 @@ public class UpdaterLogicTest extends TestCase {
 
         // Note: the mock updater logic gets the remotes packages from the array given
         // here and bypasses the source (to avoid fetching any actual URLs)
-        MockUpdaterLogic mul = new MockUpdaterLogic(new NullUpdaterData(),
+        MockSdkUpdaterLogic mul = new MockSdkUpdaterLogic(new NullUpdaterData(),
                 new Package[] { t8, pt2, t9, pt3, p9 });
 
         SdkSources sources = new SdkSources();
@@ -337,7 +338,7 @@ public class UpdaterLogicTest extends TestCase {
 
         // Now try again but reverse the order of the remote package list.
 
-        mul = new MockUpdaterLogic(new NullUpdaterData(),
+        mul = new MockSdkUpdaterLogic(new NullUpdaterData(),
                 new Package[] { p9, t9, pt3, t8, pt2 });
 
         selected = mul.computeUpdates(
@@ -363,4 +364,132 @@ public class UpdaterLogicTest extends TestCase {
                  "SDK Platform Android android-9, API 9, revision 1]",
                 Arrays.toString(selected.toArray()));
     }
+
+    public void testComputeRevisionUpdate2() {
+        // Scenario:
+        // - user has tools rev 2 installed and NO platform-tools
+        // - server has platform tools 1 rc 1 (a preview) and 2.
+        // - server has platform 2 that requires min-tools 2 that requires min-plat-tools 1rc1.
+        //
+        // One issue is that when there was only one instance of platform-tools possible,
+        // the computeUpdates() code would pick the first one. But now there can be 2 of
+        // them (preview, non-preview) and thus we need to pick up the higher one even if
+        // it's not the first choice.
+
+        final MockPlatformToolPackage pt1rc = new MockPlatformToolPackage(
+                                                    null,
+                                                    new FullRevision(1, 0, 0, 1));
+        final MockPlatformToolPackage pt2 = new MockPlatformToolPackage(2);
+
+        // Tools rev 2 requires at least plat-tools 1rc1
+        final MockToolPackage t2 = new MockToolPackage(null,
+                                                       new FullRevision(2),           // tools rev
+                                                       new FullRevision(1, 0, 0, 1)); // min-pt-rev
+
+        final MockPlatformPackage p2 = new MockPlatformPackage(2, 1, 2 /*min-tools*/);
+
+        // Note: the mock updater logic gets the remotes packages from the array given
+        // here and bypasses the source (to avoid fetching any actual URLs)
+        // Remote available packages include both plat-tools 1rc1 and 2.
+        //
+        // Order DOES matter: the issue is that computeUpdates was selecting the first platform
+        // tools (so 1rc1) and ignoring the newer revision 2 because originally there could be
+        // only one platform-tool definition. Now with previews we can have 2 and we need to
+        // select the higher one even if it's not the first choice.
+        MockSdkUpdaterLogic mul = new MockSdkUpdaterLogic(new NullUpdaterData(),
+                new Package[] { t2, pt1rc, pt2, p2 });
+
+        // Local packages only have tools 2.
+        SdkSources sources = new SdkSources();
+        Package[] localPkgs = { t2 };
+        List<Archive> selectedArchives = Arrays.asList( p2.getArchives() );
+
+        List<ArchiveInfo> selected = mul.computeUpdates(
+                selectedArchives,
+                sources,
+                localPkgs,
+                false /*includeObsoletes*/);
+
+        assertEquals(
+                "[SDK Platform Android android-2, API 2, revision 1, " +
+                 "Android SDK Platform-tools, revision 2]",
+                Arrays.toString(selected.toArray()));
+
+        mul.addNewPlatforms(
+                selected,
+                sources,
+                localPkgs,
+                false /*includeObsoletes*/);
+
+        assertEquals(
+                "[SDK Platform Android android-2, API 2, revision 1, " +
+                 "Android SDK Platform-tools, revision 2]",
+                Arrays.toString(selected.toArray()));
+    }
+
+    public void testComputeRevisionUpdate3() {
+        // Scenario:
+        // - user has tools rev 2 installed and NO platform-tools
+        // - server has platform tools 1 rc 1 (a preview) and 2.
+        // - server has platform 2 that requires min-tools 2 that requires min-plat-tools 1rc1.
+        //
+        // One issue is that when there was only one instance of tools possible,
+        // the computeUpdates() code would pick the first one. But now there can be 2 of
+        // them (preview, non-preview) and thus we need to pick up the higher one even if
+        // it's not the first choice.
+
+        final MockPlatformToolPackage pt1rc = new MockPlatformToolPackage(
+                                                    null,
+                                                    new FullRevision(1, 0, 0, 1));
+        final MockPlatformToolPackage pt2 = new MockPlatformToolPackage(2);
+
+        // Tools rev 1rc1 requires plat-tools 1rc1, and tools 2 requires plat-tools 2.
+        final MockToolPackage t1rc = new MockToolPackage(null,
+                                                       new FullRevision(1, 0, 0, 1),  // tools rev
+                                                       new FullRevision(1, 0, 0, 1)); // min-pt-rev
+        final MockToolPackage t2 = new MockToolPackage(null, 2, 2);
+
+        // Platform depends on min-tools 1rc1, so any of tools 1rc1 or 2 would satisfy.
+        final MockPlatformPackage p2 = new MockPlatformPackage(2, 1, new FullRevision(1, 0, 0, 1));
+
+        // Note: the mock updater logic gets the remotes packages from the array given
+        // here and bypasses the source (to avoid fetching any actual URLs)
+        // Remote available packages include both plat-tools 1rc1 and 2.
+        //
+        // Order DOES matter: the issue is that computeUpdates was selecting the first tools (1rc1)
+        // and ignoring the newer revision 2 because originally there could be only one tool
+        // definition. Now with previews we can have 2 and we need to select the higher version
+        // available even if it's not the first choice.
+        MockSdkUpdaterLogic mul = new MockSdkUpdaterLogic(new NullUpdaterData(),
+                new Package[] { t1rc, pt1rc, t2, pt2, p2 });
+
+        // Local packages only have tools 2.
+        SdkSources sources = new SdkSources();
+        Package[] localPkgs = {  };
+        List<Archive> selectedArchives = Arrays.asList( p2.getArchives() );
+
+        List<ArchiveInfo> selected = mul.computeUpdates(
+                selectedArchives,
+                sources,
+                localPkgs,
+                false /*includeObsoletes*/);
+
+        assertEquals(
+                "[Android SDK Platform-tools, revision 2, " +
+                 "Android SDK Tools, revision 2, " +
+                 "SDK Platform Android android-2, API 2, revision 1]",
+                Arrays.toString(selected.toArray()));
+
+        mul.addNewPlatforms(
+                selected,
+                sources,
+                localPkgs,
+                false /*includeObsoletes*/);
+
+        assertEquals(
+                "[Android SDK Platform-tools, revision 2, " +
+                 "Android SDK Tools, revision 2, " +
+                 "SDK Platform Android android-2, API 2, revision 1]",
+                Arrays.toString(selected.toArray()));
+    }
 }