Integrate snapd-xdg-open into snapd repository

snapd
1

Hey everyone,

as part of my cross-distro work I am now also looking into snapd-xdg-open and wondering if it would make sense to integrate it directly into the snapd source tree similar to what we do for snap-confine. From my point of view this would simplify the maintenance process and we can release it together with snapd instead of keeping it separate and following its own release cycle.

Currently we miss a proper test setup and release process for. The git repository was never tagged yet and has no testing. Also integrating it into the snapd repository would keep the number of packages we have to maintain across multiple distributions to a single one.

What do you think?

regards,
Simon

6 Likes
2

+1, we need to figure out a way to avoid pulling in desktop-related dependencies in server-installs, though. Not sure as to what degree this applies to other distros, though.

3

@tvoss We can still put it into a dedicated deb package to avoid it being pulled on server-installs. On the other hand it installs a just a session service which we wouldn’t start in any case on a server install as there is no session and the snapd-xdg-open deb package as of today doesn’t have any other dependencies than libglib2.0 and libc6 so it would be safe to include in the snapd package directly.

4

@morphis works for me if introducing the glib dependency is fine for server installs.

5

@tvoss Would be interesting to see if we don’t have it already on the server image. Would guess that something from the python or other bits pulls it already in.

6

I think this should have happened earlier. It’s great that you pick this up. I was actually thinking we could install it by default as it doesn’t really do anything. It is bus-activated and has minimal dependencies. Maybe we can pull it off as single package. Last time we looked at this I think there was some concern over dbus dependency but perhaps I’m mistaken.

1 Like
7

@zyga-snapd dependency on dbus-daemon is a runtime one and the debian package for snapd-xdg-open doesn’t have it. It just installs the service file and if dbus is available and a user session it gets activated when needed. So as long we can have libglib/gio on a server image I think we’re fine having it by default part of the snapd deb package.

1 Like
8

I think this is essential. Currently snapd-xdg-open only exists in Ubuntu 14.04 (recently) and Ubuntu 16.04. So installing applications that require snapd-xdg-open on other snapd enabled distros means breakage.

For example many of the Electron applications have regressions on Debian, openSUSE and Fedora right now. Integrating it into snapd means one less packaging requirement for the other distros and a guarantee of availability.

9

There we go: https://github.com/snapcore/snapd/pull/3118

1 Like
10

Thanks for the interest in improving the situation of snapd-xdg-open, @morphis. We’ve handled this poorly from the get go, by not making it more obvious that it needed to be packaged together and shipped by every distribution that intended to have web link clicks working from within snaps.

That said, we can probably do better than encouraging maintainers to ship that tool by removing its need altogether, and we should be able to do that with certain ease by now. Today we have the snapctl tool that may be called by any snap from within hooks to do safe operations that the snap will likely care about, such as getting and changing its own configuration. We can extend that idea by allowing snapctl to be run outside of hooks. There’s some boilerplate for that, but @pstolowski already has it underway in PR #2644:

https://github.com/snapcore/snapd/pull/2644

That PR was closed only because Pawel has been busy elsewhere, but it’s reviewed and should be on its way soon (feel free to help with that).

Once that’s in, we can simply implement a command like snapctl open-link that will be handled by snapd proper, and the implementation of xdg-open can then be a one-liner call to that. This removes the need for snapd-xdg-open altogether, and the need to increase the burden on package maintainers.

1 Like
11

@niemeyer That sounds like a good idea. What about taking a two step approach here:

  1. We integrated snapd-xdg-open as it exists today into the snapd package and ship it as part of the snapd package and removing any further separate packaging with that from the distributions.
  2. We rework how we handle the real problem with the approach you suggested by sending the actual call through snapctl. Another aspect of this we need to think through is how snapd dispatches a snapctl open-link call back into the user session where it is running in a system wide context.

What we need to keep in mind is that we need to migrate existing users away from snapd-xdg-open and its dbus bus name. com.canonical.SafeLauncher. We have a separate xdg-open script for that in the core snap but we don’t know if everyone is using it and not accessing the bus name directly which is allowed by the unity7 interface.

12

It’s not clear why we’d spend time and energy integrating snapd-xdg-open into snapd, asking everybody to change the way its shipped, to then fix the same problem again in a better way and ask everybody to remove it.

Instead, we can invest our time on the final solution, and never have to worry about it again.

13

It’s two folded. Either we change existing users (Ubuntu, Debian) to drop the package for it or we take the additional packaging work for others users (openSUSE, Fedora, Arch) now to package it properly until we have a replacement in place. If we can stop possible users to never package it at all by integrating it now into the snapd then we don’t need them to worry about a possible migration to a newly implemented solution in the end, whenever it is ready. So regardless what we do, we spend time on packaging snapd-xdg-open in one or another way.

14

It’s not two-fold at all. It’s really win-win:

