Ticket #4906 (closed enhancement: fixed)

Opened 6 years ago

Last modified 6 years ago

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

Reported by: gnu Owned by: rwh
Priority: blocker Milestone: Update.1
Component: sugar Version: 1.0-software-build-623
Keywords: review+ Cc: jg, kimquirk, marco, tomeu, AlexL, mstone
Action Needed: Verified: no
Deployments affected: Blocked By:
Blocking:

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?

Attachments

activity_upgrade.diff (6.5 kB) - added by rwh 6 years ago.

Change History

  Changed 6 years ago by jg

  • priority changed from normal to blocker
  • milestone changed from Never Assigned to Update.1

  Changed 6 years ago by marco

  • cc kimquirk added
  • type changed from defect to enhancement
  • milestone changed from Update.1 to Retriage, Please!

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.

  Changed 6 years ago by jg

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

  Changed 6 years ago by bert

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

  Changed 6 years ago by 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.

  Changed 6 years ago by 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.

  Changed 6 years ago by 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.

  Changed 6 years ago by marco

  • milestone changed from Retriage, Please! to Update.1

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

  Changed 6 years ago by marco

  • owner changed from marco to rwh

follow-up: ↓ 11   Changed 6 years ago by rwh

  • cc marco, tomeu added

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!

in reply to: ↑ 10 ; follow-up: ↓ 12   Changed 6 years ago by 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?

in reply to: ↑ 11   Changed 6 years ago by rwh

  • keywords review? added

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.

follow-up: ↓ 15   Changed 6 years ago by tomeu

  • keywords review- added; review? removed
      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.

  Changed 6 years ago by AlexL

  • cc AlexL added

in reply to: ↑ 13   Changed 6 years ago by rwh

  • keywords review? added; review- removed

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

  Changed 6 years ago by tomeu

  • keywords review+ added; review? removed

cool

  Changed 6 years ago by mstone

  • cc mstone added

Changed 6 years ago by rwh

  Changed 6 years ago by 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

  Changed 6 years ago by rwh

  • owner changed from rwh to marco

Cherry-picked to update-1

follow-up: ↓ 22   Changed 6 years ago by sj

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

  Changed 6 years ago by marco

  • owner changed from marco to rwh

Built 0.75.4 rpm

in reply to: ↑ 20   Changed 6 years ago by sj

Replying to sj:

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

: see #5533

  Changed 6 years ago by marco

Please test build 655 and close this.

  Changed 6 years ago by marco

665 I mean

  Changed 6 years ago by rwh

  • status changed from new to closed
  • resolution set to fixed

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.

Note: See TracTickets for help on using tickets.