cmd/snap: gnome-software install via snap:// handler #5346

jhenstridge · 2018-06-20T05:23:47Z

We would like snap:// links in the web browser to work on any system that has snapd installed. At present, these are handled by GNOME Software, provided that it has been built with the snap plugin.

This PR attempts to fill the gap for all other desktop systems with snapd installed as follows:

install a .desktop file that advertises support for the mime type x-scheme-handler/snap, and calls snap snap-uri-handler %U.
Add a snap-uri-handler command to the snap executable that does the following:
- Check to see whether the gnome-software snap is installed.
- If not, prompt the user to install it via a Zenity dialog (similar to that used by our xdg-open/xdg-mime proxies).
- Run the snapped gnome-software with the URI as an argument.

The gnome-software snap is maintained by the Ubuntu Desktop team, and we plan for this to be the primary way of delivering the app to Ubuntu desktops going forward. When run on non-Ubuntu systems, we plan to limit it to only showing results from the Snap store, so it doesn't duplicate results from the host system's software store app (and also so we don't have to ensure it supports every packaging system under the sun).

zyga

Initial review, some questions and suggestions around

zyga · 2018-06-20T09:26:44Z

tests/lib/snaps/gnome-software/bin/gnome-software

@@ -0,0 +1,2 @@
+#!/bin/sh
+echo "Open URI: $1"


Heh, nice idea :)

Something like:

Fake snap got %s

might help make the test more clear. As it is the test doesn't make clear that Open URI comes from this place. In other words, the diff is clear as we have everything here, but someone reading the test in a short while won't realize what's actually printing "Open URI".

tests/main/snap-snap-uri-handler/task.yaml

+prepare: |
+    #shellcheck source=tests/lib/pkgdb.sh
+    . "$TESTSLIB/pkgdb.sh"
+    distro_install_package xdg-utils


zyga · 2018-06-20T09:28:38Z

tests/main/snap-snap-uri-handler/task.yaml

+    MATCH "GNOME Software required" < errors.log
+
+    echo "Now with gnome-software installed"
+    snap try "$TESTSLIB"/snaps/gnome-software


I think we could also use install_local, is there any specific purpose for try here?

zyga · 2018-06-20T09:29:27Z

tests/main/snap-snap-uri-handler/task.yaml

+
+    echo "Now with gnome-software installed"
+    snap try "$TESTSLIB"/snaps/gnome-software
+    snap snap-uri-handler snap://package | MATCH "Open URI: snap://package"


Not sure if this is acknowledged with regards to design but I find it repetitive. snap uri-handler snap://package would look nicer IMO.

I guess I wanted to namespace the command to make it clear that it was for handling "snap://" URIs.

I don't think users would tend to call this directly, so ideally it'd be a hidden command. Some of the "snap debug" commands were hidden, but I couldn't see how to do the same for top level commands.

zyga · 2018-06-20T09:32:11Z

data/desktop/Makefile

+AUTOSTART_SOURCES = snap-userd-autostart.desktop.in
+AUTOSTART_FILES = $(AUTOSTART_SOURCES:.in=)
+
+DESKTOP_FILES = $(APPLICATION_FILES) $(AUTOSTART_FILES)

 .PHONY: all
 all: $(DESKTOP_FILES)

 .PHONY: install
 install: $(DESKTOP_FILES)


Not a big deal but perhaps this would be cleaner:

# NOTE: old (e.g. 14.04) GNU coreutils doesn't -D with -t install:: $(APPLICATION_FILES) install -d -m 0755 $(DESTDIR)/$(APPLICATIONSDIR) install -m 0644 -t $(DESTDIR)/$(APPLICATIONSDIR) $^ install:: $(AUTOSTART_FILES) install -d -m 0755 $(DESTDIR)/$(SYSCONFXDGAUTOSTARTDIR) install -m 0644 -t $(DESTDIR)/$(SYSCONFXDGAUTOSTARTDIR) $^

zyga · 2018-06-20T09:32:25Z

data/desktop/Makefile

+AUTOSTART_SOURCES = snap-userd-autostart.desktop.in
+AUTOSTART_FILES = $(AUTOSTART_SOURCES:.in=)
+
+DESKTOP_FILES = $(APPLICATION_FILES) $(AUTOSTART_FILES)


If you follow my advice below please remove DESKTOP_FILES

zyga · 2018-06-20T09:32:55Z

cmd/snap/cmd_snap_uri_handler.go

