Merge lp:~aaronp/software-center/usc-605048 into lp:software-center

[Aaron Peachey]

100 papercut fixes for bug 605048 and 686994.
Alters db update behaviour to stop 'rebuilding' window appearing while xapian db is being updated on detecting an externally changed cahce.
Instead, updates a new copy of the xapian db, then once finished, renamed to be the new xapian db.
Changes made in app.py and db/update.py.
Test by running "sudo update-software-center"

[Michael Vogt (mvo)]

On Sun, Dec 19, 2010 at 06:14:13AM -0000, Aaron Peachey wrote:
> Aaron Peachey has proposed merging lp:~aaronp/software-center/usc-605048 into lp:software-center.
>
> Requested reviews:
> Michael Vogt (mvo)
> Related bugs:
> #605048 "Rebuilding application catalogue" uses a separate window
> https://bugs.launchpad.net/bugs/605048
> #686994 When 'Rebuilding Software Catalogue', there is no indication that anything is happening or when it will end.
> https://bugs.launchpad.net/bugs/686994
>
>
> 100 papercut fixes for bug 605048 and 686994.
> Alters db update behaviour to stop 'rebuilding' window appearing while xapian db is being updated on detecting an externally changed cahce.
> Instead, updates a new copy of the xapian db, then once finished, renamed to be the new xapian db.
> Changes made in app.py and db/update.py.
> Test by running "sudo update-software-center"

Thanks, the branch looks fine. I will merge it, but I need to ask
first that you please sign the canonical contributors agreement, the
details are here:
http://www.canonical.com/contributors
its quick and painless.

Some small comments below:

> === modified file 'softwarecenter/app.py'
> --- softwarecenter/app.py 2010-12-14 18:57:11 +0000
> +++ softwarecenter/app.py 2010-12-19 06:13:57 +0000
> @@ -851,25 +852,12 @@
[..]
> - if is_rebuilding:
> - self.window_rebuilding.show()
[..]

Nice that we can get rid of this :)

> === modified file 'softwarecenter/db/update.py'
> --- softwarecenter/db/update.py 2010-12-15 14:02:40 +0000
> +++ softwarecenter/db/update.py 2010-12-19 06:13:57 +0000
> @@ -592,15 +594,39 @@
>
> def rebuild_database(pathname):
> cache = apt.Cache(memonly=True)
> + old_path = os.path.join(XAPIAN_BASE_PATH, "xapian_old")
> + rebuild_path = os.path.join(XAPIAN_BASE_PATH, "xapian_rb")

Its probably better to build this relative to pathname, e.g. by
just adding "_old" and "_rb" to the base pathname

> + if not os.path.exists(rebuild_path):
> + try:
> + os.makedirs(rebuild_path)
> + except:
> + LOG.warn("Problem creating rebuild path '%s'." % rebuild_path)
> + LOG.warn("Please check you have the relevant permissions.")
> + return False
> +
> # check permission
> if not os.access(pathname, os.W_OK):
> LOG.warn("Cannot write to '%s'." % pathname)
> LOG.warn("Please check you have the relevant permissions.")
> return False
> +
> + #check if old unrequired version of db still exists on filesystem
> + if os.path.exists(old_path):
> + LOG.warn("Existing xapian old db was not previously cleaned: '%s'." % old_path)
> + if os.access(old_path, os.W_OK):
> + #remove old unrequired db before beginning
> + shutil.rmtree(old_path, False, None)
> + else:
> + LOG.warn("Cannot write to '%s'." % old_path)
> + LOG.warn("Please check you have the relevant permissions.")
> + return False
> +
> +

Nice defensive style!

> # write it
> - db = xapian.WritableDatabase(pathname, xapian.DB_CREATE_OR_OVERWRITE)
> + db = xapian.WritableDatabase(...

[Michael Vogt (mvo)]

On Sun, Dec 19, 2010 at 06:14:13AM -0000, Aaron Peachey wrote:
Another tiny note:

> + shutil.rmtree(old_path, False, None)

The False and None are default values in the shutil.rmtree, so you can
just write the slightly shorter shutil.rmtree(old_path).

Cheers,
 Michael

[Aaron Peachey (aaronp)]

updated as per review feedback and new changes pushed to branch ready for re-review/merging

[Michael Vogt (mvo)]

Thanks a lot! Merged into trunk :)

