Merge lp:~chipaca/snappy/clickety into lp:~snappy-dev/snappy/snappy-moved-to-github

[John Lenton]

Chipping away at installClick.

[Sergio Schvezov (sergiusens)]

some initial comments before breakfast ;-)

[Michael Vogt (mvo)]

Looks good, some comments inline, only a single one is really relevant I think.

[lp:~chipaca/snappy/clickety]

477. By John Lenton on 2015-05-28

tweaks as per review comments

[Michael Vogt (mvo)]

Thanks!

review: Approve

=== modified file 'clickdeb/deb.go'
--- clickdeb/deb.go	2015-05-28 11:27:43 +0000
+++ clickdeb/deb.go	2015-05-28 14:48:17 +0000
@@ -198,6 +198,18 @@
 	return content, nil
 }
 
+// ExtractHashes reads "hashes.yaml" from the clickdeb and writes it to
+// the given directory
+func (d *ClickDeb) ExtractHashes(dir string) error {
+	hashesFile := filepath.Join(dir, "hashes.yaml")
+	hashesData, err := d.ControlMember("hashes.yaml")
+	if err != nil {
+		return err
+	}
+
+	return ioutil.WriteFile(hashesFile, hashesData, 0644)
+}
+
 // Unpack unpacks the data.tar.{gz,bz2,xz} into the given target directory
 // with click specific verification, i.e. no files will be extracted outside
 // of the targetdir (no ".." inside the data.tar is allowed)

=== modified file 'snappy/click.go'
--- snappy/click.go	2015-05-28 11:27:43 +0000
+++ snappy/click.go	2015-05-28 14:48:17 +0000
@@ -47,6 +47,7 @@
 	"launchpad.net/snappy/logger"
 	"launchpad.net/snappy/pkg"
 	"launchpad.net/snappy/policy"
+	"launchpad.net/snappy/progress"
 	"launchpad.net/snappy/systemd"
 
 	"github.com/mvo5/goconfigparser"
@@ -290,16 +291,6 @@
 	return nil
 }
 
-func writeHashesFile(d *clickdeb.ClickDeb, instDir string) error {
-	hashesFile := filepath.Join(instDir, "meta", "hashes.yaml")
-	hashesData, err := d.ControlMember("hashes.yaml")
-	if err != nil {
-		return err
-	}
-
-	return ioutil.WriteFile(hashesFile, hashesData, 0644)
-}
-
 // generate the name
 func generateBinaryName(m *packageYaml, binary Binary) string {
 	var binName string
@@ -757,237 +748,15 @@
 	return nil
 }
 
