interfaces: mount host system fonts in desktop interface #3889
Conversation
interfaces/builtin/desktop_test.go
Outdated
| for _, entry := range entries { | ||
| c.Check(expectedMountPoints, testutil.Contains, entry.Dir) | ||
| c.Check(entry.Name, Equals, "/var/lib/snapd/hostfs"+entry.Dir) | ||
| c.Check(entry.Options, DeepEquals, []string{"bind", "ro"}) |
There was a problem hiding this comment.
This will probably need corresponding snap-confine apparmor profile updates. While I recall adding one for /usr/share/fonts well mid last year I'd like to check if all of those are present.
There was a problem hiding this comment.
I didn't notice any problems in my manual testing. I wonder if snap-update-ns got in first so I never hit that code path in snap-confine?
Codecov Report
@@ Coverage Diff @@
## master #3889 +/- ##
==========================================
+ Coverage 75.9% 75.94% +0.04%
==========================================
Files 416 416
Lines 35988 36016 +28
==========================================
+ Hits 27315 27351 +36
+ Misses 6753 6750 -3
+ Partials 1920 1915 -5
Continue to review full report at Codecov.
|
interfaces/builtin/desktop.go
Outdated
| var desktopFontconfigDirs = []string{ | ||
| "/usr/share/fonts", | ||
| "/usr/local/share/fonts", | ||
| "/var/cache/fontconfig", |
There was a problem hiding this comment.
I think these need to be set in dirs, so they inherit the global root dir.
Also, how cross-distro are these locations?
There was a problem hiding this comment.
Also, what happens if a different base snap doesn't have the mount point location available? Does it fail gracefully, or does it break the mounting sequence? @zyga?
There was a problem hiding this comment.
I'll have a look at defining these through the dirs package.
The locations are fairly consistent. /usr/share/fonts is fontconfig's default font location. The /usr/local/share/fonts directory isn't part of upstream fontconfig, but is commonly configured by major distros:
https://sources.debian.net/src/fontconfig/2.12.3-0.2/debian/rules/#L20
https://src.fedoraproject.org/rpms/fontconfig/blob/master/f/fontconfig.spec#_69
https://build.opensuse.org/package/view_file/openSUSE:Factory/fontconfig/fontconfig.spec?expand=1
https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/fontconfig#n44
The fontconfig cache directory follows autoconf's $localstatedir directory, which is generally always configured as /var on Linux.
As for base snaps, I can see two solutions: (1) require base snaps to create these mount points in the same way they'd create /etc, /home, etc, or (2) rely on the upcoming layouts feature to ensure the mount points are available.
There was a problem hiding this comment.
The conversion from simple style interface to complex style interface looks fine (though, please add the comment I requested).
Please make the small testsuite change I requested to make it consistent with other (modern) interface test code.
As for cross-distro, I can't answer but I can say that the method @jhenstridge came up with seems sound to me. He has a list of directories to check for existence on the host and only adds the mountpoint if they exist. I can say that looking at /etc/apparmor.d/abstractions/fonts they are consistent (the things missing here that are in the abstraction I wouldn't want to add without seeing them used in the wild, with the possible exception of /var/cache/fonts). If we are missing any for cross-distro, they can easily be added to the list in future PRs.
As for the mountpoints not being in the core snap, snap-confine has a concept for handling quirks, but I think code needs to be added to support these. I'll let @zyga comment.
Since @niemeyer is blocking on the mountpoint existence in core snap question, there is no reason for me to also block. Since my requested changes are small, marking as approved (though, again, please make the requested changes).
interfaces/builtin/desktop.go
Outdated
| } | ||
|
|
||
| func (iface *desktopInterface) AutoConnect(*interfaces.Plug, *interfaces.Slot) bool { | ||
| return true |
There was a problem hiding this comment.
Can you add a comment here like we do in other interfaces:
// allow what declarations allowed
There was a problem hiding this comment.
I wonder when we can get rid of AutoConnect altogether...
interfaces/builtin/desktop_test.go
Outdated
|
|
||
| // On classic systems, a number of font related directories | ||
| // are bind mounted from the host system if they exist. | ||
| release.OnClassic = true |
There was a problem hiding this comment.
Typically we'd do this in the testsuite instead:
// on a classic system with desktop slot coming from the core snap,
// a number of font related directories are bind mounted from the
// host system if they exist.
restore = release.MockOnClassic(true)
defer restore()
There was a problem hiding this comment.
I'm already using MockOnClassic earlier to save the state. I'll switch this case to use MockOnClassic too, just in case someone splits the test later and leaks state.
There was a problem hiding this comment.
I don't have a good answer to the question of base snaps wanting to ship fonts. I can imagine that being useful on Ubuntu Core systems (which the desktop interface doesn't currently work on), but why would you want to have different fonts than the host on classic?
And unless the author of the base snap ensures they cover all the world's languages, it is going to lock out some users. In contrast, we can take it as a given that the host system contains fonts capable of displaying the user's native language.
interfaces/builtin/desktop.go
Outdated
| var desktopFontconfigDirs = []string{ | ||
| "/usr/share/fonts", | ||
| "/usr/local/share/fonts", | ||
| "/var/cache/fontconfig", |
There was a problem hiding this comment.
I'll have a look at defining these through the dirs package.
The locations are fairly consistent. /usr/share/fonts is fontconfig's default font location. The /usr/local/share/fonts directory isn't part of upstream fontconfig, but is commonly configured by major distros:
https://sources.debian.net/src/fontconfig/2.12.3-0.2/debian/rules/#L20
https://src.fedoraproject.org/rpms/fontconfig/blob/master/f/fontconfig.spec#_69
https://build.opensuse.org/package/view_file/openSUSE:Factory/fontconfig/fontconfig.spec?expand=1
https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/fontconfig#n44
The fontconfig cache directory follows autoconf's $localstatedir directory, which is generally always configured as /var on Linux.
As for base snaps, I can see two solutions: (1) require base snaps to create these mount points in the same way they'd create /etc, /home, etc, or (2) rely on the upcoming layouts feature to ensure the mount points are available.
interfaces/builtin/desktop.go
Outdated
| } | ||
|
|
||
| func (iface *desktopInterface) AutoConnect(*interfaces.Plug, *interfaces.Slot) bool { | ||
| return true |
interfaces/builtin/desktop_test.go
Outdated
|
|
||
| // On classic systems, a number of font related directories | ||
| // are bind mounted from the host system if they exist. | ||
| release.OnClassic = true |
There was a problem hiding this comment.
I'm already using MockOnClassic earlier to save the state. I'll switch this case to use MockOnClassic too, just in case someone splits the test later and leaks state.
|
If anyone else will came here with question "why no fonts in my snapped app" — make sure that you did I am electron-builder developer. Now, if I use freshly installed Ubuntu 16, no fonts in the Electron apps — apparmor denied access to fonts. Could you please confirm that this feature is available for Ubuntu 16 or when it will be released? And after system restart, fonts stopped worked again... |
|
I have added test snap to https://bugs.launchpad.net/snappy/+bug/1621568/comments/6 |
|
@develar - this is probably https://forum.snapcraft.io/t/fonts-fail-to-load-when-desktop-plug-added/3414/2 (which has information on what to do to fix your snap). This is an electron bug that should be fixed in newer releases. See electron/electron#11628 |
|
@jdstrand Thanks for answer. Not clear for me how linked issue is related to fonts issue. Anyway — I will fix electron-builder to do |
|
@develar - this denial: is basically the same as the denials listed in the forum topic I mentioned. Specifically, you have an mmap denial on a font. This coupled with the fact that electron builds sometimes end up with executable stacks lead me to my response. Good luck! |
|
@jdstrand Thanks, indeed, it is was a cause. electron-builder uses custom code in go to clear exec stack now (so, it works also on macOS). |
On classic systems, bind mount the host system fonts and fontconfig cache into the sandbox for snaps using the
desktopinterface. This is based on discussions on the forum here:https://forum.snapcraft.io/t/desktop-allow-access-to-host-system-fonts/1796
The changes in this PR depend on certain directories existing in the core snap to be used as mount points. At present this is only available in the edge channel, which can be installed with:
Here is a basic manual test plan:
Install fontconfig_0.1_amd64.snap using vanilla snapd. This snap contains the fontconfig command line utilities.
Run
fontconfig.fc-listand note that no fonts are visible.Switch to snapd from this PR.
Reconnect the
desktopinterface to make sure we've got the new rules:Run
fontconfig.fc-listand note that it lists all the host system fonts.Run
snap run --shell fontconfig.fc-listto open a shell within the sandbox to verify that fonts are visible under/usr/share/fonts.Run
This checks the validity of the caches for all system font dirs, and updates any invalid ones. You should see many "skipping, existing cache is valid" messages for the directories containing fonts.
Check that no new caches were written to the
~/snap/fontconfig/current/.cache/fontconfigdirectory.