snappy,dirs: add support to use desktop files from inside snaps #548

mvo5 · Feb 29, 2016

This adds support for desktop files inside snaps. The files will be exported to /varl/lib/snappy/desktop and the desktop environment (like unity) needs to be tweaked with something like <LegacyDir>/var/lib/snappy/desktop</LegacyDir> in /etc/xdg/menus/unity-lens-applications.menu (or better proper .directory files).

Happy to talk about the best names for the keys. Right now it looks like this:

apps:
 name:
  command: cmd
  desktop: subdir/foo.desktop

but we could also use desktop-file instead of desktop here.

sergiusens · Feb 29, 2016

This looks good; it just deviates from the command for everything paradigm.

zyga · Feb 29, 2016

zyga reviewed Feb 29, 2016

View changes

This comment is not helpful IMHO

zyga · Feb 29, 2016

zyga reviewed Feb 29, 2016

View changes

Don't we want to have version there as well?

zyga · Feb 29, 2016

Some misc comments. If you are happy about not having the version in the generated desktop file name then I'm +1

niemeyer · Feb 29, 2016

Can we have this by filename convention instead?

E.g. if meta/<app-name>.desktop exists (or maybe meta/desktop/<app-name>.desktop), we use it as the desktop file. Would we need the yaml field for other reasons?

sergiusens · Feb 29, 2016

@niemeyer can it be a template instead that uses command as one of its keys? This way we can still launch the app by either the cli or desktop UI.

mvo5 · Feb 29, 2016

@zyga I think its fine without the version, because there is only a single one active at any point in time.

mvo5 · Feb 29, 2016