-func installClick(snapFile string, flags InstallFlags, inter interacter, origin string) (name string, err error) {
+func installClick(snapFile string, flags InstallFlags, inter progress.Meter, origin string) (name string, err error) {
 	allowUnauthenticated := (flags & AllowUnauthenticated) != 0
-	if err := auditClick(snapFile, allowUnauthenticated); err != nil {
-		return "", err
-		// ?
-		//return SnapAuditError
-	}
-
-	d, err := clickdeb.Open(snapFile)
-	if err != nil {
-		return "", err
-	}
-	defer d.Close()
-
-	manifestData, err := d.ControlMember("manifest")
-	if err != nil {
-		logger.Noticef("Snap inspect failed for %q: %v", snapFile, err)
-		return "", err
-	}
-
-	yamlData, err := d.MetaMember("package.yaml")
-	if err != nil {
-		return "", err
-	}
-
-	m, err := parsePackageYamlData(yamlData)
-	if err != nil {
-		return "", err
-	}
-
-	if err := m.checkForPackageInstalled(origin); err != nil {
-		return "", err
-	}
-
-	if err := m.checkForNameClashes(); err != nil {
-		return "", err
-	}
-
-	if err := m.checkForFrameworks(); err != nil {
-		return "", err
-	}
-
-	targetDir := snapAppsDir
-	// the "oem" parts are special
-	if m.Type == pkg.TypeOem {
-		targetDir = snapOemDir
-
-		// TODO do the following at a higher level once the store publishes snap types
-		// this is horrible
-		if allowOEM := (flags & AllowOEM) != 0; !allowOEM {
-			if currentOEM, err := getOem(); err == nil {
-				if currentOEM.Name != m.Name {
-					return "", ErrOEMPackageInstall
-				}
-			} else {
-				// there should always be an oem package now
-				return "", ErrOEMPackageInstall
-			}
-		}
-
-		if err := installOemHardwareUdevRules(m); err != nil {
-			return "", err
-		}
-	}
-
-	fullName := m.qualifiedName(origin)
-	instDir := filepath.Join(targetDir, fullName, m.Version)
-	currentActiveDir, _ := filepath.EvalSymlinks(filepath.Join(instDir, "..", "current"))
-
-	if err := m.checkLicenseAgreement(inter, d, currentActiveDir); err != nil {
-		return "", err
-	}
-
-	dataDir := filepath.Join(snapDataDir, fullName, m.Version)
-
-	if err := os.MkdirAll(instDir, 0755); err != nil {
-		logger.Noticef("Can not create %q: %v", instDir, err)
-		return "", err
-	}
-
-	// if anything goes wrong here we cleanup
-	defer func() {
-		if err != nil {
-			if e := os.RemoveAll(instDir); e != nil && !os.IsNotExist(e) {
-				logger.Noticef("Failed to remove %q: %v", instDir, e)
-			}
-		}
-	}()
-
-	// we need to call the external helper so that we can reliable drop
-	// privs
-	if err := d.UnpackWithDropPrivs(instDir, globalRootDir); err != nil {
-		return "", err
-	}
-
-	// legacy, the hooks (e.g. apparmor) need this. Once we converted
-	// all hooks this can go away
-	clickMetaDir := path.Join(instDir, ".click", "info")
-	if err := os.MkdirAll(clickMetaDir, 0755); err != nil {
-		return "", err
-	}
-	if err := writeCompatManifestJSON(clickMetaDir, manifestData, origin); err != nil {
-		return "", err
-	}
-
-	// write the hashes now
-	if err := writeHashesFile(d, instDir); err != nil {
-		return "", err
-	}
-
-	inhibitHooks := (flags & InhibitHooks) != 0
-
-	// deal with the data:
-	//
-	// if there was a previous version, stop it
-	// from being active so that it stops running and can no longer be
-	// started then copy the data
-	//
-	// otherwise just create a empty data dir
-	if currentActiveDir != "" {
-		oldM, err := parsePackageYamlFile(filepath.Join(currentActiveDir, "meta", "package.yaml"))
-		if err != nil {
-			return "", err
-		}
-
-		// we need to stop making it active
-		err = unsetActiveClick(currentActiveDir, inhibitHooks, inter)
-		defer func() {
-			if err != nil {
-				if cerr := setActiveClick(currentActiveDir, inhibitHooks, inter); cerr != nil {
-					logger.Noticef("Setting old version back to active failed: %v", cerr)
-				}
-			}
-		}()
-		if err != nil {
-			return "", err
-		}
-
-		err = copySnapData(fullName, oldM.Version, m.Version)
-	} else {
-		err = os.MkdirAll(dataDir, 0755)
-	}
-
-	defer func() {
-		if err != nil {
-			if cerr := removeSnapData(fullName, m.Version); cerr != nil {
-				logger.Noticef("When cleaning up data for %s %s: %v", m.Name, m.Version, cerr)
-			}
-		}
-	}()
-
-	if err != nil {
-		return "", err
-	}
-
-	// and finally make active
-	err = setActiveClick(instDir, inhibitHooks, inter)
-	defer func() {
-		if err != nil && currentActiveDir != "" {
-			if cerr := setActiveClick(currentActiveDir, inhibitHooks, inter); cerr != nil {
-				logger.Noticef("When setting old %s version back to active: %v", m.Name, cerr)
-			}
-		}
-	}()
-	if err != nil {
-		return "", err
-	}
-
-	// oh, one more thing: refresh the security bits
-	if !inhibitHooks {
-		part, err := NewSnapPartFromYaml(filepath.Join(instDir, "meta", "package.yaml"), origin, m)
-		if err != nil {
-			return "", err
-		}
-
-		deps, err := part.Dependents()
-		if err != nil {
-			return "", err
-		}
-
-		sysd := systemd.New(globalRootDir, inter)
-		stopped := make(map[string]time.Duration)
-		defer func() {
-			if err != nil {
-				for serviceName := range stopped {
-					if e := sysd.Start(serviceName); e != nil {
-						inter.Notify(fmt.Sprintf("unable to restart %s with the old %s: %s", serviceName, part.Name(), e))
-					}
-				}
-			}
-		}()
-
-		for _, dep := range deps {
-			if !dep.IsActive() {
-				continue
-			}
-			for _, svc := range dep.Services() {
-				serviceName := filepath.Base(generateServiceFileName(dep.m, svc))
-				timeout := time.Duration(svc.StopTimeout)
-				if err = sysd.Stop(serviceName, timeout); err != nil {
-					inter.Notify(fmt.Sprintf("unable to stop %s; aborting install: %s", serviceName, err))
-					return "", err
-				}
-				stopped[serviceName] = timeout
-			}
-		}
-
-		if err := part.RefreshDependentsSecurity(currentActiveDir, inter); err != nil {
-			return "", err
-		}
-
-		started := make(map[string]time.Duration)
-		defer func() {
-			if err != nil {
-				for serviceName, timeout := range started {
-					if e := sysd.Stop(serviceName, timeout); e != nil {
-						inter.Notify(fmt.Sprintf("unable to stop %s with the old %s: %s", serviceName, part.Name(), e))
-					}
-				}
-			}
-		}()
-		for serviceName, timeout := range stopped {
-			if err = sysd.Start(serviceName); err != nil {
-				inter.Notify(fmt.Sprintf("unable to restart %s; aborting install: %s", serviceName, err))
-				return "", err
-			}
-			started[serviceName] = timeout
-		}
-	}
-
-	return m.Name, nil
+	part, err := NewSnapPartFromSnapFile(snapFile, origin, allowUnauthenticated)
+	if err != nil {
+		return "", err
+	}
+	defer part.deb.Close()
+
+	return part.Install(inter, flags)
 }
 
 // removeSnapData removes the data for the given version of the given snap

=== modified file 'snappy/click_test.go'
--- snappy/click_test.go	2015-05-28 11:27:43 +0000
+++ snappy/click_test.go	2015-05-28 14:48:17 +0000
@@ -246,19 +246,6 @@
 	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) {
@@ -271,7 +258,7 @@
 // Agreed returns false, install fails
 func (s *SnapTestSuite) TestLocalSnapInstallNegAccepterFails(c *C) {
 	pkg := makeTestSnapPackage(c, "explicit-license-agreement: Y")
-	_, err := installClick(pkg, 0, &agreerator{y: false}, testOrigin)
+	_, err := installClick(pkg, 0, &MockProgressMeter{y: false}, testOrigin)
 	c.Check(err, Equals, ErrLicenseNotAccepted)
 }
 
@@ -282,7 +269,7 @@
 	defer func() { licenseChecker = checkLicenseExists }()
 
 	pkg := makeTestSnapPackageFull(c, "explicit-license-agreement: Y", false)
-	_, err := installClick(pkg, 0, &agreerator{y: true}, testOrigin)
+	_, err := installClick(pkg, 0, &MockProgressMeter{y: true}, testOrigin)
 	c.Check(err, Equals, ErrLicenseNotProvided)
 }
 
