Merge lp:~sergiusens/snappy/frameworkPath into lp:~snappy-dev/snappy/snappy-moved-to-github

[Sergio Schvezov]

Use /frameworks for framework snaps

Notice that this requires security/apparmor work.

[Michael Vogt (mvo)]

Thanks, this looks good.

review: Approve

[Michael Vogt (mvo)]

Does more need to change for this to land, i.e. do we need apparmor changes in lock-step?

[Sergio Schvezov (sergiusens)]

On Wed, May 27, 2015 at 06:40:01AM -0000, Michael Vogt wrote:
> Does more need to change for this to land, i.e. do we need apparmor changes in lock-step?

Not sure if in lockstep, but the apparmor changes would need to land
before this, reason for adding jdstrand as a reviewer.

[Jamie Strandboge (jdstrand)]

I've not looked at the code changes, but this states that it needs apparmor work. What is needed, adjustments to handle /frameworks instead of just /apps? If so, I think this should be sufficient in click-apparmor's click.py:

=== modified file 'src/apparmor/click.py'
--- src/apparmor/click.py 2015-04-15 21:32:33 +0000
+++ src/apparmor/click.py 2015-05-27 19:56:41 +0000
@@ -542,7 +542,7 @@
     # snappy hardcodes these and doing this allows snappy images to not require
     # click
     if os.path.exists("/usr/bin/snappy"):
- return "{%s}" % ",".join(["/apps", "/oem"])
+ return "{%s}" % ",".join(["/apps", "/oem", "/frameworks"])

     from gi.repository import Click

review: Needs Information

[Jamie Strandboge (jdstrand)]

Oh, and I *hate* 'if os.path.exists("/usr/bin/snappy")' - is there a better way to determine if this is an actual snappy system?

[lp:~sergiusens/snappy/frameworkPath]

472. By Sergio Schvezov on 2015-06-03

trunkpdate

473. By Sergio Schvezov on 2015-06-03

adding /frameworks to 'dirs'

474. By Sergio Schvezov on 2015-06-03

Adding /oem and /frameworks to purge list

[Sergio Schvezov (sergiusens)]

> I've not looked at the code changes, but this states that it needs apparmor
> work. What is needed, adjustments to handle /frameworks instead of just /apps?
> If so, I think this should be sufficient in click-apparmor's click.py:
>
> === modified file 'src/apparmor/click.py'
> --- src/apparmor/click.py 2015-04-15 21:32:33 +0000
> +++ src/apparmor/click.py 2015-05-27 19:56:41 +0000
> @@ -542,7 +542,7 @@
> # snappy hardcodes these and doing this allows snappy images to not
> require
> # click
> if os.path.exists("/usr/bin/snappy"):
> - return "{%s}" % ",".join(["/apps", "/oem"])
> + return "{%s}" % ",".join(["/apps", "/oem", "/frameworks"])
>
> from gi.repository import Click

With this installation works fine but the profile symlinks are wrong:

(amd64)ubuntu@localhost:~$ ls /var/lib/apparmor/snappy/profiles/ -l
total 0
lrwxrwxrwx 1 root ubuntu 36 Jun 3 19:10 webdm_snappyd_0.8 -> /apps/webdm/0.8/meta/snappyd.profile

that's a broken link; who creates the symlink these days? Snappy or apparmor?

[Jamie Strandboge (jdstrand)]

snappy creates the symlink in /var/lib/apparmor/clicks, and apparmor creates the profile in /var/lib/apparmor/profiles based on the symlink.

[Jamie Strandboge (jdstrand)]

Oh, webdm uses aa-profile-hook, not aa-clickapparmor. In that case, snappy creates the symlink in /var/lib/apparmor/snappy/profiles, then apparmor (aa-profilehook) creates the profile in /var/lib/apparmor/profiles.

(Gosh it will be nice to get this cleaned up)

[Sergio Schvezov (sergiusens)]

ah, stale files, actually no link is being created.

[Sergio Schvezov (sergiusens)]

ugh, this was related to the bug Chipaca mentioned and is ready for review here: https://code.launchpad.net/~chipaca/snappy/mangle/+merge/260934

When applying https://code.launchpad.net/~sergiusens/click-apparmor/snappyFameworksDir/+merge/261032 we should be good.

[John Lenton (chipaca)]

You've resurrected agreerator, which points to a bad merge; fix that.

Please also consider changing targetDirForType() to a method of type. This is more work, because you'd have to add something like SetRootDir to pkg, and call it from snappy's SetRootDir, so only do it if you're feeling fancy.

I suspect this is going to conflict with some of the branches i've had up for review for a few days now... sigh.

[Federico Gimenez (fgimenez)]

PASSED: Continuous integration, rev:474
http://10.55.60.183:8080/job/snappy-rolling-ci/28/
Executed test runs:

Click here to trigger a rebuild:
http://10.55.60.183:8080/job/snappy-rolling-ci/28/rebuild

review: Approve (continuous-integration)

[Leo Arias (elopio)]