@niemeyer sure, happy to use the meta/*.desktop convention, that is actually nicer!

mvo5 · Feb 29, 2016

@sergiusens I'm not sure I understand? The recommendation should be that you have:

name: snappypkg
apps:
 foo:
  command: start-foo

and the desktop file would have a matching

[Desktop entry]
Name=foo
Exec=snappypkg_start-foo

In this case the app can be launcher both via CLI and desktop UI. Or am I misreading your question/suggestion?

mvo5 · Feb 29, 2016

I updated the branch to look for meta/*.desktop now.

sergiusens · Feb 29, 2016

I am saying that maybe

Exec could be dynamically generated. I'm more for defining things once. This is from my experience with click where files all over the place had to be changed just because you changed the click name field.

sergiusens · Feb 29, 2016

Given that it has been changed to be done by convention though my suggestion just makes things fragile.

niemeyer · Feb 29, 2016

niemeyer reviewed Feb 29, 2016

View changes

Left over?

niemeyer · Feb 29, 2016

niemeyer reviewed Feb 29, 2016

View changes

cannot

niemeyer · Feb 29, 2016

niemeyer reviewed Feb 29, 2016

View changes

cannot (sorry for the nitpicks)

niemeyer · Feb 29, 2016

LGTM

mvo5 · Feb 29, 2016

One small change: we decided to put the desktop files under meta/gui/*.desktop

mvo5 · Feb 29, 2016

And one more small change: move the icon(s) under meta/gui/ as well.

mvo5 · Feb 29, 2016

Thanks for the feedback. The branch is updated and I addressed the feedback.

3v1n0 · Feb 29, 2016

Hey, I've still to check the internals of this PR, but in the mean time, for here's what I and @mvo5 discussed in order to get proper unity/BAMF support:

On 12/02/2016 13:05, Michael Vogt wrote:

On Tue, Feb 09, 2016 at 09:32:38PM +0100, Marco Trevisan wrote:

the snap ships the desktop files as part of the snap

in meta/snap.yaml we have under "apps" a new "destkop" entry that
points to the relevant file, e.g.
"""
apps:
xbomb:
command: bin/xbomb
desktop: data/xbomb.desktop
"""

Is command still relevant in this case (since there's already the
desktop file?). I guess it might be important for launching the app from
terminal, but xdg-open'ing the .desktop file should be the right thing
to do (once xdg-open is fixed to do that, or we've a proper tool).

We will have to rewrite that file on install and expand variables
like @command@ or something because the apps will not know what
origin they have (e.g. it may be mvo.xbomb or chipaca.xbomb to start
it)

Ok, or maybe just the path? Or you want to define also the parameters
inside the yaml (or things such as the %U, %F or... Elements that might
be replaced on launch with actual parameters?).

on install the desktop file is rewriten and copied into
e.g. /var/lib/snappy/desktop

we make unit look into that snappy desktop file dir

How is this working for different versions of the same app, or different
distributions of it?
Basically, how would be the file paths look?

In order to get better matching of the application with the proper
desktop file it would be nice to have the process basename to match the
desktop-id.

So, probably having something like:
/var/lib/snappy/desktop/applications/mvo/xbomb.desktop
(as per the desktop specs the desktop id would be "mvo-xbomb")
Would help... And thus also having the actual binary to be called
mvo-xbomb would ensure that BAMF will match it properly.

Otherwise we might be in the situation where we match the system
installed (via apt) desktop file as the one of the snap.

Unfortunately, this might still be the case, since the application
window could have a wrong class name (it depends in how it' computed, by
default it uses the binary basename, but it can be overridden) or, for
gtk apps a _GTK_APPLICATION_ID that is always the same.

So, there are still some edge cases we can't cover, but we can try to do
our best.

So I guess we can continue here.

ted-gould · Feb 29, 2016

ted-gould reviewed Feb 29, 2016

View changes

I don't think it's a good idea to use a desktop file verbatim. It needs to be associated with a command and have the Exec line match that command. For instance my desktop Exec could read data at the permissions level of the shell before going into confinement. I think that a much better approach is to generate the desktop file from data in the YAML.

ted-gould · Feb 29, 2016

Can we please have an interface for this? Here are a few reasons I think an interface is important:

When we switch Unity8 over to being a snap we can have a hook for the interface so we don't have to watch file folders for updates. This makes it easier to build caches and is less error prone.
Putting all of the data for the application in a single file format makes building for that platform easier. If you look at what we have with Click today there are a collection of file formats, all with different syntax and meanings. My favorite is that they have different capitalizations of "true" (I mean "True" or "TRUE").
What goes into a desktop file gets read by several critical processes in the system, we want to ensure what goes in it is correct and valid. Generating it is the best way to do that.

All in all, if it is an interface, we can still have a desktop file for systems like Unity7 and BAMF to use, we just remove it from the things that snap packagers have to know about.

mvo5 · Mar 1, 2016

Closing for now, the concern about Exec and TryExec running without confinement is very valid and needs addressing. It sounds like feature/apps may need resurrecting which auto-generates desktop files. Which in itself has problems of course.

mvo5 · Mar 2, 2016

Thanks everyone for the review and the comments.

@ted-gould I talked about the generate vs filter/whitelist with @niemeyer yesterday and after going over the pros and cons he suggested to go with the approach implemented in this branch plus proper whitelist based filtering. So I reopen this pull-request again.

An extra careful eye on the whitelist is appreciated

ted-gould · Mar 2, 2016

That's fine, can we then say that it is Snappy policy that for extension of snappy we expect files to be put in /meta/$(appname).* ? We have a lot more extensions of this manner that we want to land and so I'd like to know what that policy is.

niemeyer · Mar 2, 2016

@ted-gould You sent a mail to the list, so let's not cover this here any further.

niemeyer · Mar 2, 2016

niemeyer reviewed Mar 2, 2016

View changes

Should we make this path absolute when copying it over?

Also, the test should likely be `cmd == validCmd || HasPrefix(cmd, validCmd+" ")

Not sure TryExec makes much sense in this context. Does it?

Good catch! I Fixed the cmd == validCmd || HasPrefix() bit, that was indeed incorrect before. I also dropped TryExec as I agree that it does not make sense in the snap world.

I also added code to make the path absolute in f4357d1

niemeyer · Mar 2, 2016

niemeyer reviewed Mar 2, 2016

View changes

That's a bit surprising, and may be the source of future bugs as these are not actually valid Exec lines. It's easy to fix that by inverting the logic: rename the function to badExecLine, and invert the false<=>true logic (and the call site test). Then, it's true that something not being an Exec line is also not a bad Exec line.

Indeed, this is fixed now.

niemeyer · Mar 2, 2016

niemeyer reviewed Mar 2, 2016

View changes

This would be if badExecLine(m, line) {

niemeyer · Mar 3, 2016

niemeyer reviewed Mar 3, 2016

View changes

Should this logic be in something like a desktop.go file? click.go seems to be used as a catch-all at the moment.

Good point! I moved everything into desktop{,_test}.go now.

niemeyer · Mar 3, 2016

LGTM

mvo5 · Mar 3, 2016

Thanks for the excellent review, I addressed the points, should be ready for a re-review.

zyga · Mar 3, 2016

zyga reviewed Mar 3, 2016

View changes

Did you mean t live this "unterminated", e.g. no \]$?

Yes, this is for actions, but its confusing so I made this explicit now and added tests.

chipaca · Mar 3, 2016

chipaca reviewed Mar 3, 2016

View changes

...and will be removed / they will be ignored / snap installation will fail / the computer will be considered untrusted and all your cookies will be eaten.

I think that looking at the code we're stripping all other keys, for instance X-* keys, as well. We should note that in the documentation. (I do think it's a good idea, just saying we should tell people)

chipaca · Mar 3, 2016

chipaca reviewed Mar 3, 2016

View changes

why are we parsing it by hand instead of using e.g. goconfigparser?

I was tempted to suggest the exact opposite when looking over the branch: why are we even using regexps when this pretty much just prefix checking? I doubt using goconfigparser would simplify the logic below, and it would definitely increase the complexity by adding a dependency into the picture.

Thanks.

I started out with plain prefix checking. For the i18n strings like GenericName[de_DE@NL]= I switched to regexp. I'm of course open for alternative suggestions how we could check those without the need to hard-to-read regexp.

👍

Sorry, this is confusing now, I commented on an out-of-date version and apparently it just gets appended. I think that using a proper ini parser is better. That way we can also ensure things like extra groups don't get copied over easily.

http://play.golang.org/p/2KAuk4nXUg

The goal is having simple and correct logic only. A "proper" parser creates an invalid sense of safety if it's simply adding a third party dependency that requires significantly more logic both locally and in the third-party code itself.

@niemeyer Thanks for the example. This will require some more tweaks, if you think that is worth the extra work to avoid the regexp, I will work on this now.

ted-gould · Mar 3, 2016

ted-gould reviewed Mar 3, 2016

View changes

It seems like instead of getting all desktop files, you only want ones that are the name of an application you know. Or put a check lower to ensure you have an application that matches the desktop file name.

Thanks! So far I was not considering restricting the name of the desktop files. Should we do this in order to help unity/BAMF to match desktop-file to application? It feels a little heavy handed to restrict it so that application foo can only have meta/gui/foo.desktop but if it is actually important for unity/BAMF I can change that of course.

I don't think BAMF is a concern with that naming, as it won't be able to use the binary name matching anyway. As the Exec is going to point to a shell script in most cases. So the actual binary name will be different. But more I was thinking that you'd have a 1:1 ratio of apps to desktop files. I realize now that you weren't thinking of that restriction. I'm not sure there's a technical reason to prefer that, but it does feel weird to me that we'd have multiple GUI icons for each Snappy Application.

ted-gould · Mar 3, 2016

ted-gould reviewed Mar 3, 2016

View changes

I'm not sure how wide spread it is, but the spec does not limit the name of the action like this. It is just a string. I have seen action names that use spaces for sure, not sure of the usage of other characters.

Interesting. The way I read https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s10.html#extra-actions-keys

Each action is identified by a string, following the same format as key names (see the section called “Entries”). Each identifier is associated with an action group that must be present in the .desktop file. The action group is a group named Desktop Action %s, where %s is the action identifier.

and in https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s02.html#entries: Only the characters A-Za-z0-9- may be used in key names.

However this may actually not reflect reality. I see for example [Desktop Action window-shot] from gnome-screenshot which clearly violates this :) So maybe this needs to be relaxed for the real world.

I missed that part of the spec, but whether we become less restrictive or not, we should at least add in "-" to the regex.

Indeed, I wonder if this part of the spec is actually enforced anywhere :) I will add the "-" for now and we may need to relax it further.

ted-gould · Mar 3, 2016

ted-gould reviewed Mar 3, 2016

View changes

Would be handy to put the link in the text: https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-1.1.html

ted-gould · Mar 3, 2016

ted-gould reviewed Mar 3, 2016

View changes

Seems like, since different apps can have different permissions, we need to ensure that the exec line matches the application that it is associated with instead of any app in the package.

ted-gould · Mar 3, 2016

ted-gould reviewed Mar 3, 2016

View changes

I'm not sure that's a valid assumption. I don't know that we can reasonably protect against it, but we should probably at least change the comment to "Unity 7 doesn't execute them with a shell, and that's all we care about." There are a lot of desktops out there that do different things.

mvo5 · Mar 3, 2016

Thanks for the detailed reviews I got! I addressed all points, except for the regexp removal one. I wonder if we may consider leaving the regexp (unless you think its much easier code without them, but I suspect it will also not be super straightforward without them). If there is anything else that should be addressed before this can land, please let me know!

niemeyer · Mar 3, 2016

@mvo5 I didn't comment on regexp originally because I didn't feel like creating more work for this branch, but have you seen this: http://play.golang.org/p/2KAuk4nXUg This looks quite straightforward.

mvo5 · Mar 7, 2016

@niemeyer The code is updated with the suggested changes. Please let me know if there is anything else I should do.

elopio · Mar 7, 2016

elopio reviewed Mar 7, 2016

View changes

s/starts/start

elopio · Mar 7, 2016

@mvo5 what about a snap+test for basic-with-desktop-file in https://github.com/ubuntu-core/snappy/tree/master/integration-tests/data/snaps ?

niemeyer · Mar 7, 2016

Thanks Michael, and sorry for the churn on this one.

LGTM

mvo5 · Mar 7, 2016

@elopio I will prepare a new branch with a integration test, this review is already pretty long

elopio · Mar 7, 2016

agree. Thanks mvo.

niemeyer added the Reviewed label Feb 29, 2016

mvo5 closed this Mar 1, 2016

mvo5 reopened this Mar 2, 2016

mvo5 removed the Reviewed label Mar 2, 2016

pedronis changed the title from add support to use desktop files from inside snaps to snappy,dirs: add support to use desktop files from inside snaps Mar 2, 2016

mvo5 referenced this pull request Mar 3, 2016
Merged
snappy: cleanup click.go and move functionatlity into more meaningful places #569

Merge remote-tracking branch 'upstream/master' into feature/desktop-f…
…iles

9ecc3ca

fix typo (thanks to Elopio!)

Some checks were not successful
1 failing and 1 successful checks

Integration tests — 23 tests run, 0 skipped, 1 failed.

Details

continuous-integration/travis-ci/pr — The Travis CI build passed

Details

b8fab03

mvo5 merged commit 9ddb765 into snapcore:master Mar 7, 2016
1 of 2 checks passed

1 of 2 checks passed

Integration tests 23 tests run, 0 skipped, 1 failed.
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

+        +	app := &AppYaml{
+        +		DesktopFile: "foo.desktop",
+        +	}
+        +	c.Assert(generateDesktopFileName(m, app), Equals, filepath.Join(dirs.SnapDesktopFilesDir, "foo_foo.desktop"))

@@ -63,6 +63,9 @@ type AppYaml struct {
          	Command string `yaml:"command"`
          	Daemon  string `yaml:"daemon"`
+        +	// for desktop-file integration
+        +	DesktopFile string `yaml:"desktop"`

+        +	glob := filepath.Join(dirs.SnapDesktopFilesDir, m.Name+"_*.desktop")
+        +	activeDesktopFiles, err := filepath.Glob(glob)
+        +	if err != nil {
+        +		return fmt.Errorf("can not get desktop files for %v: %s", glob, err)

+        +		validCmd := filepath.Base(generateBinaryName(m, app))
+        +		// just check the prefix to allow %flag style args
+        +		// this is ok because desktop files are not run through sh
+        +		if strings.HasPrefix(cmd, validCmd) {

+        +		if !isValidDesktopFilePrefix(line) {
+        +			continue
+        +		}
+        +		if !validExecLine(m, line) {

snapcore/snapd

Join GitHub today

snappy,dirs: add support to use desktop files from inside snaps #548

Conversation

mvo5 commented Feb 29, 2016

Some checks were not successful

sergiusens commented Feb 29, 2016

zyga reviewed Feb 29, 2016 View changes

zyga reviewed Feb 29, 2016 View changes

zyga commented Feb 29, 2016

Some checks were not successful

niemeyer commented Feb 29, 2016

niemeyer added the Reviewed label Feb 29, 2016

sergiusens commented Feb 29, 2016

mvo5 commented Feb 29, 2016

mvo5 commented Feb 29, 2016

mvo5 commented Feb 29, 2016

Some checks were not successful

mvo5 commented Feb 29, 2016

sergiusens commented Feb 29, 2016

sergiusens commented Feb 29, 2016

niemeyer reviewed Feb 29, 2016 View changes

niemeyer reviewed Feb 29, 2016 View changes

niemeyer reviewed Feb 29, 2016 View changes

niemeyer commented Feb 29, 2016

mvo5 commented Feb 29, 2016

mvo5 commented Feb 29, 2016

mvo5 added some commits Feb 29, 2016

All checks have passed

All checks have passed

mvo5 commented Feb 29, 2016

3v1n0 commented Feb 29, 2016

ted-gould reviewed Feb 29, 2016 View changes

ted-gould commented Feb 29, 2016

mvo5 closed this Mar 1, 2016

mvo5 commented Mar 1, 2016

mvo5 added some commits Mar 2, 2016

All checks have passed

mvo5 commented Mar 2, 2016

mvo5 reopened this Mar 2, 2016

mvo5 removed the Reviewed label Mar 2, 2016

ted-gould commented Mar 2, 2016

pedronis changed the title from add support to use desktop files from inside snaps to snappy,dirs: add support to use desktop files from inside snaps Mar 2, 2016

niemeyer commented Mar 2, 2016

niemeyer reviewed Mar 2, 2016 View changes

niemeyer reviewed Mar 2, 2016 View changes

niemeyer reviewed Mar 2, 2016 View changes

niemeyer reviewed Mar 3, 2016 View changes

niemeyer commented Mar 3, 2016

mvo5 added some commits Mar 3, 2016

All checks have passed

All checks have passed

All checks have passed

mvo5 commented Mar 3, 2016

All checks have passed

mvo5 referenced this pull request Mar 3, 2016

zyga reviewed Mar 3, 2016 View changes

All checks have passed

chipaca reviewed Mar 3, 2016 View changes

chipaca reviewed Mar 3, 2016 View changes

ted-gould reviewed Mar 3, 2016 View changes

mvo5 added some commits Mar 3, 2016

Some checks were not successful

Some checks were not successful

ted-gould reviewed Mar 3, 2016 View changes

ted-gould reviewed Mar 3, 2016 View changes

ted-gould reviewed Mar 3, 2016 View changes

ted-gould reviewed Mar 3, 2016 View changes

Some checks were not successful

mvo5 commented Mar 3, 2016

niemeyer commented Mar 3, 2016

mvo5 added some commits Mar 7, 2016

All checks have passed

All checks have passed

mvo5 commented Mar 7, 2016

elopio reviewed Mar 7, 2016 View changes

elopio commented Mar 7, 2016

Some checks were not successful

niemeyer commented Mar 7, 2016

mvo5 commented Mar 7, 2016

zyga reviewed Feb 29, 2016

View changes

zyga reviewed Feb 29, 2016

View changes

niemeyer reviewed Feb 29, 2016

View changes

niemeyer reviewed Feb 29, 2016

View changes

niemeyer reviewed Feb 29, 2016

View changes

ted-gould reviewed Feb 29, 2016

View changes

niemeyer reviewed Mar 2, 2016

View changes

niemeyer reviewed Mar 2, 2016

View changes

niemeyer reviewed Mar 2, 2016

View changes

niemeyer reviewed Mar 3, 2016

View changes

zyga reviewed Mar 3, 2016

View changes

chipaca reviewed Mar 3, 2016

View changes

chipaca reviewed Mar 3, 2016

View changes

ted-gould reviewed Mar 3, 2016

View changes

ted-gould reviewed Mar 3, 2016

View changes

ted-gould reviewed Mar 3, 2016

View changes

ted-gould reviewed Mar 3, 2016

View changes

ted-gould reviewed Mar 3, 2016

View changes

elopio reviewed Mar 7, 2016

View changes

mvo5 merged commit `9ddb765` into snapcore:master Mar 7, 2016
1 of 2 checks passed