Code review comment for lp:~ted/ubuntu-app-launch/click-exec

On Wed, Jul 24, 2013 at 04:22:37PM -0000, Ted Gould wrote:
> On Wed, 2013-07-24 at 07:22 +0000, Colin Watson wrote:
> > + cd ${HOME}/.cache/upstart-app-launch/desktop/${APP_ID}
> >
> > ${HOME}/.cache/upstart-app-launch/desktop/${APP_ID} is a symlink to the .desktop file, not a directory. You need to get the package directory instead and cd to that. Something like:
> >
> > CLICK_DIR="$(click pkgdir "${HOME}/.cache/upstart-app-launch/desktop/${APP_ID}")"
> > cd "$CLICK_DIR"
>
> That's really confusing. Doesn't that mean that the click packaging
> system knows about desktop files then? Why would it be dependent on the
> representation of the data?

No, it doesn't know about desktop files and is not dependent on the
representation of the data. With the exception of the temporary "click
desktophook" command, there is absolutely nothing in click with any
knowledge of desktop files; that was a design requirement I was very
clear on from the beginning, and you can verify the isolation for
yourself with grep.

You have to declare the location of the desktop file in the manifest one
way or another, and that information clearly has to be passed to the
hook somehow. It's simplest to pass that by way of linking to the
desktop file whose location is declared in the manifest. This means
that in the simple case where you don't need any postprocessing but only
need to drop the file that's the object of the hook somewhere, you can
write a hook that doesn't need an Exec line at all because it can just
tell click to symlink the object into the right place. It also
simplifies hooks that just need to do fairly simple sed-style
postprocessing because all they need to do is walk two directories,
apply substitutions to any files that are missing in the target
directory or that are newer in the source than the target, and remove
any files from the target that aren't in the source.

If you need to get the directory from the
hook-object-file-within-directory, you can always use "click pkgdir" to
get it; but I think it's likely that quite a few simple hooks won't need
to care about this, and in any case linking to the
hook-object-file-within-directory is more informative in that it's
easier to go from that path to the containing package directory than it
is to take the containing directory, parse the manifest to find the
entry for the hook in question, walk all its applications, and find each
of their hook-object-files. The latter approach makes particularly
little sense to me given that click has just had to do much the same
parsing itself to figure out which hooks to execute.

> > +pre-start exec @libexecdir@/desktop-exec ${APP_ID} "${APP_URIS}"
> >
> > It is odd to pass a list of URIs as a single argument separated by spaces rather than just letting the shell's word-splitting sort it out. Did you really mean to quote ${APP_URIS}? (The C code would need to be adjusted to take URIs as more than one element of argv, but that would seem much more normal anyway.) On the other hand, ${APP_ID} should be quoted.
>
>
> Yes, I've struggled with how that should be done. Since it is always
> passed through the Upstart jobs as a single variable it seemed weird to
> have the shell expand it in just that case. For instance, you can't
> start a job without making it a single variable. Consistency between
> what was passed into the job and what was passed to the exec script
> seemed logical.

OK. It seemed counterintuitive to me, but if you need it this way for
consistency with something else then that's fair enough.