review: Approve

=== modified file 'softwarecenter/app.py'
--- softwarecenter/app.py	2010-12-15 19:23:03 +0000
+++ softwarecenter/app.py	2010-12-22 09:39:57 +0000
@@ -134,6 +134,7 @@
         self.backend.connect("channels-changed", self.on_channels_changed)
         # xapian
         pathname = os.path.join(xapian_base_path, "xapian")
+
         try:
             self.db = StoreDatabase(pathname, self.cache)
             self.db.open()
@@ -851,25 +852,12 @@
     def _on_database_rebuilding_handler(self, is_rebuilding):
         self._logger.debug("_on_database_rebuilding_handler %s" % is_rebuilding)
         self._database_is_rebuilding = is_rebuilding
-        self.window_rebuilding.set_transient_for(self.window_main)
 
-        # set a11y text
-        try:
-            text = self.window_rebuilding.get_children()[0]
-            text.set_property("can-focus", True)
-            text.a11y = text.get_accessible()
-            text.a11y.set_name(text.get_children()[0].get_text())
-        except IndexError:
+        if is_rebuilding:
             pass
-
-        self.window_main.set_sensitive(not is_rebuilding)
-        # show dialog about the rebuilding status
-        if is_rebuilding:
-            self.window_rebuilding.show()
         else:
             # we need to reopen when the database finished updating
             self.db.reopen()
-            self.window_rebuilding.hide()
 
     def setup_database_rebuilding_listener(self):
         """

=== modified file 'softwarecenter/db/update.py'
--- softwarecenter/db/update.py	2010-12-15 14:02:40 +0000
+++ softwarecenter/db/update.py	2010-12-22 09:39:57 +0000
@@ -25,11 +25,13 @@
 import os
 import simplejson
 import string
+import shutil
 import sys
 import time
 import urllib
 import xapian
 
+
 from ConfigParser import RawConfigParser, NoOptionError
 from gettext import gettext as _
 from glob import glob
@@ -592,15 +594,39 @@
 
 def rebuild_database(pathname):
     cache = apt.Cache(memonly=True)
+    old_path = pathname+"_old"
+    rebuild_path = pathname+"_rb"
+    
+    if not os.path.exists(rebuild_path):
+        try:
+            os.makedirs(rebuild_path)
+        except:
+            LOG.warn("Problem creating rebuild path '%s'." % rebuild_path)
+            LOG.warn("Please check you have the relevant permissions.")
+            return False
+    
     # check permission
     if not os.access(pathname, os.W_OK):
         LOG.warn("Cannot write to '%s'." % pathname)
         LOG.warn("Please check you have the relevant permissions.")
         return False
+    
+    #check if old unrequired version of db still exists on filesystem
+    if os.path.exists(old_path):
+        LOG.warn("Existing xapian old db was not previously cleaned: '%s'." % old_path)
+        if os.access(old_path, os.W_OK):
+            #remove old unrequired db before beginning
+            shutil.rmtree(old_path)
+        else:
+            LOG.warn("Cannot write to '%s'." % old_path)
+            LOG.warn("Please check you have the relevant permissions.")
+            return False
+
+            
     # write it
-    db = xapian.WritableDatabase(pathname, xapian.DB_CREATE_OR_OVERWRITE)
+    db = xapian.WritableDatabase(rebuild_path, xapian.DB_CREATE_OR_OVERWRITE)
     update(db, cache)
-    # write the database version into the file
+    # write the database version into the filep
     db.set_metadata("db-schema-version", DB_SCHEMA_VERSION)
     # update the mo file stamp for the langpack checks
     mofile = gettext.find("app-install-data")
@@ -608,5 +634,9 @@
         mo_time = os.path.getctime(mofile)
         db.set_metadata("app-install-mo-time", str(mo_time))
     db.flush()
+ 
+    os.rename(pathname, old_path)
+    os.rename(rebuild_path, pathname)
+    shutil.rmtree(old_path)
     return True