+
+func init() {
+	addCommand("snap-uri-handler",
+		"Handle a snap:// URI",


Nitpick, please use i18n helpers.

zyga · 2018-06-20T09:33:56Z

cmd/snap/cmd_snap_uri_handler.go

+		return err
+	}
+	answeredYes := dialog.YesNo(
+		i18n.G("Install GNOME Software?"),


This is misleading. The user may already have GNOME Software installed, just not as a snap. How about "Install snap-aware GNOME Software snap?"

zyga · 2018-06-20T09:34:38Z

cmd/snap/cmd_snap_uri_handler.go

+	if _, _, err := cli.Snap("gnome-software"); err == nil {
+		return nil
+	}
+


Would it be okay to use gnome-software from the classic distribution if it meets some requirements that we can test for?

@kenvandine's original idea was to use the gnome-software snap everywhere. I guess there are two questions when trying to determine if the host system version is sufficient:

does it have the snap plugin?

is it a new enough version of gnome-software to include all the features we want to expose? (e.g. old versions won't let you pick a channel for installed snaps).

I think checking for the presence of /usr/share/metainfo/org.gnome.Software.Plugin.Snap.metainfo.xml would satisfy (1), but (2) is a bit harder. @robert-ancell might have some input too.

Would it be okay to use gnome-software from the classic distribution if it meets some requirements that we can test for?

We can delay that decision for a while. It'll be nice to exercise the exception path on our end for longer and make sure it works well for everybody, before deciding to take the shortcut.

jhenstridge · 2018-06-20T10:59:10Z

A few more notes about the change:

without a mimeapps.list file setting the default handler for the URI scheme, it is undefined which .desktop file will be launched to handle the URI when there are multiple handlers. For the case of a snapped gnome-software, the two .desktop files should behave identically: one launches gnome-software directly, and the other launches it via "snap snap-uri-handler".
I'm not sure what we can do about a classic packaged gnome-software. Looking at the source code, it advertises support for the snap URI scheme in its desktop file unconditionally. While we could improve things to only advertise the URI scheme if the snap plugin is actually built, that doesn't really for distros that split the app into multiple binary packages (e.g. Debian, Ubuntu, Fedora).

robert-ancell · 2018-06-20T22:52:26Z

You may be able to make this a true "handler of last resort" by using TryExec in snap-uri-handler.desktop. That could run a program to determine if any other snap handler is installed, and return false in that case. This will avoid prompting the user to choose between "GNOME Software" (or KDE Discover etc) and "Handler for snap:// URIs".

A cleaner way might be to install /usr/share/applications/snap-store.desktop:
Name=Snap Store (install)
Exec=<snap install gnome-software, launch gnome-software with requested snap>
TryExec=<false if gnome-software snap installed>

In terms of trying to work out if you have a sufficiently recent version of the GNOME Software snap plugin installed, I think you're going to have a lot of trouble determining this reliably. I think it should be considered a bug in GNOME Software if it is reporting that it can handle snap:// URLs when it can't.

jhenstridge · 2018-06-21T05:13:30Z

Looking at glib/gio/gdesktopappinfo.c, it looks like it only checks for the presence of the TryExec executable rather than executing it, so I'm not sure that would actually work. The shell implementation in xdg-open doesn't seem to do anything with TryExec at all.

As far as gnome-software advertising support for the URI scheme goes, it could probably get around this by installing multiple .desktop files, with the URI schemes being advertised by hidden files that could be packaged with the plugins.

jhenstridge · 2018-06-21T09:51:49Z

I've filed https://gitlab.gnome.org/GNOME/gnome-software/issues/405 to cover the issue of snap-less gnome-software installs advertising the snap: URI scheme.

One other issue with this PR is that it should be installing gnome-software from the stable channel. It isn't doing so currently because we haven't yet published to that channel and I needed something to test.

codecov-io · 2018-06-22T08:31:45Z

Codecov Report

Merging #5346 into master will decrease coverage by 0.06%.
The diff coverage is 18.42%.

@@            Coverage Diff             @@
##           master    #5346      +/-   ##
==========================================
- Coverage   78.99%   78.93%   -0.07%     
==========================================
  Files         503      504       +1     
  Lines       37947    37985      +38     
==========================================
+ Hits        29977    29984       +7     
- Misses       5547     5578      +31     
  Partials     2423     2423

Impacted Files	Coverage Δ
cmd/snap/cmd_snap_uri_handler.go	`18.42% <18.42%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3b7633...8e01d6e. Read the comment docs.

zyga

Code wise this looks fine. I added two comments for a makefile tweak. Please get a review from @niemeyer for the design.

zyga · 2018-06-29T13:06:27Z

data/desktop/Makefile

 	install -d -m 0755 $(DESTDIR)/$(SYSCONFXDGAUTOSTARTDIR)
-	install -m 0644 -t $(DESTDIR)/$(SYSCONFXDGAUTOSTARTDIR) $^
+	install -m 0644 -t $(DESTDIR)/$(SYSCONFXDGAUTOSTARTDIR) $(AUTOSTART_FILES)


You can keep using $^ here

zyga · 2018-06-29T13:06:35Z

data/desktop/Makefile

+# NOTE: old (e.g. 14.04) GNU coreutils doesn't -D with -t
+install:: $(DESKTOP_FILES)
+	install -d -m 0755 $(DESTDIR)/$(APPLICATIONSDIR)
+	install -m 0644 -t $(DESTDIR)/$(APPLICATIONSDIR) $(DESKTOP_FILES)


You can use $^ as well

Conan-Kudo · 2018-07-01T14:44:36Z

@jhenstridge In the Fedora case, we actually do build the snap plugin for both gnome-software and plasma-discover. I'd rather see this backup handler thing first query for a package that provides it and install it if it's not available. If that fails, then install the snap version of g-s...

niemeyer

A few comments, but nothing major.

Idea looks nice, thanks!

niemeyer · 2018-07-02T12:46:43Z

cmd/snap/cmd_snap_uri_handler.go

+}
+
+func init() {
+	addCommand("snap-uri-handler",


I suggest "handle-link" for this, as being slightly more comfortable to use in conversations and nicer to read/write, and it's also a verb rather than a noun.

niemeyer · 2018-07-02T12:49:00Z

cmd/snap/cmd_snap_uri_handler.go

+	if _, _, err := cli.Snap("gnome-software"); err == nil {
+		return nil
+	}
+


Would it be okay to use gnome-software from the classic distribution if it meets some requirements that we can test for?

We can delay that decision for a while. It'll be nice to exercise the exception path on our end for longer and make sure it works well for everybody, before deciding to take the shortcut.

niemeyer · 2018-07-02T12:49:41Z

cmd/snap/cmd_snap_uri_handler.go

+func init() {
+	addCommand("snap-uri-handler",
+		i18n.G("Handle a snap:// URI"),
+		i18n.G("The snap-uri-handler command installs the gnome-software snap and then invokes it."),


Hadn't we decided not to call this "gnome-software", so it doesn't get confused with the deb or rpm version, and highlight the fact this is snap only?

That is my understanding too, talking to Ken and Robert. I don't think the name has been settled on yet, so I haven't changed it here. That's definitely something that blocks merging of this branch though.

There's now agreement that this is the "snap store" snap.

niemeyer · 2018-07-02T13:58:54Z

packaging/fedora/snapd.spec

@@ -624,6 +624,7 @@ popd
 %{_unitdir}/snapd.seeded.service
 %{_unitdir}/snapd.autoimport.service
 %{_unitdir}/snapd.seeded.service
+%{_datadir}/applications/snap-uri-handler.desktop


Let's please name it after the new command name, whatever it ends up being (e.g. snap-handle-link.desktop assuming suggestion).

niemeyer · 2018-07-02T14:00:59Z

tests/lib/snaps/gnome-software/bin/gnome-software

@@ -0,0 +1,2 @@
+#!/bin/sh
+echo "Open URI: $1"


Something like:

Fake snap got %s

might help make the test more clear. As it is the test doesn't make clear that Open URI comes from this place. In other words, the diff is clear as we have everything here, but someone reading the test in a short while won't realize what's actually printing "Open URI".

niemeyer · 2018-07-02T14:22:30Z

tests/main/snap-snap-uri-handler/task.yaml

+
+    echo "URI Handler fails if gnome-software is not installed and user refuses to install it"
+    mount --bind /bin/false /usr/bin/zenity
+    if snap snap-uri-handler snap://package 2>errors.log; then


Can't that be just something like:

snap handle-link snap://package 2>&1 | MATCH "GNOME Software required"

?

We want to both check the exit code of the command invocation and that the expected message was printed to stderr (not stdout).

niemeyer

LGTM.. just needs to be update to reflect the actual name of the snap store snap. Let's please avoid any mentions of "gnome software" here, so it's consistent.

niemeyer · 2018-08-21T10:53:38Z

cmd/snap/cmd_handle_link.go

+func init() {
+	addCommand("handle-link",
+		i18n.G("Handle a snap:// URI"),
+		i18n.G("The handle-link command installs the gnome-software snap and then invokes it."),


Okay, we have agreement about the name of the store snap:

% snap info snap-store name: snap-store summary: Snap Store allows you to find and install new applications and remove existing installed applications. publisher: Canonical✓ license: GPL-3.0+

niemeyer · 2018-08-21T10:54:12Z

cmd/snap/cmd_handle_link.go

+
+func (x *cmdHandleLink) ensureGnomeSoftwareInstalled(cli *client.Client) error {
+	// If the gnome-software snap is installed, our work is done
+	if _, _, err := cli.Snap("gnome-software"); err == nil {


Needs tweaking.

niemeyer · 2018-08-21T10:54:37Z

cmd/snap/cmd_handle_link.go

+		return err
+	}
+	answeredYes := dialog.YesNo(
+		i18n.G("Install snap-aware GNOME Software snap?"),


Needs tweaking. ("Install snap store snap?" maybe?)

niemeyer · 2018-08-21T10:54:43Z

cmd/snap/cmd_handle_link.go

+	}
+	answeredYes := dialog.YesNo(
+		i18n.G("Install snap-aware GNOME Software snap?"),
+		i18n.G("GNOME Software is required to open snaps from a web browser."),


Needs tweaking.

niemeyer · 2018-08-21T10:54:51Z

cmd/snap/cmd_handle_link.go

+			Footer:  i18n.G("This dialog will close automatically after 5 minutes of inactivity."),
+		})
+	if !answeredYes {
+		return fmt.Errorf(i18n.G("GNOME Software required"))


Needs tweaking.

niemeyer · 2018-08-21T10:54:59Z

cmd/snap/cmd_handle_link.go

+		Channel: "edge", // FIXME: remove this when gnome-software published to stable
+		Classic: true,
+	}
+	changeID, err := cli.Install("gnome-software", &opts)


Needs tweaking.

niemeyer · 2018-08-21T10:55:07Z

cmd/snap/cmd_handle_link.go

+		return err
+	}
+
+	argv := []string{"snap", "run", "gnome-software", x.Positional.Uri}


Needs tweaking.

niemeyer · 2018-08-21T10:55:30Z

cmd/snap/cmd_handle_link.go

+		}, nil, nil)
+}
+
+func (x *cmdHandleLink) ensureGnomeSoftwareInstalled(cli *client.Client) error {


Needs tweaking.

niemeyer · 2018-08-21T10:57:01Z

By the way, I suggest testing with the actual snap which is published, just to make sure it really works in practice.

Conan-Kudo · 2018-08-21T11:17:21Z

To be clear, this should not activate in cases that there's already a graphical tool to handle snaps, right?

niemeyer · 2018-08-29T18:04:25Z

This is missing a second review. @pedronis? @chipaca?

niemeyer · 2018-08-29T18:06:02Z

@Conan-Kudo This just registers a handler for snap:// as usual for the desktop.

zyga

Looks good

zyga · 2018-09-12T11:56:43Z

Tests need to mock a browser for xdg-open

This version is using a shell implementation to determine the desktop ID selected to handle the URI scheme and getting it wrong. Full implementations of the spec (e.g. GIO, Qt) will still get it right on these distros.

jhenstridge · 2018-09-14T11:26:44Z

I've disabled that part of the spread test on 16.04. It was passing when I started this branch but doesn't seem to now. The attempt to open a web browser is a fallback after failing to find the handler.

The shell fallback implementation in xdg-open seems a bit fragile, particularly the versions in older releases. So it might make sense to try and use the gio open instead, which has a more conformant implementation and uses the same code that e.g. Firefox ends up using.

As for other blockers, this branch still tries to install snap-store from the edge channel. This should be switched to stable, but of course that won't work until something is released in that channel.

…require classic confinement

niemeyer · 2018-10-04T16:15:31Z

@jdstrand remaining here? I had some review comments above unanswered, but I suppose they were all addressed? Anything else pending or should this go in?

jdstrand · 2018-10-17T20:48:54Z

@niemeyer pinged me on this, but I'm lacking context. I don't object to the concept of prompting to install the snap-store and quickly perusing the code, it defaults to not installing unless the user clicks to install it. I have not verified that mechanism can't be subverted, but at the level I looked at this, seems fine.

jhenstridge force-pushed the snap-uri-handler branch from a22cd3b to 8c7574f Compare June 20, 2018 06:02

zyga reviewed Jun 20, 2018

View reviewed changes

jhenstridge force-pushed the snap-uri-handler branch from 008b37e to 8c23ac5 Compare June 20, 2018 10:49

jhenstridge force-pushed the snap-uri-handler branch from 8c23ac5 to 2d90d4f Compare June 20, 2018 14:00

jhenstridge force-pushed the snap-uri-handler branch 3 times, most recently from 7b01d2e to 699dcbc Compare June 21, 2018 04:47

jhenstridge force-pushed the snap-uri-handler branch 2 times, most recently from e0fe27a to e5687e9 Compare June 21, 2018 09:48

jhenstridge force-pushed the snap-uri-handler branch 3 times, most recently from 0d71597 to 634bfa5 Compare June 22, 2018 08:01

jhenstridge force-pushed the snap-uri-handler branch from 634bfa5 to 8e01d6e Compare June 22, 2018 09:30

mvo5 added this to the 2.34 milestone Jun 27, 2018

zyga reviewed Jun 29, 2018

View reviewed changes

niemeyer requested changes Jul 2, 2018

View reviewed changes

mvo5 removed this from the 2.34 milestone Jul 5, 2018

jhenstridge force-pushed the snap-uri-handler branch 2 times, most recently from 418b7d0 to 4d90b4b Compare July 9, 2018 14:50

niemeyer changed the title ~~cmd/snap: add a backup handler for snap:// URIs that prompts the user to install the gnome-software snap~~ cmd/snap: gnome-software install via snap:// handler Aug 16, 2018

niemeyer approved these changes Aug 21, 2018

View reviewed changes

jhenstridge added 9 commits September 7, 2018 17:20

cmd/snap: add snap-uri-handler command.

91f65d0

tests: add a test for "snap snap-uri-handler"

9d51d71

data, tests: add .desktop file to bind snap: URIs to the uri handler

68e796d

packaging: include new desktop file in fedora/opensuse packaging

259554f

tests: don't perform the xdg-open portion of the test on Ubuntu 14.04

4175367

cmd, data, tests: small fixes from zyga's review

ccf31c2

tests: source snaps.sh for install_local

9aecdf6

cmd: rename snap-uri-handler to handle-link

05ff087

cmd/snap: use the snap-store snap instead of gnome-software

03e008c

jhenstridge force-pushed the snap-uri-handler branch from 4d90b4b to 03e008c Compare September 7, 2018 09:36

zyga approved these changes Sep 12, 2018

View reviewed changes

tests: disable the xdg-open invocation on 16.04 too.

cb6c945

This version is using a shell implementation to determine the desktop ID selected to handle the URI scheme and getting it wrong. Full implementations of the spec (e.g. GIO, Qt) will still get it right on these distros.

cmd/snap: snap-store is now published to stable channel and does not …

ed4ca93

…require classic confinement

jhenstridge force-pushed the snap-uri-handler branch 2 times, most recently from 05b6c57 to e342929 Compare October 18, 2018 15:08

Merge remote-tracking branch 'upstream/master' into snap-uri-handler

5e57109

jhenstridge force-pushed the snap-uri-handler branch from e342929 to 5e57109 Compare October 19, 2018 00:05

bboozzoo approved these changes Oct 23, 2018

View reviewed changes

bboozzoo merged commit 4d120db into canonical:master Oct 23, 2018

cmd/snap: gnome-software install via snap:// handler #5346

cmd/snap: gnome-software install via snap:// handler #5346

Conversation

jhenstridge commented Jun 20, 2018

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhenstridge commented Jun 20, 2018

robert-ancell commented Jun 20, 2018 • edited Loading

jhenstridge commented Jun 21, 2018

jhenstridge commented Jun 21, 2018

codecov-io commented Jun 22, 2018 • edited Loading

Codecov Report

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conan-Kudo commented Jul 1, 2018

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer commented Aug 21, 2018

Conan-Kudo commented Aug 21, 2018

niemeyer commented Aug 29, 2018

niemeyer commented Aug 29, 2018

zyga left a comment

Choose a reason for hiding this comment

zyga commented Sep 12, 2018

jhenstridge commented Sep 14, 2018

niemeyer commented Oct 4, 2018

jdstrand commented Oct 17, 2018

robert-ancell commented Jun 20, 2018 •

edited

Loading

codecov-io commented Jun 22, 2018 •

edited

Loading