this has a conflict with trunk.

review: Needs Fixing

Unmerged revisions

474. By Sergio Schvezov on 2015-06-03

Adding /oem and /frameworks to purge list

473. By Sergio Schvezov on 2015-06-03

adding /frameworks to 'dirs'

472. By Sergio Schvezov on 2015-06-03

trunkpdate

471. By Sergio Schvezov on 2015-05-26

Use /frameworks for framework snaps

=== modified file 'debian/ubuntu-snappy.dirs'
--- debian/ubuntu-snappy.dirs	2015-04-15 22:08:10 +0000
+++ debian/ubuntu-snappy.dirs	2015-06-03 19:51:29 +0000
@@ -1,2 +1,3 @@
 /apps
+/frameworks
 /oem

=== modified file 'snappy/click_test.go'
--- snappy/click_test.go	2015-06-03 14:03:45 +0000
+++ snappy/click_test.go	2015-06-03 19:51:29 +0000
@@ -245,6 +245,19 @@
 	c.Assert(err, NotNil)
 }
 
+type agreerator struct {
+	y       bool
+	intro   string
+	license string
+}
+
+func (a *agreerator) Agreed(intro, license string) bool {
+	a.intro = intro
+	a.license = license
+	return a.y
+}
+func (a *agreerator) Notify(string) {}
+
 // if the snap asks for accepting a license, and an agreer isn't provided,
 // install fails
 func (s *SnapTestSuite) TestLocalSnapInstallMissingAccepterFails(c *C) {
@@ -428,8 +441,8 @@
 	policy.SecBase = c.MkDir()
 
 	s.buildFramework(c)
-	appdir := filepath.Join(s.tempdir, "apps", "hello", "1.0.1")
-	c.Assert(removeClick(appdir, nil), IsNil)
+	fwDir := filepath.Join(s.tempdir, "frameworks", "hello", "1.0.1")
+	c.Assert(removeClick(fwDir, nil), IsNil)
 }
 
 func (s *SnapTestSuite) TestSnapRemovePackagePolicyWeirdClickManifest(c *C) {
@@ -438,13 +451,12 @@
 	policy.SecBase = c.MkDir()
 
 	s.buildFramework(c)
-	appdir := filepath.Join(s.tempdir, "apps", "hello", "1.0.1")
-	// c.Assert(removeClick(appdir, nil), IsNil)
+	fwDir := filepath.Join(s.tempdir, "frameworks", "hello", "1.0.1")
 
-	manifestFile := filepath.Join(appdir, ".click", "info", "hello.manifest")
+	manifestFile := filepath.Join(fwDir, ".click", "info", "hello.manifest")
 	c.Assert(ioutil.WriteFile(manifestFile, []byte(`{"name": "xyzzy","type":"framework"}`), 0644), IsNil)
 
-	c.Assert(removeClick(appdir, nil), IsNil)
+	c.Assert(removeClick(fwDir, nil), IsNil)
 }
 
 func (s *SnapTestSuite) TestLocalOemSnapInstall(c *C) {
@@ -915,12 +927,12 @@
 	c.Check(cmdlog, DeepEquals, []string{"stop", "show", "stop", "show", "start", "start"})
 
 	// check it got set active
-	content, err := ioutil.ReadFile(filepath.Join(snapAppsDir, "fmk", "current", "meta", "package.yaml"))
+	content, err := ioutil.ReadFile(filepath.Join(snapFrameworksDir, "fmk", "current", "meta", "package.yaml"))
 	c.Assert(err, IsNil)
 	c.Assert(strings.Contains(string(content), "version: 2"), Equals, true)
 
 	// just in case (cf. the following tests)
-	_, err = os.Stat(filepath.Join(snapAppsDir, "fmk", "2"))
+	_, err = os.Stat(filepath.Join(snapFrameworksDir, "fmk", "2"))
 	c.Assert(err, IsNil)
 
 }
@@ -944,12 +956,12 @@
 	c.Check(cmdlog, DeepEquals, []string{"stop", "show", "stop", "start"})
 
 	// check it got rolled back
-	content, err := ioutil.ReadFile(filepath.Join(snapAppsDir, "fmk", "current", "meta", "package.yaml"))
+	content, err := ioutil.ReadFile(filepath.Join(snapFrameworksDir, "fmk", "current", "meta", "package.yaml"))
 	c.Assert(err, IsNil)
 	c.Assert(strings.Contains(string(content), "version: 1"), Equals, true)
 
 	// no leftovers from the failed install
-	_, err = os.Stat(filepath.Join(snapAppsDir, "fmk", "2"))
+	_, err = os.Stat(filepath.Join(snapFrameworksDir, "fmk", "2"))
 	c.Assert(err, NotNil)
 }
 
@@ -975,12 +987,12 @@
 	})
 
 	// check it got rolled back
