Packages: handle catalog versions better

When a catalog version is stale, disable it but don’t remove it,
and still try to refresh it. This should give much better behaviour
when the FG version changes, should behave as users expect.

simgear/package/Catalog.cxx

@@ -31,6 +31,7 @@
 #include <simgear/package/Package.hxx>
 #include <simgear/package/Root.hxx>
 #include <simgear/package/Install.hxx>
+#include <simgear/misc/strutils.hxx>
 
 namespace simgear {
 
@@ -40,18 +41,32 @@ bool checkVersion(const std::string& aVersion, SGPropertyNode_ptr props)
 {
     BOOST_FOREACH(SGPropertyNode* v, props->getChildren("version")) {
         std::string s(v->getStringValue());
-        if (s== aVersion) {
+        if (s == aVersion) {
             return true;
         }
 
-        // allow 3.5.* to match any of 3.5.0, 3.5.1rc1, 3.5.11 or so on
+        // examine each dot-seperated component in turn, supporting a wildcard
-        if (strutils::ends_with(s, ".*")) {
+        // in the versions from the catalog.
-            size_t lastDot = aVersion.rfind('.');
+        string_list appVersionParts = simgear::strutils::split(aVersion, ".");
-            std::string ver = aVersion.substr(0, lastDot);
+        string_list catVersionParts = simgear::strutils::split(s, ".");
-            if (ver == s.substr(0, s.length() - 2)) {
+
-                return true;
+        size_t partCount = appVersionParts.size();
+        if (partCount != catVersionParts.size()) {
+            continue;
+        }
+
+        bool ok = true;
+        for (unsigned int p=0; p < partCount; ++p) {
+            if (catVersionParts[p] == "*") {
+                // always passes
+            } else if (appVersionParts[p] != catVersionParts[p]) {
+                ok = false;
             }
         }
+
+        if (ok) {
+            return true;
+        }
     }
     return false;
 }
@@ -196,22 +211,13 @@ CatalogRef Catalog::createFromPath(Root* aRoot, const SGPath& aPath)
         return NULL;
     }
 
-    if (!checkVersion(aRoot->applicationVersion(), props)) {
+    bool versionCheckOk = checkVersion(aRoot->applicationVersion(), props);
-        std::string redirect = redirectUrlForVersion(aRoot->applicationVersion(), props);
-        if (!redirect.empty()) {
-            SG_LOG(SG_GENERAL, SG_WARN, "catalog at " << aPath << ", version mismatch:\n\t"
-                   << "redirecting to alternate URL:" << redirect);
-            CatalogRef c = Catalog::createFromUrl(aRoot, redirect);
-            c->m_installRoot = aPath;
-            return c;
-        } else {
-            SG_LOG(SG_GENERAL, SG_WARN, "skipping catalog at " << aPath << ", version mismatch:\n\t"
-               << props->getStringValue("version") << " vs required " << aRoot->catalogVersion());
-            return NULL;
-        }
 
+    if (!versionCheckOk) {
+        SG_LOG(SG_GENERAL, SG_INFO, "catalog at:" << aPath << " failed version check: need" << aRoot->applicationVersion());
+        // keep the catalog but mark it as needing an update
     } else {
-        SG_LOG(SG_GENERAL, SG_INFO, "creating catalog from:" << aPath);
+        SG_LOG(SG_GENERAL, SG_DEBUG, "creating catalog from:" << aPath);
     }
 
     CatalogRef c = new Catalog(aRoot);
@@ -219,8 +225,12 @@ CatalogRef Catalog::createFromPath(Root* aRoot, const SGPath& aPath)
     c->parseProps(props);
     c->parseTimestamp();
 
-    // parsed XML ok, mark status as valid
+    if (versionCheckOk) {
-    c->changeStatus(Delegate::STATUS_SUCCESS);
+        // parsed XML ok, mark status as valid
+        c->changeStatus(Delegate::STATUS_SUCCESS);
+    } else {
+        c->changeStatus(Delegate::FAIL_VERSION);
+    }
 
     return c;
 }
@@ -469,6 +479,11 @@ unsigned int Catalog::ageInSeconds() const
 
 bool Catalog::needsRefresh() const
 {
+    // always refresh in these cases
+    if ((m_status == Delegate::FAIL_VERSION) || (m_status == Delegate::FAIL_DOWNLOAD)) {
+        return true;
+    }
+
     unsigned int maxAge = m_props->getIntValue("max-age-sec", m_root->maxAgeSeconds());
     return (ageInSeconds() > maxAge);
 }

simgear/package/Root.cxx

@@ -159,6 +159,7 @@ public:
     std::string locale;
     HTTP::Client* http;
     CatalogDict catalogs;
+    CatalogList disabledCatalogs;
     unsigned int maxAgeSeconds;
     std::string version;
 
@@ -240,10 +241,16 @@ Root::Root(const SGPath& aPath, const std::string& aVersion) :
         return;
     }
 
-    BOOST_FOREACH(SGPath c, dir.children(Dir::TYPE_DIR)) {
+    BOOST_FOREACH(SGPath c, dir.children(Dir::TYPE_DIR | Dir::NO_DOT_OR_DOTDOT)) {
         CatalogRef cat = Catalog::createFromPath(this, c);
         if (cat) {
-           d->catalogs[cat->id()] = cat;
+            if (cat->status() == Delegate::STATUS_SUCCESS) {
+                d->catalogs[cat->id()] = cat;
+            } else {
+                // catalog has problems, such as needing an update
+                // keep it out of the main collection for now
+                d->disabledCatalogs.push_back(cat);
+            }
         }
     } // of child directories iteration
 }
@@ -357,12 +364,23 @@ Root::packagesNeedingUpdate() const
 void Root::refresh(bool aForce)
 {
     bool didStartAny = false;
+
+    // copy all candidate ctalogs to a seperate list, since refreshing
+    // can modify both the main collection and/or the disabled list
+    CatalogList toRefresh;
     CatalogDict::iterator it = d->catalogs.begin();
     for (; it != d->catalogs.end(); ++it) {
-        if (aForce || it->second->needsRefresh()) {
+        toRefresh.push_back(it->second);
-            it->second->refresh();
+    }
-            didStartAny = true;
+
-        }
+    toRefresh.insert(toRefresh.end(), d->disabledCatalogs.begin(),
+                     d->disabledCatalogs.end());
+
+
+    CatalogList::iterator j = toRefresh.begin();
+    for (; j != toRefresh.end(); ++j) {
+        (*j)->refresh();
+        didStartAny =  true;
     }
 
     if (!didStartAny) {
@@ -486,21 +504,35 @@ void Root::catalogRefreshStatus(CatalogRef aCat, Delegate::StatusCode aReason)
         d->refreshing.erase(aCat);
     }
 
-    if ((aReason != Delegate::STATUS_REFRESHED) && (aReason != Delegate::STATUS_IN_PROGRESS)) {
-        // if the failure is permanent, delete the catalog from our
-        // list (don't touch it on disk)
-        bool isPermanentFailure = (aReason == Delegate::FAIL_VERSION);
-        if (isPermanentFailure) {
-            SG_LOG(SG_GENERAL, SG_WARN, "permanent failure for catalog:" << aCat->id());
-            if (catIt != d->catalogs.end()) {
-                d->catalogs.erase(catIt);
-            }
-        }
-    }
-
     if ((aReason == Delegate::STATUS_REFRESHED) && (catIt == d->catalogs.end())) {
         assert(!aCat->id().empty());
         d->catalogs.insert(catIt, CatalogDict::value_type(aCat->id(), aCat));
+
+        // catalog might have been previously disabled, let's remove in that case
+        CatalogList::iterator j = std::find(d->disabledCatalogs.begin(),
+                                            d->disabledCatalogs.end(),
+                                            aCat);
+        if (j != d->disabledCatalogs.end()) {
+            SG_LOG(SG_GENERAL, SG_INFO, "re-enabling disabled catalog:" << aCat->id());
+            d->disabledCatalogs.erase(j);
+        }
+    }
+
+    if ((aReason != Delegate::STATUS_REFRESHED) &&
+        (aReason != Delegate::STATUS_IN_PROGRESS) &&
+        (aReason != Delegate::STATUS_SUCCESS))
+    {
+        // catalog has errors, disable it
+        CatalogList::iterator j = std::find(d->disabledCatalogs.begin(),
+                                            d->disabledCatalogs.end(),
+                                            aCat);
+        if (j == d->disabledCatalogs.end()) {
+            SG_LOG(SG_GENERAL, SG_INFO, "disabling catalog:" << aCat->id());
+            d->disabledCatalogs.push_back(aCat);
+        }
+
+        // and remove it from the active collection
+        d->catalogs.erase(catIt);
     }
 
     if (d->refreshing.empty()) {
@@ -511,17 +543,31 @@ void Root::catalogRefreshStatus(CatalogRef aCat, Delegate::StatusCode aReason)
 
 bool Root::removeCatalogById(const std::string& aId)
 {
+    CatalogRef cat;
+
     CatalogDict::iterator catIt = d->catalogs.find(aId);
     if (catIt == d->catalogs.end()) {
-        SG_LOG(SG_GENERAL, SG_WARN, "removeCatalogById: unknown ID:" << aId);
+        // check the disabled list
-        return false;
+        CatalogList::iterator j = d->disabledCatalogs.begin();
+        for (; j != d->disabledCatalogs.end(); ++j) {
+            if ((*j)->id() == aId) {
+                break;
+            }
+        }
+
+        if (j == d->disabledCatalogs.end()) {
+            SG_LOG(SG_GENERAL, SG_WARN, "removeCatalogById: no catalog with id:" << aId);
+            return false;
+        }
+
+        cat = *j;
+        d->disabledCatalogs.erase(j);
+    } else {
+        cat = catIt->second;
+        // drop the reference
+        d->catalogs.erase(catIt);
     }
 
-    CatalogRef cat = catIt->second;
-
-    // drop the reference
-    d->catalogs.erase(catIt);
-
     bool ok = cat->uninstall();
     if (!ok) {
         SG_LOG(SG_GENERAL, SG_WARN, "removeCatalogById: catalog :" << aId