Code review comment for lp:~sergiusens/ubuntu-touch-session/install_clicks

On Tue, Aug 27, 2013 at 7:37 AM, Colin Watson <email address hidden> wrote:

> I understand that the world couldn't stop while I was away :-), but it's
> unfortunate that this branch was merged when I was on holiday and didn't
> have the opportunity to comment on the fundamental design issues around
> preinstalled click packages, which is an item on my near-term to-do list
> to improve (it wasn't a priority for the IoM demo and then I was at
> DebConf and then on vacation).
>
> I'm afraid I think this branch takes quite the wrong approach and should
> be reverted for reconsideration, preferably as soon as possible before
> its behaviour becomes entrenched.
>

Reverting it now now would prevent people getting updates for the click
packages when image based upgrading, but I get your point after reading all
of this.

>
> On Thu, Aug 22, 2013 at 10:29:49PM -0500, Ted Gould wrote:
> > On Thu, 2013-08-22 at 22:05 +0000, Loïc Minier wrote:
> > > I guess there are a number of different use cases:
> > > a) package is in app store but we want to preinstall it
> > > b) package is not in app store but we want to preinstall it
> > >
> > > and another subcase:
> > > 1) package may be removed by end-user
> > > 2) package may not be removed by end-user
> > >
> > >
> > >
> > > and what we want to decide is whether we're upgrading/reinstalling
> > > packages and whether we do this on every boot.
> >
> > To be clear, we can't do it on boot because versions are per-user and so
> > are hooks that need to be run by the click system. Also, at boot the
> > user's home directory may not be decrypted.
>
> Quite so.
>

I'm doing everything at session init.

> > > Now some conclusions:
> > > - it seems clear to me that for a), it doesn't matter whether we
> > > update it or not as updates can be delivered from appstore, so we
> > > could optionally skip this on boot
> > > - it seems natural that things that are in appstore and hence can be
> > > installed after the fact should be uninstallable, so that a) should
> > > imply 1)
> > > - it is obvious that in the case of b), we must update it on each boot
> > > - it seems natural that in the case of b), we should not allow removal
> > > -- otherwise lets just put it in the appstore
> >
> > I think that we can't simplify things like this with the per-user
> > installation. There's no reason both users on a system would have the
> > same version of Facebook whether it was pre-installed or not. It's a
> > disk space issue, not an automatic update one. And, as a user, I could
> > not want to update something.
>
> There's really only one decision to take here, and it's whether a
> preinstalled package should be upgraded after a system image update. My
> take on this is that, if the user has installed a newer version, then
> that should be preserved, but otherwise the preinstalled version should
> win. The solution implemented in this branch has the unfortunate
> property that an older version delivered by a system image update will
> take precedence over a newer one installed by the user.
>

I am doing version checking of what's installed against what is provided
though.

> There is a straightforward way to implement this which does not involve
> doing any work at boot time and not much at session init time, but does
> involve some (mostly already planned) extensions to click:
>
> * change click to support multiple base unpack directories rather than
> just /opt/click.ubuntu.com/ (it's already architected to support
> this, with just a few missing pieces), with system configuration for
> the priority order
>

+1

> * add a mechanism in click to register an app for all users in a
> particular base unpack directory (which would update the symlinks in
> e.g. /usr/share/preinstalled-apps/.click/all-users/APP/ - note that
> this does not require knowing the set of users on the system from
> recovery mode)
>

+1

> * add a hook to the system image update mechanism that runs in recovery
> mode (NOT at boot time or in the user session) that does this
> register-for-all-users step for all preinstalled apps
>
> * we can't entirely do without running code in the user session, since
> we need to do things like creating .desktop files; so at session init
> time, check whether there are newly-registered versions of
> preinstalled apps, and if so then run their hooks
>

Sounds reasonable.

 * if a user installs a new version of an app, it is unpacked into the
> last unpack directory in the priority order (probably still
> /opt/click.ubuntu.com/), and automatically shadows the preinstalled
> version on the system partition for that user
>
> * if a user removes an app which they had upgraded from the
> preinstalled version, it is removed from their symlink farm in
> /opt/click.ubuntu.com/.users/, the unpacked copy of the app is
> removed if no other users refer to it, and click automatically falls
> back to the preinstalled version on the system partition
>
> I still need to flesh out some details, but IMO this is far more elegant
> and robust, and is definitely less wasteful of disk space. It has the
> additional benefit that click automatically has all the metadata about
> which apps came from where, and so can inform the UI about things like
> whether an app may be removed.
>

>
> > > So what I'd propose is followings:
> > > * /usr/share/preinstalled/click and /custom/preinstalled/click contain
> > > clicks that aren't in the appstore and must be installed or upgraded
> > > automatically and can't be removed (no flag to allow this for now, but
> > > should add one)
>
> The semantics of these paths as implemented in this branch are entirely
> wrong, I'm afraid. My intent is to provide multiple unpack directories
> for apps, some of which can be in a writable location and some not.
> This makes a lot more sense than shipping the .click files and then
> unpacking them post-boot, which has the effect of using up user space as
> well as system space for preinstalled apps.
>
> > > * /opt/click.u.c is prepopulated in the rootfs with appstore packages
> > > that we want to install by default but may remove, but is never
> > > upgraded (which I think is what happens with writable pathes?)
> >
> > Hmm, I don't think that we should have paths hard coded anywhere but in
> > the click package. So this is, in my opinion a strong vote for putting
> > any upstart job in the click package directly.
>
> I agree. For the time being, anything outside the click package that
> thinks it needs to care about full paths needs special dispensation from
> me, as it's probably wrong and may well cause problems.
>

I agree too, but there is no path here that directly affects click in
itself. If it's counting on /opt/click.ubuntu.com being r/w that's an image
based upgrader thing.

> > We should add a flag for packages that can't be removed (both for
> > > appstore and preinstalled packages).
> >
> > Wouldn't they be on a read-only partition? Isn't that "can't be
> > removed" enough? :-)
>
> Yes, with the exception that we need some work at the UI layer to avoid
> offering a removal option that won't work.
>