If you fix snapd to not require the package, you don’t even have to touch the package in Ubuntu and Debian. We’ll obviously want to remove it in the next distribution, but the fact it’s sitting on the system won’t hurt anyone. Every other distribution don’t have to worry at all. They’ll get it working out of the box with snapd.

If on the other hand you integrate snapd-xdg-open into the snapd repository, you’ll need to fix the package in Ubuntu and Debian to refer to the new location, and will need do the work to package the software to every single distribution, so that you can then to the above either way!

15

We don’t have to change the existing package in debian to look at a different location for the source. We more or less add a transitional package as we already do for other things (e.g. ubuntu-core-launcher) and it will get replaced on existing installations with the new transitional one. I don’t see this as much work and its already done in the PR linked above.

However, lets take snapd-xdg-open from where it is today and add necessary packages to the different distributions to have a fix for them real soon so we don’t loose existing functionality we have on Ubuntu there and have them as first class citizen. Afterwards we can work torwards a better solution if time and priority permits. I am adding this on my list.

This removes the need for snapd-xdg-open altogether, and the need to increase the burden on package maintainers.

This is not entirely true. As said above we still need to translate the call snapctl issues to snapd back into the user session context which snapd-xdg-open handled before as it was started as part of the user session.

snapd is part of the system context and does not have direct access into the user session to cause there a web link to be opened for example. So in the end we need again something living in the user session which translates the call back from snapd so it ends up in the right session context.

16

My biggest concern about this idea of “merging everything into the snapd repo” is that it’s just going to mask the lack of activity to all of these things. Moving snapd-xdg-open into the snapd repo will not make it better maintained.

The SELinux module was merged into snapd in the hopes that when Snappy developers actually change things, they’d fix the policy at the same time (like they do for AppArmor profiles). That obviously does not happen. Some of this is because it’s not packaged or used anywhere yet, but it’s not particularly hard to add a line for a new executable or delete one when it goes away. Will this get better now that Fedora has snapd builds that work? I don’t know, but I’m not holding my breath. I personally suspect that I will wind up doing everything alone until someone adds SELinux module packaging to the Debian and Ubuntu packages, because then someone else will have to care. With that kind of weight, why should have I bothered to merge it into the snapd repo?

I strongly suspect it’ll be exactly the same for snapd-xdg-open. For the most part, it is for snap-confine. It is almost exclusively worked on by @zyga-snapd alone. There were virtually no maintainability benefits to merging everything into snapd. If anything, it’s now harder to fix things in mainline because now we have to wait for someone to care to review the PR and merge it.

At first, I was for merging snap-confine into snapd, but as I watched the development and interacted with you guys, I became increasingly against it, because the whole process has too much friction and snapd alone is too complex to review. By the time the proposal to merge it came in, I was against it because it became clear to me that it’s not actually going to provide any material benefits. So far, from my point of view, I believe we haven’t gained anything from merging snap-confine into snapd, and if anything, it’s now more difficult to be agile. @zyga-snapd and I made more progress on snap-confine in Fedora when it was separate than when it was merged into snapd.

But frankly, I’m starting to not care one way or the other anymore, since apparently we want to have a monorepo. At least from the Fedora perspective, I’m likely to just maintain each component as subpackages unless there’s a good reason not to. That gives me the flexibility to easily deal with it being broken back out when it becomes unsustainable as a monorepo.

At least specifically in regards to snapd-xdg-open, the namespaces of the services should be changed as part of the merge from com.canonical.* to io.snapcraft.*. It should have happened when it was first put up on GitHub. I did file an issue about it several months ago, and brought it up in person even earlier. The fact nothing happened despite me bringing up twice in a short span of time is damning.

17

It will once we have CI enabled for Fedora. Then if you change things that break something in SELinux you don’t get to land it. SELinux is complicated and not everyone in the team runs Fedora (I suspect only two/three developers actively use it). Merging it was a step in the right direction because at least now we are one step closer to enabling CI. Working and understandable CI across several packages is harder to build and use. Inside one package / repository it is really easier.

18

Believe me that if you wanted to hack on snap-confine and ensure that it really works it is far easier because we didn’t have to maintain two sets of tests. Really, go and try it. Make a change and run all of snapd test suite. It’s a very reasonable assurance things work. We were burned by the two repositories in the past and the only reason it was a separate repository is because snap-confine was derived from ubuntu-core-launcher or something like that.

19

Folks, let’s please stop discussing and arguing about snapd-xdg-open. This is dead and should not be added to other distributions @morphis.

Let’s fix the real problem, now.

20

To add my two cents (which is not worth a lot), I would love to see this fixed regardless of the way in which that is done. My only concern is that as the developer I certainly don’t want to implement something special in order to handle snaps. If I am using xdg-open in my electron app, I would want to ensure I don’t have to rewrite how that works to specifically accommodate Snaps. I’m not sure that would be a problem, just telling you my only concern regarding how this is implemented.