Downloading a higher version of an activity doesn't upgrade the one in the XO

[gnu]

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?

[marco]

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.

[jg]

What's the alternative? is there a feasible way to remove an activity manually?

[bert]

Depends on if you count "rm -r /usr/share/activities/Some.activity" as root "feasible" or not.

[marco]

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.

[jg]

Is this rm command exactly what removing it from the journal will do?

Sounds like a solution for Update.1 to me (I'm presuming that removes all traces of the activity).

I'm only interested in upgrading downloaded activities for Upgrade.1; we can face the problem of sorting out preloaded vs. user loaded activities later.

If so, the Update.1 workaround might be to just warn that you must remove the activity before you can do an installation.

[marco]

It's similar to what the Journal does, but it's for system installed activities.

I really think it's easier to have user installed activities has priority over the system ones than to force people to rm. Also it might be easier to implement than a warning.

[marco]

Given the change in the schedule, I think Update-1 is now a fair milestone, so reverting my change...

[rwh]

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).

Comment and discussion is welcome!

[marco]

Replying to rwh:

This method has a few concerns that I'd like to address: # Should we also allow to downgrade activities (I think not)

We might allow to keep multiple versions of the same activity around at some point, though that's all another can of worms, let's keep it simple for now.

# Is there data in /home/olpc/Activities/<myactivitiy>/ that needs to be retained across upgrades? (/data or /config or something).

Nope, I don't think so. /data and /config are stored separately in the rainbow containers space.

Want a review?

[rwh]

Replying to marco:

# Is there data in /home/olpc/Activities/<myactivitiy>/ that needs to be retained across upgrades? (/data or /config or something).

Nope, I don't think so. /data and /config are stored separately in the rainbow containers space. Want a review?

Yes, then I think this patch is ready for review.

[tomeu]

      def remove_bundle(self, bundle_path): 
+         self._service_name_to_activity_info.clear() 
+         self._mime_type_to_activities.clear() 
          return self._registry.RemoveBundle(bundle_path) 

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().

+     def get_installed_path(self): 
+         act = activity.get_registry().get_activity(self._bundle_id) 
+         if act is None: 
+             return None 
+         else: 
+             return act.path

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.

[rwh]

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.

Replying to tomeu:

{{{ def remove_bundle(self, bundle_path): + self._service_name_to_activity_info.clear() + self._mime_type_to_activities.clear() return self._registry.RemoveBundle(bundle_path) }}} We are already invalidating the cache on ActivityRemoved and ActivityAdded. That's not enough?

No, that's not enough. We get the ActivityRemoved DBus signal only after we check...

[tomeu]

cool

[rwh]

Pushed with minor difference of also allowing downgrades. This makes the user experience better since you actually get the bundle version that you click on. Also included a fix for #5382

[rwh]

Cherry-picked to update-1

[sj]

Note: this is needed for library bundles as well. Filing a separate bug.

[marco]

Built 0.75.4 rpm

[sj]

Replying to sj:

Note: this is needed for library bundles as well. Filing a separate bug.

: see #5533

[marco]

Please test build 655 and close this.

[marco]

665 I mean

[rwh]

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.