-	content, err := ioutil.ReadFile(filepath.Join(snapAppsDir, "fmk", "current", "meta", "package.yaml"))
+	content, err := ioutil.ReadFile(filepath.Join(snapFrameworksDir, "fmk", "current", "meta", "package.yaml"))
 	c.Assert(err, IsNil)
 	c.Assert(strings.Contains(string(content), "version: 1"), Equals, true)
 
 	// no leftovers from the failed install
-	_, err = os.Stat(filepath.Join(snapAppsDir, "fmk", "2"))
+	_, err = os.Stat(filepath.Join(snapFrameworksDir, "fmk", "2"))
 	c.Assert(err, NotNil)
 
 }

=== modified file 'snappy/dirs.go'
--- snappy/dirs.go	2015-05-15 13:33:27 +0000
+++ snappy/dirs.go	2015-06-03 19:51:29 +0000
@@ -19,19 +19,24 @@
 
 package snappy
 
-import "path/filepath"
+import (
+	"path/filepath"
+
+	"launchpad.net/snappy/pkg"
+)
 
 // the various file paths
 var (
 	globalRootDir string
 
-	snapAppsDir      string
-	snapOemDir       string
-	snapDataDir      string
-	snapDataHomeGlob string
-	snapAppArmorDir  string
-	snapSeccompDir   string
-	snapUdevRulesDir string
+	snapAppsDir       string
+	snapFrameworksDir string
+	snapOemDir        string
+	snapDataDir       string
+	snapDataHomeGlob  string
+	snapAppArmorDir   string
+	snapSeccompDir    string
+	snapUdevRulesDir  string
 
 	snapBinariesDir  string
 	snapServicesDir  string
@@ -47,6 +52,7 @@
 	globalRootDir = rootdir
 
 	snapAppsDir = filepath.Join(rootdir, "/apps")
+	snapFrameworksDir = filepath.Join(rootdir, "/frameworks")
 	snapOemDir = filepath.Join(rootdir, "/oem")
 	snapDataDir = filepath.Join(rootdir, "/var/lib/apps")
 	snapDataHomeGlob = filepath.Join(rootdir, "/home/*/apps/")
@@ -63,3 +69,20 @@
 
 	snapUdevRulesDir = filepath.Join(rootdir, "/etc/udev/rules.d")
 }
+
+func targetDirForType(t pkg.Type) string {
+	switch t {
+	case pkg.TypeApp:
+		return snapAppsDir
+	case pkg.TypeOem:
+		return snapOemDir
+	case pkg.TypeFramework:
+		return snapFrameworksDir
+	default:
+		return snapAppsDir
+	}
+}
+
+func allTargetDirs() []string {
+	return []string{snapAppsDir, snapFrameworksDir, snapOemDir}
+}

=== modified file 'snappy/parts.go'
--- snappy/parts.go	2015-05-20 17:24:29 +0000
+++ snappy/parts.go	2015-06-03 19:51:29 +0000
@@ -139,6 +139,9 @@
 	if repo := NewLocalSnapRepository(snapAppsDir); repo != nil {
 		m.all = append(m.all, repo)
 	}
+	if repo := NewLocalSnapRepository(snapFrameworksDir); repo != nil {
+		m.all = append(m.all, repo)
+	}
 	if repo := NewLocalSnapRepository(snapOemDir); repo != nil {
 		m.all = append(m.all, repo)
 	}

=== modified file 'snappy/purge.go'
--- snappy/purge.go	2015-05-20 17:24:29 +0000
+++ snappy/purge.go	2015-06-03 19:51:29 +0000
@@ -50,17 +50,19 @@
 	var active []*SnapPart
 
 	for _, datadir := range datadirs {
-		yamlPath := filepath.Join(snapAppsDir, datadir.QualifiedName(), datadir.Version, "meta", "package.yaml")
-		part, err := NewInstalledSnapPart(yamlPath, datadir.Origin)
-		if err != nil {
-			// no such part installed
-			continue
-		}
-		if part.IsActive() {
-			if !purgeActive {
-				return ErrStillActive
-			}
-			active = append(active, part)
+		for _, snapDir := range allTargetDirs() {
+			yamlPath := filepath.Join(snapDir, datadir.QualifiedName(), datadir.Version, "meta", "package.yaml")
+			part, err := NewInstalledSnapPart(yamlPath, datadir.Origin)
+			if err != nil {
+				// no such part installed
+				continue
+			}
+			if part.IsActive() {
+				if !purgeActive {
+					return ErrStillActive
+				}
+				active = append(active, part)
+			}
 		}
 	}
 

=== modified file 'snappy/snapp.go'
--- snappy/snapp.go	2015-05-29 13:22:14 +0000
+++ snappy/snapp.go	2015-06-03 19:51:29 +0000
@@ -484,11 +484,7 @@
 		return nil, err
 	}
 
-	targetDir := snapAppsDir
-	// the "oem" parts are special
-	if m.Type == pkg.TypeOem {
-		targetDir = snapOemDir
-	}
+	targetDir := targetDirForType(m.Type)
 	fullName := m.qualifiedName(origin)
 	instDir := filepath.Join(targetDir, fullName, m.Version)