Description

I had a SimCity activity with activity_version = 1 installed. I downloaded a new one with activity_version = 2. It was not installed; instead, when I resumed it from the journal, the old version 1 copy was resumed.

This seems to be a critical bug for Ship.1; it would mean that buggy activities couldn't get replaced by fixed ones.

There are two cases that may be handled differently. The existing activity could be in /usr/share/activities if it came with the OS. Or the existing activity could be in /home/olpc/Activities if it was previously downloaded. In both cases, the higher-version application should be the one that runs when you resume that .xo file from the journal. (And I think it should replace the icon on the bottom bar, so new invocations will get the new version.)

Whether installing a new version should physically remove the older version (freeing up its scarce flash space) is a separate question. If it doesn't get deleted, how and when WILL it ever get deleted? If it doesn't get deleted, is there any way to access it to run it?
If you use the GUI to remove the new version, does the old version become accessible again?

This is a new feature (upgrading was never supported) and I agree it's an important one. Though we are supposed to have a process in place for new features, I don't understand why we are *never* following it.

I suggest running this through the process. Feel free to mark it Update.1 again if I'm missing something.

It should be possible to make ~/Activities have priority over /usr/share/activities (and I would consider this a defect rather than an enh), I can explore this possibility if wanted. For manually installed .xo you can delete them from the journal and then install a new version.

Part of the reason I want this to go through the enh process is that it's not really clear what we will be the story with system/default activities for Update.1 and that's strictly related to this ticket.

Attached a patch that allows an upgrade of an installed activity be doing an uninstall() of the old one, and then an install() for the new one. uninstall() will only be available for activities in the user homedir. The model of having only a few core/bundled activities living in /usr/share/activities and the majority in /home/olpc/Activities makes sense to me: bundled activities will be upgraded with a system upgrade; other activities at will (and with system upgrade). Note that it is possible to install a newer version of a bundled activity to /home/olpc/Activities while having an older version in /usr/share/activities; not sure what would happen then, and I agree with Marco that this is undesired.

This method has a few concerns that I'd like to address:
# Should we also allow to downgrade activities (I think not)
# Is there data in /home/olpc/Activities/<myactivitiy>/ that needs to be retained across upgrades? (/data or /config or something).

A different method would be to just extract the new bundle in the same directory as the old bundle. It will take care of issue 2, but could cause some trouble if mimetype handling changes over different versions (although this can of course be solved).

We are already invalidating the cache on ActivityRemoved and ActivityAdded. That's not enough?

+ def compare_version(self):
+ """Compare this bundle with installed bundle.
+ Returns -1 if older, 0 if same and 1 if newer"""

This API looks a bit too cumbersome to me. I would prefer if from the name of the method it was clear how it needs to be used. It also is currently only used by need_upgrade(), do you have any other use case in mind? If not, I would just merge compare_version() into need_upgrade().

Why add this method to the public API? AFAICS, that functionality is already provided by the activity registry, so that method is just a convenience one. But I don't see how this method is called by other classes. I would move that method to be private if it makes the code clearer, or just merge into upgrade() if not.

- def uninstall(self):
+ def uninstall(self, install_path=None):

Why do we need to add that install_path arg to a public method? Any class outside from ActivityBundle will need to pass an install_path that ActivityBundle itself cannot determine? If possible, I would maintain the API and just add logic inside uninstall() to calculate the correct install_path to use.

Attached new patch: cleaned up mostly like tomeu suggested, and improved logic for handling system/non-system activities. Also removing an old bundle from the Journal does not uninstall a newer installed bundle. Now I think of it, the bundle should only be uninstalled if the version matches.

Verified as working in update-1 build 665. Note again: not possible to upgrade activities installed in /usr/share. Maybe some of them should be moved to ~/Activities to make that possible; the other ones will be upgraded with a system upgrade.