@@ -290,14 +277,14 @@
 // Agreed returns true, install succeeds
 func (s *SnapTestSuite) TestLocalSnapInstallPosAccepterWorks(c *C) {
 	pkg := makeTestSnapPackage(c, "explicit-license-agreement: Y")
-	_, err := installClick(pkg, 0, &agreerator{y: true}, testOrigin)
+	_, err := installClick(pkg, 0, &MockProgressMeter{y: true}, testOrigin)
 	c.Check(err, Equals, nil)
 }
 
 // Agreed is given reasonable values for intro and license
 func (s *SnapTestSuite) TestLocalSnapInstallAccepterReasonable(c *C) {
 	pkg := makeTestSnapPackage(c, "name: foobar\nexplicit-license-agreement: Y")
-	ag := &agreerator{y: true}
+	ag := &MockProgressMeter{y: true}
 	_, err := installClick(pkg, 0, ag, testOrigin)
 	c.Assert(err, Equals, nil)
 	c.Check(ag.intro, Matches, ".*foobar.*requires.*license.*")
@@ -307,7 +294,7 @@
 // If a previous version is installed with the same license version, the agreer
 // isn't called
 func (s *SnapTestSuite) TestPreviouslyAcceptedLicense(c *C) {
-	ag := &agreerator{y: true}
+	ag := &MockProgressMeter{y: true}
 	yaml := "name: foox\nexplicit-license-agreement: Y\nlicense-version: 2\n"
 	yamlFile, err := makeInstalledMockSnap(s.tempdir, yaml+"version: 1")
 	pkgdir := filepath.Dir(filepath.Dir(yamlFile))
@@ -325,7 +312,7 @@
 // If a previous version is installed with the same license version, but without
 // explicit license agreement set, the agreer *is* called
 func (s *SnapTestSuite) TestSameLicenseVersionButNotRequired(c *C) {
-	ag := &agreerator{y: true}
+	ag := &MockProgressMeter{y: true}
 	yaml := "name: foox\nlicense-version: 2\n"
 	yamlFile, err := makeInstalledMockSnap(s.tempdir, yaml+"version: 1")
 	pkgdir := filepath.Dir(filepath.Dir(yamlFile))
@@ -342,7 +329,7 @@
 // If a previous version is installed with a different license version, the
 // agreer *is* called
 func (s *SnapTestSuite) TestDifferentLicenseVersion(c *C) {
-	ag := &agreerator{y: true}
+	ag := &MockProgressMeter{y: true}
 	yaml := "name: foox\nexplicit-license-agreement: Y\n"
 	yamlFile, err := makeInstalledMockSnap(s.tempdir, yaml+"license-version: 2\nversion: 1")
 	pkgdir := filepath.Dir(filepath.Dir(yamlFile))

=== modified file 'snappy/common_test.go'
--- snappy/common_test.go	2015-05-20 17:24:29 +0000
+++ snappy/common_test.go	2015-05-28 14:48:17 +0000
@@ -64,7 +64,7 @@
 		return "", err
 	}
 
-	dirName := fmt.Sprintf("%s.%s", m.Name, testOrigin)
+	dirName := m.qualifiedName(testOrigin)
 	metaDir := filepath.Join(tempdir, "apps", dirName, m.Version, "meta")
 	if err := os.MkdirAll(metaDir, 0775); err != nil {
 		return "", err
@@ -191,7 +191,12 @@
 	spin     bool
 	spinMsg  string
 	written  int
+	// Notifier:
 	notified []string
+	// Agreer:
+	intro   string
+	license string
+	y       bool
 }
 
 func (m *MockProgressMeter) Start(pkg string, total float64) {
@@ -214,8 +219,10 @@
 func (m *MockProgressMeter) Finished() {
 	m.finished = true
 }
-func (m *MockProgressMeter) Agreed(string, string) bool {
-	return false
+func (m *MockProgressMeter) Agreed(intro, license string) bool {
+	m.intro = intro
+	m.license = license
+	return m.y
 }
 func (m *MockProgressMeter) Notify(msg string) {
 	m.notified = append(m.notified, msg)

=== modified file 'snappy/oem.go'
--- snappy/oem.go	2015-05-28 11:17:44 +0000
+++ snappy/oem.go	2015-05-28 14:48:17 +0000
@@ -102,7 +102,7 @@
 // logic for an oem package in every other function
 var getOem = getOemImpl
 
-var getOemImpl = func() (*packageYaml, error) {
+func getOemImpl() (*packageYaml, error) {
 	oems, _ := ActiveSnapsByType(pkg.TypeOem)
 	if len(oems) == 1 {
 		return oems[0].(*SnapPart).m, nil

=== modified file 'snappy/snapp.go'
--- snappy/snapp.go	2015-05-20 17:24:29 +0000
+++ snappy/snapp.go	2015-05-28 14:48:17 +0000
@@ -22,7 +22,6 @@
 import (
 	"bytes"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -46,6 +45,7 @@
 	"launchpad.net/snappy/policy"
 	"launchpad.net/snappy/progress"
 	"launchpad.net/snappy/release"
+	"launchpad.net/snappy/systemd"
 )
 
 const (
@@ -170,8 +170,8 @@
 	isActive    bool
 	isInstalled bool
 	description string
-
-	basedir string
+	deb         *clickdeb.ClickDeb
+	basedir     string
 }
 
 var commasplitter = regexp.MustCompile(`\s*,\s*`).Split
@@ -456,10 +456,50 @@
 	if err != nil {
 		return nil, err
 	}
+	part.isInstalled = true
 
 	return part, nil
 }
 
+// NewSnapPartFromSnapFile loads a snap from the given (clickdeb) snap file.
+// Caller should call Close on the clickdeb.
+// TODO: expose that Close.
+func NewSnapPartFromSnapFile(snapFile string, origin string, unauthOk bool) (*SnapPart, error) {
+	if err := clickdeb.Verify(snapFile, unauthOk); err != nil {
+		return nil, err
+	}
+
+	d, err := clickdeb.Open(snapFile)
+	if err != nil {
+		return nil, err
+	}
+
+	yamlData, err := d.MetaMember("package.yaml")
+	if err != nil {
+		return nil, err
+	}
+
+	m, err := parsePackageYamlData(yamlData)
+	if err != nil {
+		return nil, err
+	}
+
+	targetDir := snapAppsDir
+	// the "oem" parts are special
+	if m.Type == pkg.TypeOem {
+		targetDir = snapOemDir
+	}
+	fullName := m.qualifiedName(origin)
+	instDir := filepath.Join(targetDir, fullName, m.Version)
+
+	return &SnapPart{
+		basedir: instDir,
+		origin:  origin,
+		m:       m,
+		deb:     d,
+	}, nil
+}
+
 // NewSnapPartFromYaml returns a new SnapPart from the given *packageYaml at yamlPath
 func NewSnapPartFromYaml(yamlPath, origin string, m *packageYaml) (*SnapPart, error) {
 	if _, err := os.Stat(yamlPath); err != nil {
@@ -467,10 +507,9 @@
 	}
 
 	part := &SnapPart{
-		basedir:     filepath.Dir(filepath.Dir(yamlPath)),
-		isInstalled: true,
-		origin:      origin,
-		m:           m,
+		basedir: filepath.Dir(filepath.Dir(yamlPath)),
+		origin:  origin,
+		m:       m,
 	}
 
 	// check if the part is active
@@ -610,8 +649,184 @@
 }
 
 // Install installs the snap
-func (s *SnapPart) Install(pb progress.Meter, flags InstallFlags) (name string, err error) {
-	return "", errors.New("Install of a local part is not possible")
+func (s *SnapPart) Install(inter progress.Meter, flags InstallFlags) (name string, err error) {
+	allowOEM := (flags & AllowOEM) != 0
+	inhibitHooks := (flags & InhibitHooks) != 0
+
+	if s.IsInstalled() {
+		return "", ErrAlreadyInstalled
+	}
+
+	if err := s.CanInstall(allowOEM, inter); err != nil {
+		return "", err
+	}
+
+	manifestData, err := s.deb.ControlMember("manifest")
+	if err != nil {
+		logger.Noticef("Snap inspect failed for %q: %v", s.Name(), err)
+		return "", err
+	}
+
+	// the "oem" parts are special
+	if s.Type() == pkg.TypeOem {
+		if err := installOemHardwareUdevRules(s.m); err != nil {
+			return "", err
+		}
+	}
+
+	fullName := QualifiedName(s)
+	currentActiveDir, _ := filepath.EvalSymlinks(filepath.Join(s.basedir, "..", "current"))
+	dataDir := filepath.Join(snapDataDir, fullName, s.Version())
+
+	if err := os.MkdirAll(s.basedir, 0755); err != nil {
+		logger.Noticef("Can not create %q: %v", s.basedir, err)
+		return "", err
+	}
+
+	// if anything goes wrong here we cleanup
+	defer func() {
+		if err != nil {
+			if e := os.RemoveAll(s.basedir); e != nil && !os.IsNotExist(e) {
+				logger.Noticef("Failed to remove %q: %v", s.basedir, e)
+			}
+		}
+	}()
+
+	// we need to call the external helper so that we can reliable drop
+	// privs
+	if err := s.deb.UnpackWithDropPrivs(s.basedir, globalRootDir); err != nil {
+		return "", err
+	}
+
+	// legacy, the hooks (e.g. apparmor) need this. Once we converted
+	// all hooks this can go away
+	clickMetaDir := filepath.Join(s.basedir, ".click", "info")
+	if err := os.MkdirAll(clickMetaDir, 0755); err != nil {
+		return "", err
+	}
+	if err := writeCompatManifestJSON(clickMetaDir, manifestData, s.origin); err != nil {
+		return "", err
+	}
+
+	// write the hashes now
+	if err := s.deb.ExtractHashes(filepath.Join(s.basedir, "meta")); err != nil {
+		return "", err
+	}
+
+	// deal with the data:
+	//
+	// if there was a previous version, stop it
+	// from being active so that it stops running and can no longer be
+	// started then copy the data
+	//
+	// otherwise just create a empty data dir
+	if currentActiveDir != "" {
+		oldM, err := parsePackageYamlFile(filepath.Join(currentActiveDir, "meta", "package.yaml"))
+		if err != nil {
+			return "", err
+		}
+
+		// we need to stop making it active
+		err = unsetActiveClick(currentActiveDir, inhibitHooks, inter)
+		defer func() {
+			if err != nil {
+				if cerr := setActiveClick(currentActiveDir, inhibitHooks, inter); cerr != nil {
+					logger.Noticef("Setting old version back to active failed: %v", cerr)
+				}
+			}
+		}()
+		if err != nil {
+			return "", err
+		}
+
+		err = copySnapData(fullName, oldM.Version, s.Version())
+	} else {
+		err = os.MkdirAll(dataDir, 0755)
+	}
+
+	defer func() {
+		if err != nil {
+			if cerr := removeSnapData(fullName, s.Version()); cerr != nil {
+				logger.Noticef("When cleaning up data for %s %s: %v", s.Name(), s.Version(), cerr)
+			}
+		}
+	}()
+
+	if err != nil {
+		return "", err
+	}
+
+	// and finally make active
+	err = setActiveClick(s.basedir, inhibitHooks, inter)
+	defer func() {
+		if err != nil && currentActiveDir != "" {
+			if cerr := setActiveClick(currentActiveDir, inhibitHooks, inter); cerr != nil {
+				logger.Noticef("When setting old %s version back to active: %v", s.Name(), cerr)
+			}
+		}
+	}()
+	if err != nil {
+		return "", err
+	}
+
+	// oh, one more thing: refresh the security bits
+	if !inhibitHooks {
+		deps, err := s.Dependents()
+		if err != nil {
+			return "", err
+		}
+
+		sysd := systemd.New(globalRootDir, inter)
+		stopped := make(map[string]time.Duration)
+		defer func() {
+			if err != nil {
+				for serviceName := range stopped {
+					if e := sysd.Start(serviceName); e != nil {
+						inter.Notify(fmt.Sprintf("unable to restart %s with the old %s: %s", serviceName, s.Name(), e))
+					}
+				}
+			}
+		}()
+
+		for _, dep := range deps {
+			if !dep.IsActive() {
+				continue
+			}
+			for _, svc := range dep.Services() {
+				serviceName := filepath.Base(generateServiceFileName(dep.m, svc))
+				timeout := time.Duration(svc.StopTimeout)
+				if err = sysd.Stop(serviceName, timeout); err != nil {
+					inter.Notify(fmt.Sprintf("unable to stop %s; aborting install: %s", serviceName, err))
+					return "", err
+				}
+				stopped[serviceName] = timeout
+			}
+		}
+
+		if err := s.RefreshDependentsSecurity(currentActiveDir, inter); err != nil {
+			return "", err
+		}
+
+		started := make(map[string]time.Duration)
+		defer func() {
+			if err != nil {
+				for serviceName, timeout := range started {
+					if e := sysd.Stop(serviceName, timeout); e != nil {
+						inter.Notify(fmt.Sprintf("unable to stop %s with the old %s: %s", serviceName, s.Name(), e))
+					}
+				}
+			}
+		}()
+		for serviceName, timeout := range stopped {
+			if err = sysd.Start(serviceName); err != nil {
+				inter.Notify(fmt.Sprintf("unable to restart %s; aborting install: %s", serviceName, err))
+				return "", err
+			}
+			started[serviceName] = timeout
+		}
+	}
+
+	return s.Name(), nil
 }
 
 // SetActive sets the snap active
@@ -716,6 +931,45 @@
 	return needed, nil
 }
 
+// CanInstall checks whether the SnapPart passes a series of tests required for installation
+func (s *SnapPart) CanInstall(allowOEM bool, inter interacter) error {
+	if s.IsInstalled() {
+		return ErrAlreadyInstalled
+	}
+
+	if err := s.m.checkForPackageInstalled(s.Origin()); err != nil {
+		return err
+	}
+
+	if err := s.m.checkForNameClashes(); err != nil {
+		return err
+	}
+
+	if err := s.m.checkForFrameworks(); err != nil {
+		return err
+	}
+
+	if s.Type() == pkg.TypeOem {
+		if !allowOEM {
+			if currentOEM, err := getOem(); err == nil {
+				if currentOEM.Name != s.Name() {
+					return ErrOEMPackageInstall
+				}
+			} else {
+				// there should always be an oem package now
+				return ErrOEMPackageInstall
+			}
+		}
+	}
+
+	curr, _ := filepath.EvalSymlinks(filepath.Join(s.basedir, "..", "current"))
+	if err := s.m.checkLicenseAgreement(inter, s.deb, curr); err != nil {
+		return err
+	}
+
+	return nil
+}
+
 var timestampUpdater = helpers.UpdateTimestamp
 
 func updateAppArmorJSONTimestamp(fullName, thing, version string) error {
@@ -829,6 +1083,7 @@
 	if err != nil {
 		return nil, err
 	}
+
 	for _, yamlfile := range matches {
 
 		// skip "current" and similar symlinks
@@ -840,20 +1095,8 @@
 			continue
 		}
 
-		m, err := parsePackageYamlFile(realpath)
-		if err != nil {
-			return nil, err
-		}
-
-		origin := ""
-		if m.Type != pkg.TypeFramework && m.Type != pkg.TypeOem {
-			origin, err = originFromYamlPath(realpath)
-			if err != nil {
-				return nil, err
-			}
-		}
-
-		snap, err := NewSnapPartFromYaml(realpath, origin, m)
+		origin, _ := originFromYamlPath(realpath)
+		snap, err := NewInstalledSnapPart(realpath, origin)
 		if err != nil {
 			return nil, err
 		}
@@ -873,6 +1116,7 @@
 	return ext[1:]
 }
 
+// originFromYamlPath *must* return "" if it's returning error.
 func originFromYamlPath(path string) (string, error) {
 	origin := originFromBasedir(filepath.Join(path, "..", ".."))
 

=== modified file 'snappy/snapp_test.go'
--- snappy/snapp_test.go	2015-05-20 17:24:29 +0000
+++ snappy/snapp_test.go	2015-05-28 14:48:17 +0000
@@ -145,6 +145,7 @@
 	c.Check(snap.Version(), Equals, "1.10")
 	c.Check(snap.IsActive(), Equals, false)
 	c.Check(snap.Description(), Equals, "Hello")
+	c.Check(snap.IsInstalled(), Equals, true)
 
 	services := snap.Services()
 	c.Assert(services, HasLen, 1)