Merge pull request #6767 from jhenstridge/icon-theme-support

wrappers: allow snaps to install icon theme icons

dirs/dirs.go

@@ -88,6 +88,7 @@ var (
 	SnapUserServicesDir string
 	SnapSystemdConfDir  string
 	SnapDesktopFilesDir string
+	SnapDesktopIconsDir string
 	SnapBusPolicyDir    string
 
 	SystemApparmorDir      string
@@ -242,7 +243,11 @@ func SetRootDir(rootdir string) {
 	SnapMountPolicyDir = filepath.Join(rootdir, snappyDir, "mount")
 	SnapMetaDir = filepath.Join(rootdir, snappyDir, "meta")
 	SnapBlobDir = SnapBlobDirUnder(rootdir)
+	// ${snappyDir}/desktop is added to $XDG_DATA_DIRS.
+	// Subdirectories are interpreted according to the relevant
+	// freedesktop.org specifications
 	SnapDesktopFilesDir = filepath.Join(rootdir, snappyDir, "desktop", "applications")
+	SnapDesktopIconsDir = filepath.Join(rootdir, snappyDir, "desktop", "icons")
 	SnapRunDir = filepath.Join(rootdir, "/run/snapd")
 	SnapRunNsDir = filepath.Join(SnapRunDir, "/ns")
 	SnapRunLockDir = filepath.Join(SnapRunDir, "/lock")

overlord/snapstate/backend/link.go

@@ -143,22 +143,41 @@ func (b Backend) StopServices(apps []*snap.AppInfo, reason snap.ServiceStopReaso
 	return wrappers.StopServices(apps, reason, meter, tm)
 }
 
-func generateWrappers(s *snap.Info) error {
+func generateWrappers(s *snap.Info) (err error) {
+	var cleanupFuncs []func(*snap.Info) error
+	defer func() {
+		if err != nil {
+			for _, cleanup := range cleanupFuncs {
+				cleanup(s)
+			}
+		}
+	}()
+
 	// add the CLI apps from the snap.yaml
-	if err := wrappers.AddSnapBinaries(s); err != nil {
+	if err = wrappers.AddSnapBinaries(s); err != nil {
 		return err
 	}
+	cleanupFuncs = append(cleanupFuncs, wrappers.RemoveSnapBinaries)
+
 	// add the daemons from the snap.yaml
-	if err := wrappers.AddSnapServices(s, progress.Null); err != nil {
-		wrappers.RemoveSnapBinaries(s)
+	if err = wrappers.AddSnapServices(s, progress.Null); err != nil {
 		return err
 	}
+	cleanupFuncs = append(cleanupFuncs, func(s *snap.Info) error {
+		return wrappers.RemoveSnapServices(s, progress.Null)
+	})
+
 	// add the desktop files
-	if err := wrappers.AddSnapDesktopFiles(s); err != nil {
-		wrappers.RemoveSnapServices(s, progress.Null)
-		wrappers.RemoveSnapBinaries(s)
+	if err = wrappers.AddSnapDesktopFiles(s); err != nil {
+		return err
+	}
+	cleanupFuncs = append(cleanupFuncs, wrappers.RemoveSnapDesktopFiles)
+
+	// add the desktop icons
+	if err = wrappers.AddSnapIcons(s); err != nil {
 		return err
 	}
+	cleanupFuncs = append(cleanupFuncs, wrappers.RemoveSnapIcons)
 
 	return nil
 }
@@ -179,7 +198,12 @@ func removeGeneratedWrappers(s *snap.Info, meter progress.Meter) error {
 		logger.Noticef("Cannot remove desktop files for %q: %v", s.InstanceName(), err3)
 	}
 
-	return firstErr(err1, err2, err3)
+	err4 := wrappers.RemoveSnapIcons(s)
+	if err4 != nil {
+		logger.Noticef("Cannot remove desktop icons for %q: %v", s.InstanceName(), err4)
+	}
+
+	return firstErr(err1, err2, err3, err4)
 }
 
 // UnlinkSnap makes the snap unavailable to the system removing wrappers and symlinks.

tests/lib/snaps/test-snapd-icon-theme/bin/echo

@@ -0,0 +1,2 @@
+#!/bin/sh
+echo "From test-snapd-icon-theme snap"

tests/lib/snaps/test-snapd-icon-theme/meta/gui/echo.desktop

@@ -0,0 +1,5 @@
+[Desktop Entry]
+Type=Application
+Name=Echo
+Exec=test-snapd-icon-theme.echo
+Icon=snap.test-snapd-icon-theme.foo

tests/lib/snaps/test-snapd-icon-theme/meta/snap.yaml

@@ -0,0 +1,6 @@
+name: test-snapd-icon-theme
+version: 1.0
+
+apps:
+    echo:
+        command: bin/echo

tests/main/parallel-install-snap-icons/task.yaml

@@ -0,0 +1,37 @@
+summary: Parallel installed snaps have non-conflicting icons
+
+restore: |
+    snap unset system experimental.parallel-instances
+
+execute: |
+    #shellcheck source=tests/lib/snaps.sh
+    . "$TESTSLIB"/snaps.sh
+
+    echo "Install a snap providing icons"
+    install_local test-snapd-icon-theme
+
+    echo "Install additional instances of the snap"
+    snap set system experimental.parallel-instances=true
+    install_local_as test-snapd-icon-theme test-snapd-icon-theme_longname
+    install_local_as test-snapd-icon-theme test-snapd-icon-theme_foo
+
+    echo "Each instance provides its own icons"
+    icondir=/var/lib/snapd/desktop/icons/hicolor/scalable/apps
+    [ -f "$icondir/snap.test-snapd-icon-theme.foo.svg" ]
+    [ -f "$icondir/snap.test-snapd-icon-theme_longname.foo.svg" ]
+    [ -f "$icondir/snap.test-snapd-icon-theme_foo.foo.svg" ]
+
+    echo "Each instance's desktop file references its own icon"
+    desktopdir=/var/lib/snapd/desktop/applications
+    MATCH '^Icon=snap.test-snapd-icon-theme.foo$' < "$desktopdir/test-snapd-icon-theme_echo.desktop"
+    MATCH '^Icon=snap.test-snapd-icon-theme_longname.foo$' < "$desktopdir/test-snapd-icon-theme+longname_echo.desktop"
+    MATCH '^Icon=snap.test-snapd-icon-theme_foo.foo$' < "$desktopdir/test-snapd-icon-theme+foo_echo.desktop"
+
+    echo "Removing once instance does not remove the other instances' icons"
+    snap remove test-snapd-icon-theme_foo
+    [ -f "$icondir/snap.test-snapd-icon-theme.foo.svg" ]
+    [ -f "$icondir/snap.test-snapd-icon-theme_longname.foo.svg" ]
+    [ ! -f "$icondir/snap.test-snapd-icon-theme_foo.foo.svg" ]
+
+    snap remove test-snapd-icon-theme
+    [ -f "$icondir/snap.test-snapd-icon-theme_longname.foo.svg" ]

tests/main/snap-icons/task.yaml

@@ -0,0 +1,29 @@
+summary: Snaps can install icon theme icons
+
+execute: |
+    #shellcheck source=tests/lib/snaps.sh
+    . "$TESTSLIB"/snaps.sh
+
+    echo "Install a snap providing icons"
+    install_local test-snapd-icon-theme
+
+    echo "Icons provided by the snap are installed to a shared location"
+    iconfile=/var/lib/snapd/desktop/icons/hicolor/scalable/apps/snap.test-snapd-icon-theme.foo.svg
+    [ -f "$iconfile" ]
+    MATCH "icon from test-snapd-icon-theme" < "$iconfile"
+
+    echo "Desktop files can reference installed icons"
+    desktopfile=/var/lib/snapd/desktop/applications/test-snapd-icon-theme_echo.desktop
+    MATCH '^Icon=snap.test-snapd-icon-theme.foo$' < "$desktopfile"
+
+    echo "Remove the snap"
+    snap remove test-snapd-icon-theme
+
+    echo "The icon has been removed"
+    [ ! -f "$iconfile" ]
+
+    echo "The empty icon theme subdirectories have also been removed"
+    [ ! -d /var/lib/snapd/desktop/icons/hicolor ]
+
+    echo "But the base icons directory remains"
+    [ -d /var/lib/snapd/desktop/icons ]

wrappers/desktop.go

@@ -142,6 +142,36 @@ func rewriteExecLine(s *snap.Info, desktopFile, line string) (string, error) {
 	return "", fmt.Errorf("invalid exec command: %q", cmd)
 }
 
+func rewriteIconLine(s *snap.Info, line string) (string, error) {
+	icon := strings.SplitN(line, "=", 2)[1]
+
+	// If there is a path separator, assume the icon is a path name
+	if strings.ContainsRune(icon, filepath.Separator) {
+		if !strings.HasPrefix(icon, "${SNAP}/") {
+			return "", fmt.Errorf("icon path %q is not part of the snap", icon)
+		}
+		if filepath.Clean(icon) != icon {
+			return "", fmt.Errorf("icon path %q is not canonicalized, did you mean %q?", icon, filepath.Clean(icon))
+		}
+		return line, nil
+	}
+
+	// If the icon is prefixed with "snap.${SNAP_NAME}.", rewrite
+	// to the instance name.
+	snapIconPrefix := fmt.Sprintf("snap.%s.", s.SnapName())
+	if strings.HasPrefix(icon, snapIconPrefix) {
+		return fmt.Sprintf("Icon=snap.%s.%s", s.InstanceName(), icon[len(snapIconPrefix):]), nil
+	}
+
+	// If the icon has any other "snap." prefix, treat this as an error.
+	if strings.HasPrefix(icon, "snap.") {
+		return "", fmt.Errorf("invalid icon name: %q, must start with %q", icon, snapIconPrefix)
+	}
+
+	// Allow other icons names through unchanged.
+	return line, nil
+}
+
 func sanitizeDesktopFile(s *snap.Info, desktopFile string, rawcontent []byte) []byte {
 	var newContent bytes.Buffer
 	mountDir := []byte(s.MountDir())
@@ -165,6 +195,16 @@ func sanitizeDesktopFile(s *snap.Info, desktopFile string, rawcontent []byte) []
 			bline = []byte(line)
 		}
 
+		// rewrite icon line if it references an icon theme icon
+		if bytes.HasPrefix(bline, []byte("Icon=")) {
+			line, err := rewriteIconLine(s, string(bline))
+			if err != nil {
+				logger.Debugf("ignoring icon in source desktop file %q: %s", filepath.Base(desktopFile), err)
+				continue
+			}
+			bline = []byte(line)
+		}
+
 		// do variable substitution
 		bline = bytes.Replace(bline, []byte("${SNAP}"), mountDir, -1)
 

wrappers/desktop_test.go

@@ -482,6 +482,68 @@ func (s *sanitizeDesktopFileSuite) TestLangLang(c *C) {
 	}
 }
 
+func (s *sanitizeDesktopFileSuite) TestRewriteIconLine(c *C) {
+	snap, err := snap.InfoFromSnapYaml([]byte(`
+name: snap
+version: 1.0
+`))
+	c.Assert(err, IsNil)
+
+	newl, err := wrappers.RewriteIconLine(snap, "Icon=${SNAP}/icon.png")
+	c.Check(err, IsNil)
+	c.Check(newl, Equals, "Icon=${SNAP}/icon.png")
+
+	newl, err = wrappers.RewriteIconLine(snap, "Icon=snap.snap.icon")
+	c.Check(err, IsNil)
+	c.Check(newl, Equals, "Icon=snap.snap.icon")
+
+	newl, err = wrappers.RewriteIconLine(snap, "Icon=other-icon")
+	c.Check(err, IsNil)
+	c.Check(newl, Equals, "Icon=other-icon")
+
+	snap.InstanceKey = "bar"
+	newl, err = wrappers.RewriteIconLine(snap, "Icon=snap.snap.icon")
+	c.Check(err, IsNil)
+	c.Check(newl, Equals, "Icon=snap.snap_bar.icon")
+
+	_, err = wrappers.RewriteIconLine(snap, "Icon=snap.othersnap.icon")
+	c.Check(err, ErrorMatches, `invalid icon name: "snap.othersnap.icon", must start with "snap.snap."`)
+
+	_, err = wrappers.RewriteIconLine(snap, "Icon=/etc/passwd")
+	c.Check(err, ErrorMatches, `icon path "/etc/passwd" is not part of the snap`)
+
+	_, err = wrappers.RewriteIconLine(snap, "Icon=${SNAP}/./icon.png")
+	c.Check(err, ErrorMatches, `icon path "\${SNAP}/./icon.png" is not canonicalized, did you mean "\${SNAP}/icon.png"\?`)
+
+	_, err = wrappers.RewriteIconLine(snap, "Icon=${SNAP}/../outside/icon.png")
+	c.Check(err, ErrorMatches, `icon path "\${SNAP}/../outside/icon.png" is not canonicalized, did you mean "outside/icon.png"\?`)
+}
+
+func (s *sanitizeDesktopFileSuite) TestSanitizeParallelInstancesIconName(c *C) {
+	snap, err := snap.InfoFromSnapYaml([]byte(`
+name: snap
+version: 1.0
+apps:
+ app:
+  command: cmd
+`))
+	snap.InstanceKey = "bar"
+	c.Assert(err, IsNil)
+	desktopContent := []byte(`[Desktop Entry]
+Name=foo
+Icon=snap.snap.icon
+Exec=snap.app
+`)
+	df := filepath.Base(snap.Apps["app"].DesktopFile())
+	e := wrappers.SanitizeDesktopFile(snap, df, desktopContent)
+	c.Assert(string(e), Equals, fmt.Sprintf(`[Desktop Entry]
+X-SnapInstanceName=snap_bar
+Name=foo
+Icon=snap.snap_bar.icon
+Exec=env BAMF_DESKTOP_FILE_HINT=snap_bar_app.desktop %s/bin/snap_bar.app
+`, dirs.SnapMountDir))
+}
+
 func (s *desktopSuite) TestAddRemoveDesktopFiles(c *C) {
 	var tests = []struct {
 		instance                string

wrappers/export_test.go

@@ -33,10 +33,14 @@ var (
 	// desktop
 	SanitizeDesktopFile    = sanitizeDesktopFile
 	RewriteExecLine        = rewriteExecLine
+	RewriteIconLine        = rewriteIconLine
 	IsValidDesktopFileLine = isValidDesktopFileLine
 
 	// timers
 	GenerateOnCalendarSchedules = generateOnCalendarSchedules
+
+	// icons
+	FindIconFiles = findIconFiles
 )
 
 func MockKillWait(wait time.Duration) (restore func()) {