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.

diff --git a/simgear/package/Catalog.cxx b/simgear/package/Catalog.cxx
index a867a81e26d96bbd57237952473302b6aa02584e..c9c6565836ae0a4d970fb8c76671e8477fbef496 100644 (file)
--- a/simgear/package/Catalog.cxx
+++ b/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
-        if (strutils::ends_with(s, ".*")) {
-            size_t lastDot = aVersion.rfind('.');
-            std::string ver = aVersion.substr(0, lastDot);
-            if (ver == s.substr(0, s.length() - 2)) {
-                return true;
+        // examine each dot-seperated component in turn, supporting a wildcard
+        // in the versions from the catalog.
+        string_list appVersionParts = simgear::strutils::split(aVersion, ".");
+        string_list catVersionParts = simgear::strutils::split(s, ".");
+
+        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)) {
-        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;
-        }
+    bool versionCheckOk = checkVersion(aRoot->applicationVersion(), props);
 
+    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
-    c->changeStatus(Delegate::STATUS_SUCCESS);
+    if (versionCheckOk) {
+        // 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);
 }

diff --git a/simgear/package/Root.cxx b/simgear/package/Root.cxx
index 13f2b91fcfe69954784363d5344359bcb064c558..7f2e8ea80560bcbd576ee71b1742956151596324 100644 (file)
--- a/simgear/package/Root.cxx
+++ b/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()) {
-            it->second->refresh();
-            didStartAny = true;
-        }
+        toRefresh.push_back(it->second);
+    }
+
+    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,16 +543,30 @@ 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);
-        return false;
-    }
+        // check the disabled list
+        CatalogList::iterator j = d->disabledCatalogs.begin();
+        for (; j != d->disabledCatalogs.end(); ++j) {
+            if ((*j)->id() == aId) {
+                break;
+            }
+        }
 
-    CatalogRef cat = catIt->second;
+        if (j == d->disabledCatalogs.end()) {
+            SG_LOG(SG_GENERAL, SG_WARN, "removeCatalogById: no catalog with id:" << aId);
+            return false;
+        }
 
-    // drop the reference
-    d->catalogs.erase(catIt);
+        cat = *j;
+        d->disabledCatalogs.erase(j);
+    } else {
+        cat = catIt->second;
+        // drop the reference
+        d->catalogs.erase(catIt);
+    }
 
     bool ok = cat->uninstall();
     if (!ok) {