Merge lp:~sergiusens/snapcraft/1481122 into lp:~snappy-dev/snapcraft/core

[Sergio Schvezov]

Plugins raise exception instead of true/false

[lp:~sergiusens/snapcraft/1481122]

252. By Sergio Schvezov on 2015-10-21

Move lifecycle exception handling to not lose data (__str__())

[Leo Arias (elopio)]

Just one needs information because I don't understand one of the changes.

I love the rest, thanks a lot!

review: Needs Information

[lp:~sergiusens/snapcraft/1481122]

253. By Sergio Schvezov on 2015-10-26

Improve the base plugin's pull logic

254. By Sergio Schvezov on 2015-10-26

Reverting a rogue change

[Leo Arias (elopio)]

land!

review: Approve

=== modified file 'snapcraft/__init__.py'
--- snapcraft/__init__.py	2015-10-19 19:00:55 +0000
+++ snapcraft/__init__.py	2015-10-26 14:06:39 +0000
@@ -106,21 +106,9 @@
         Override or inherit from this method if you need to implement or
         enhance with custom pull logic.
         """
-        if not getattr(self.options, 'source', None):
-            return True
-        try:
-            return snapcraft.sources.get(
+        if getattr(self.options, 'source', None):
+            snapcraft.sources.get(
                 self.sourcedir, self.builddir, self.options)
-        except ValueError as e:
-            logger.error('Unrecognized source %r for part %r: %s.',
-                         self.options.source, self.name, e)
-            snapcraft.common.fatal()
-        except snapcraft.sources.IncompatibleOptionsError as e:
-            logger.error(
-                'Issues while setting up sources for part \'%s\': %s.',
-                self.name,
-                e.message)
-            snapcraft.common.fatal()
 
     def build(self):
         """Build the source code retrieved from the pull phase.
@@ -128,7 +116,7 @@
         The base implementation does nothing by default. Override this method
         if you need to process the source code to make it runnable.
         """
-        return True
+        pass
 
     def snap_fileset(self):
         """Return a list of files to include or exclude in the resulting snap

=== modified file 'snapcraft/cmds.py'
--- snapcraft/cmds.py	2015-10-20 13:49:11 +0000
+++ snapcraft/cmds.py	2015-10-26 14:06:39 +0000
@@ -26,9 +26,9 @@
 import tempfile
 import time
 
-import snapcraft.lifecycle
 import snapcraft.yaml
 from snapcraft import common
+from snapcraft import lifecycle
 from snapcraft import meta
 
 logger = logging.getLogger(__name__)
@@ -55,7 +55,7 @@
     if args.part:
         yaml += 'parts:\n'
     for part_name in args.part:
-        part = snapcraft.lifecycle.load_plugin(part_name, part_name)
+        part = lifecycle.load_plugin(part_name, part_name)
         yaml += '    ' + part.name + ':\n'
         for opt in part.config.get('options', []):
             if part.config['options'][opt].get('required', False):
@@ -264,7 +264,7 @@
     for part in parts:
         # Gather our own files up
         fileset = getattr(part.code.options, 'stage', ['*']) or ['*']
-        part_files, _ = snapcraft.lifecycle.migratable_filesets(
+        part_files, _ = lifecycle.migratable_filesets(
             fileset,
             part.installdir)
 
@@ -335,8 +335,10 @@
             common.env = config.build_env_for_part(part)
             force = forceAll or cmd == forceCommand
 
-            if not getattr(part, cmd)(force=force):
-                logger.error('Failed doing %s for %s!', cmd, part.name)
+            try:
+                getattr(part, cmd)(force=force)
+            except Exception as e:
+                logger.error('Failed doing %s for %s: %s', cmd, part.name, e)
                 sys.exit(1)
 
 
@@ -393,3 +395,5 @@
         logger.error('Issue detected while analyzing '
                      'snapcraft.yaml: {}'.format(e.message))
         sys.exit(1)
+    except lifecycle.PluginError as e:
+        logger.error('Issue while loading plugin: {}'.format(e))

=== modified file 'snapcraft/common.py'
--- snapcraft/common.py	2015-10-05 18:24:04 +0000
+++ snapcraft/common.py	2015-10-26 14:06:39 +0000
@@ -46,7 +46,7 @@
         f.write('\n')
         f.write('exec $*')
         f.flush()
-        return subprocess.call(['/bin/sh', f.name] + cmd, **kwargs) == 0
+        subprocess.check_call(['/bin/sh', f.name] + cmd, **kwargs)
 
 
 def run_output(cmd, **kwargs):

=== modified file 'snapcraft/lifecycle.py'
--- snapcraft/lifecycle.py	2015-10-19 20:42:36 +0000
+++ snapcraft/lifecycle.py	2015-10-26 14:06:39 +0000
@@ -25,7 +25,6 @@
 
 import snapcraft
 from snapcraft import common
-from snapcraft import repo
 
 
 logger = logging.getLogger(__name__)
@@ -64,15 +63,9 @@
 
         try:
             self._load_code(plugin_name, properties)
-            # only set to valid if it loads without PluginError
-            self.valid = True
-        except PluginError as e:
-            logger.error(str(e))
-            return
         except jsonschema.ValidationError as e:
-            logger.error('Issues while loading properties for '
-                         '{}: {}'.format(part_name, e.message))
-            return
+            raise PluginError('properties failed to load for {}: {}'.format(
+                part_name, e.message))
 
     def _load_code(self, plugin_name, properties):
         module_name = plugin_name.replace('-', '_')
@@ -92,7 +85,7 @@
             with contextlib.suppress(ImportError):
                 module = _load_local(module_name)
             if not module:
-                raise PluginError('Unknown plugin: {}'.format(plugin_name))
+                raise PluginError('unknown plugin: {}'.format(plugin_name))
 
         plugin = _get_plugin(module)
         options = _make_options(properties, plugin.schema())
@@ -112,9 +105,6 @@
         for d in dirs:
             os.makedirs(d, exist_ok=True)
 
-    def is_valid(self):
-        return self.valid
-
     def notify_stage(self, stage, hint=''):
         logger.info('%s %s %s', stage, self.name, hint)
 
@@ -146,33 +136,20 @@
 
     def pull(self, force=False):
         if not self.should_stage_run('pull', force):
-            return True
+            return
         self.makedirs()
-
         self.notify_stage("Pulling")
-
-        try:
-            self._setup_stage_packages()
-        except repo.PackageNotFoundError as e:
-            logger.error(e.message)
-            return False
-
-        if not self.code.pull():
-            return False
-
+        self._setup_stage_packages()
+        self.code.pull()
         self.mark_done('pull')
-        return True
 
     def build(self, force=False):
         if not self.should_stage_run('build', force):
-            return True
+            return
         self.makedirs()
         self.notify_stage("Building")
-        if not self.code.build():
-            return False
-
+        self.code.build()
         self.mark_done('build')
-        return True
 
     def _migratable_fileset_for(self, stage):
         plugin_fileset = self.code.snap_fileset()
@@ -294,11 +271,7 @@
 
 
 def load_plugin(part_name, plugin_name, properties={}):
-    part = PluginHandler(plugin_name, part_name, properties)
-    if not part.is_valid():
-        logger.error('Could not load part %s', plugin_name)
-        sys.exit(1)
-    return part
+    return PluginHandler(plugin_name, part_name, properties)
 
 
 def migratable_filesets(fileset, srcdir):

=== modified file 'snapcraft/meta.py'
--- snapcraft/meta.py	2015-10-14 21:42:35 +0000
+++ snapcraft/meta.py	2015-10-26 14:06:39 +0000
@@ -216,11 +216,8 @@
                       'which "{}"\n'.format(relexepath))
             tempf.write(script)
             tempf.flush()
-            if common.run(['/bin/sh', tempf.name], cwd=snap_dir):
-                wrapexec = relexepath
-            else:
-                logger.warning('Warning: unable to find "{}" in the path'.
-                               format(relexepath))
+            common.run(['/bin/sh', tempf.name], cwd=snap_dir)
+            wrapexec = relexepath
 
     _write_wrap_exe(wrapexec, wrappath)
 

=== modified file 'snapcraft/plugins/ant.py'
--- snapcraft/plugins/ant.py	2015-10-17 02:25:13 +0000
+++ snapcraft/plugins/ant.py	2015-10-26 14:06:39 +0000
@@ -17,7 +17,6 @@
 import glob
 import logging
 import os
-import sys
 
 import snapcraft
 import snapcraft.common
@@ -35,16 +34,13 @@
 
     def build(self):
         super().build()
-        if not self.run(['ant']):
-            return False
+        self.run(['ant'])
         files = glob.glob(os.path.join(self.builddir, 'target', '*.jar'))
         if not files:
-            logger.error('Could not find any built jar files for part %s',
-                         self.name)
-            sys.exit(1)
+            raise RuntimeError('could not find any built jar files for part')
         jardir = os.path.join(self.installdir, 'jar')
-        return self.run(['mkdir', '-p', jardir]) and \
-            self.run(['cp', '-a'] + files + [jardir])
+        self.run(['mkdir', '-p', jardir])
+        self.run(['cp', '-a'] + files + [jardir])
 
     def env(self, root):
         env = super().env(root)

=== modified file 'snapcraft/plugins/autotools.py'
--- snapcraft/plugins/autotools.py	2015-10-19 16:00:59 +0000
+++ snapcraft/plugins/autotools.py	2015-10-26 14:06:39 +0000
@@ -47,11 +47,10 @@
     def build(self):
         if not os.path.exists(os.path.join(self.builddir, "configure")):
             if os.path.exists(os.path.join(self.builddir, "autogen.sh")):
-                if not self.run(['env', 'NOCONFIGURE=1', './autogen.sh'],
-                                cwd=self.builddir):
-                    return False
+                self.run(['env', 'NOCONFIGURE=1', './autogen.sh'],
+                         cwd=self.builddir)
             else:
-                if not self.run(['autoreconf', '-i'], cwd=self.builddir):
-                    return False
-        return self.run(['./configure', '--prefix='] +
-                        self.options.configflags) and super().build()
+                self.run(['autoreconf', '-i'], cwd=self.builddir)
+        self.run(['./configure', '--prefix='] +
+                 self.options.configflags)
+        super().build()

=== modified file 'snapcraft/plugins/catkin.py'
--- snapcraft/plugins/catkin.py	2015-10-17 02:25:13 +0000
+++ snapcraft/plugins/catkin.py	2015-10-26 14:06:39 +0000
@@ -65,16 +65,8 @@
         self._deb_packages = []
 
     def pull(self):
-        if not super().pull():
-            return False
-
-        try:
-            self._setup_deb_packages()
-        except snapcraft.repo.PackageNotFoundError as e:
-            logger.error(e.message)
-            return False
-
-        return True
+        super().pull()
+        self._setup_deb_packages()
 
     def env(self, root):
         return [
@@ -186,41 +178,34 @@
             f.write('exec {}\n'.format(' '.join(commandlist)))
             f.flush()
 
-            return self.run(['/bin/bash', f.name], cwd=cwd)
+            self.run(['/bin/bash', f.name], cwd=cwd)
 
     def build(self):
         # Fixup ROS Cmake files that have hardcoded paths in them
-        if not self.run([
+        self.run([
             'find', self.rosdir, '-name', '*.cmake',
             '-exec', 'sed', '-i', '-e',
             r's|\(\W\)/usr/lib/|\1{0}/usr/lib/|g'.format(self.installdir),
             '{}', ';'
-        ]):
-            return False
+        ])
 
         self._find_package_deps()
-
-        if not self._build_packages_deps():
-            return False
+        self._build_packages_deps()
 
         # the hacks
         findcmd = ['find', self.installdir, '-name', '*.cmake', '-delete']
-        if not self.run(findcmd):
-            return False
+        self.run(findcmd)
 
-        if not self.run(
+        self.run(
             ['rm', '-f',
              'opt/ros/' + self.options.rosversion + '/.catkin',
              'opt/ros/' + self.options.rosversion + '/.rosinstall',
              'opt/ros/' + self.options.rosversion + '/setup.sh',
              'opt/ros/' + self.options.rosversion + '/_setup_util.py'],
-                cwd=self.installdir):
-            return False
+            cwd=self.installdir)
 
         os.remove(os.path.join(self.installdir, 'usr/bin/xml2-config'))
 
-        return True
-
     def _build_packages_deps(self):
         # Ugly dependency resolution, just loop through until we can
         # find something to build. When we do, build it. Loop until we
@@ -233,17 +218,13 @@
             for pkg in self.packages - built:
                 if len(self.package_local_deps[pkg] - built) > 0:
                     continue
-
-                if not self._handle_package(pkg):
-                    return False
+                self._handle_package(pkg)
 
                 built.add(pkg)
                 built_pkg = True
 
         if not built_pkg:
-            return False
-
-        return True
+            raise RuntimeError('some packages failed to build')
 
     def _handle_package(self, pkg):
         catkincmd = ['catkin_make_isolated']
@@ -278,7 +259,4 @@
                 os.path.join(self.installdir, 'usr', 'bin', 'g++'))
         ])
 
-        if not self._rosrun(catkincmd):
-            return False
-
-        return True
+        self._rosrun(catkincmd)

=== modified file 'snapcraft/plugins/cmake.py'
--- snapcraft/plugins/cmake.py	2015-10-09 17:43:20 +0000
+++ snapcraft/plugins/cmake.py	2015-10-26 14:06:39 +0000
@@ -39,5 +39,6 @@
         self.build_packages.append('cmake')
 
     def build(self):
-        return self.run(['cmake', '.', '-DCMAKE_INSTALL_PREFIX='] +
-                        self.options.configflags) and super().build()
+        self.run(['cmake', '.', '-DCMAKE_INSTALL_PREFIX='] +
+                 self.options.configflags)
+        super().build()

=== modified file 'snapcraft/plugins/copy.py'
--- snapcraft/plugins/copy.py	2015-10-09 17:43:20 +0000
+++ snapcraft/plugins/copy.py	2015-10-26 14:06:39 +0000
@@ -39,17 +39,11 @@
         }
 
     def build(self):
-        res = True
         for src in sorted(self.options.files):
             dst = self.options.files[src]
             if not os.path.lexists(src):
-                logger.warning("WARNING: file '%s' missing", src)
-                res = False
-                continue
+                raise EnvironmentError('file "{}" missing'.format(src))
             dst = os.path.join(self.installdir, dst)
-            dst_dir = os.path.dirname(dst)
-            if not os.path.exists(dst_dir):
-                os.makedirs(dst_dir)
-            res &= self.run(["cp", "--preserve=all", "-R", src, dst],
-                            cwd=os.getcwd())
-        return res
+            os.makedirs(os.path.dirname(dst), exist_ok=True)
+            self.run(["cp", "--preserve=all", "-R", src, dst],
+                     cwd=os.getcwd())

=== modified file 'snapcraft/plugins/go.py'
--- snapcraft/plugins/go.py	2015-10-09 17:43:20 +0000
+++ snapcraft/plugins/go.py	2015-10-26 14:06:39 +0000
@@ -51,16 +51,14 @@
     def pull(self):
         # use -d to only download (build will happen later)
         # use -t to also get the test-deps
-        return self.run(['go', 'get', '-t', '-d', self.fullname])
+        self._run(['go', 'get', '-t', '-d', self.fullname])
 
     def build(self):
-        if not self.run(['go', 'build', self.fullname]):
-            return False
-        if not self.run(['go', 'install', self.fullname]):
-            return False
-        return self.run(['cp', '-a', os.path.join(self.builddir, 'bin'),
-                        self.installdir])
+        self._run(['go', 'build', self.fullname])
+        self._run(['go', 'install', self.fullname])
+        self._run(['cp', '-a', os.path.join(self.builddir, 'bin'),
+                  self.installdir])
 
-    def run(self, cmd, **kwargs):
+    def _run(self, cmd, **kwargs):
         cmd = ['env', 'GOPATH=' + self.builddir] + cmd
-        return super().run(cmd, **kwargs)
+        return self.run(cmd, **kwargs)

=== modified file 'snapcraft/plugins/make.py'
--- snapcraft/plugins/make.py	2015-10-17 02:25:13 +0000
+++ snapcraft/plugins/make.py	2015-10-26 14:06:39 +0000
@@ -24,5 +24,5 @@
         self.build_packages.append('make')
 
     def build(self):
-        return self.run(['make']) and \
-            self.run(['make', 'install', 'DESTDIR=' + self.installdir])
+        self.run(['make'])
+        self.run(['make', 'install', 'DESTDIR=' + self.installdir])

=== modified file 'snapcraft/plugins/maven.py'
--- snapcraft/plugins/maven.py	2015-10-19 23:44:27 +0000
+++ snapcraft/plugins/maven.py	2015-10-26 14:06:39 +0000
@@ -50,22 +50,16 @@
     def build(self):
         super().build()
 
-        if not self.run(['mvn', 'package'] + self.options.maven_options):
-            return False
+        self.run(['mvn', 'package'] + self.options.maven_options)
         jarfiles = glob.glob(os.path.join(self.builddir, 'target', '*.jar'))
         warfiles = glob.glob(os.path.join(self.builddir, 'target', '*.war'))
         if not (jarfiles or warfiles):
-            logger.error('Could not find any built jar or war files for '
-                         'part %s', self.name)
-            snapcraft.common.fatal()
+            raise RuntimeError('could not find any built jar files for part')
         if jarfiles:
             jardir = os.path.join(self.installdir, 'jar')
             os.makedirs(jardir, exist_ok=True)
-            if not self.run(['cp', '-a'] + jarfiles + [jardir]):
-                return False
+            self.run(['cp', '-a'] + jarfiles + [jardir])
         if warfiles:
             wardir = os.path.join(self.installdir, 'war')
             os.makedirs(wardir, exist_ok=True)
-            if not self.run(['cp', '-a'] + warfiles + [wardir]):
-                return False
-        return True
+            self.run(['cp', '-a'] + warfiles + [wardir])

=== modified file 'snapcraft/plugins/python2.py'
--- snapcraft/plugins/python2.py	2015-10-17 02:25:13 +0000
+++ snapcraft/plugins/python2.py	2015-10-26 14:06:39 +0000
@@ -45,10 +45,8 @@
             root, 'usr', 'lib', self.python_version, 'dist-packages')]
 
     def pull(self):
-        if not super().pull():
-            return False
-
-        return self._pip()
+        super().pull()
+        self._pip()
 
     def _pip(self):
         setup = 'setup.py'
@@ -59,7 +57,7 @@
             requirements = os.path.join(os.getcwd(), self.options.requirements)
 
         if not os.path.exists(setup) and not self.options.requirements:
-            return True
+            return
 
         easy_install = os.path.join(
             self.installdir, 'usr', 'bin', 'easy_install')
@@ -73,21 +71,17 @@
                              'dist-packages'),
                 site_packages_dir)
 
-        if not self.run(['python2', easy_install, '--prefix', prefix, 'pip']):
-            return False
+        self.run(['python2', easy_install, '--prefix', prefix, 'pip'])
 
         pip2 = os.path.join(self.installdir, 'usr', 'bin', 'pip2')
         pip_install = ['python2', pip2, 'install', '--target',
                        site_packages_dir]
 
-        if self.options.requirements and not self.run(
-                pip_install + ['--requirement', requirements]):
-            return False
-
-        if os.path.exists(setup) and not self.run(pip_install + ['.', ]):
-            return False
-
-        return True
+        if self.options.requirements:
+            self.run(pip_install + ['--requirement', requirements])
+
+        if os.path.exists(setup):
+            self.run(pip_install + ['.', ])
 
     def build(self):
         # If setuptools is used, it tries to create files in the
@@ -96,11 +90,11 @@
         # used.
 
         if not os.path.exists(os.path.join(self.builddir, 'setup.py')):
-            return True
+            return
 
         os.makedirs(self.dist_packages_dir, exist_ok=True)
         setuptemp = self.copy_setup()
-        return self.run(
+        self.run(
             ['python2', setuptemp.name, 'install', '--install-layout=deb',
              '--prefix={}/usr'.format(self.installdir)], cwd=self.builddir)
 

=== modified file 'snapcraft/plugins/python3.py'
--- snapcraft/plugins/python3.py	2015-10-17 02:25:13 +0000
+++ snapcraft/plugins/python3.py	2015-10-26 14:06:39 +0000
@@ -45,10 +45,8 @@
             root, 'usr', 'lib', self.python_version, 'dist-packages')]
 
     def pull(self):
-        if not super().pull():
-            return False
-
-        return self._pip()
+        super().pull()
+        self._pip()
 
     def _pip(self):
         setup = 'setup.py'
@@ -59,7 +57,7 @@
             requirements = os.path.join(os.getcwd(), self.options.requirements)
 
         if not os.path.exists(setup) and not self.options.requirements:
-            return True
+            return
 
         easy_install = os.path.join(
             self.installdir, 'usr', 'bin', 'easy_install3')
@@ -72,21 +70,17 @@
                 os.path.join(prefix, 'lib', 'python3', 'dist-packages'),
                 site_packages_dir)
 
-        if not self.run(['python3', easy_install, '--prefix', prefix, 'pip']):
-            return False
+        self.run(['python3', easy_install, '--prefix', prefix, 'pip'])
 
         pip3 = os.path.join(self.installdir, 'usr', 'bin', 'pip3')
         pip_install = ['python3', pip3, 'install', '--target',
                        site_packages_dir]
 
-        if self.options.requirements and not self.run(
-                pip_install + ['--requirement', requirements]):
-            return False
-
-        if os.path.exists(setup) and not self.run(pip_install + ['.', ]):
-            return False
-
-        return True
+        if self.options.requirements:
+            self.run(pip_install + ['--requirement', requirements])
+
+        if os.path.exists(setup):
+            self.run(pip_install + ['.', ])
 
     def build(self):
         # If setuptools is used, it tries to create files in the
@@ -95,11 +89,11 @@
         # used.
 
         if not os.path.exists(os.path.join(self.builddir, 'setup.py')):
-            return True
+            return
 
         os.makedirs(self.dist_packages_dir, exist_ok=True)
         setuptemp = self.copy_setup()
-        return self.run(
+        self.run(
             ['python3', setuptemp.name, 'install', '--install-layout=deb',
              '--prefix={}/usr'.format(self.installdir)], cwd=self.builddir)
 

=== modified file 'snapcraft/plugins/qml.py'
--- snapcraft/plugins/qml.py	2015-10-09 17:43:20 +0000
+++ snapcraft/plugins/qml.py	2015-10-26 14:06:39 +0000
@@ -68,7 +68,7 @@
                 'etc/xdg/qtchooser/snappy-qt5.conf',
                 ]
 
-    def build_qt_config(self):
+    def _build_qt_config(self):
         arch = snapcraft.common.get_arch_triplet()
         configdir = os.path.join(self.installdir, 'etc', 'xdg', 'qtchooser')
         os.makedirs(configdir, exist_ok=True)
@@ -76,10 +76,9 @@
         config.write('./usr/lib/{}/qt5/bin\n'.format(arch))
         config.write('./usr/lib/{}\n'.format(arch))
         config.close
-        return True
 
     def build(self):
-        return self.build_qt_config()
+        self._build_qt_config()
 
     def env(self, root):
         arch = snapcraft.common.get_arch_triplet()

=== modified file 'snapcraft/plugins/roscore.py'
--- snapcraft/plugins/roscore.py	2015-10-16 19:05:42 +0000
+++ snapcraft/plugins/roscore.py	2015-10-26 14:06:39 +0000
@@ -66,7 +66,6 @@
             f.write('exec {}/bin/rosmaster\n'.format(ros_dir))
 
         os.chmod(service_wrapper, 0o755)
-        return True
 
     def snap_fileset(self):
         rospath = os.path.join('opt', 'ros', self.options.rosversion)

=== modified file 'snapcraft/plugins/scons.py'
--- snapcraft/plugins/scons.py	2015-10-17 02:25:13 +0000
+++ snapcraft/plugins/scons.py	2015-10-26 14:06:39 +0000
@@ -43,6 +43,5 @@
     def build(self):
         env = os.environ.copy()
         env['DESTDIR'] = self.installdir
-        return (self.run(['scons', ] + self.options.scons_options) and
-                self.run(['scons', 'install'] +
-                         self.options.scons_options, env=env))
+        self.run(['scons', ] + self.options.scons_options)
+        self.run(['scons', 'install'] + self.options.scons_options, env=env)

=== modified file 'snapcraft/plugins/tar_content.py'
--- snapcraft/plugins/tar_content.py	2015-10-09 17:43:20 +0000
+++ snapcraft/plugins/tar_content.py	2015-10-26 14:06:39 +0000
@@ -38,7 +38,7 @@
         self.tar = snapcraft.sources.Tar(self.options.source, self.builddir)
 
     def pull(self):
-        return self.tar.pull()
+        self.tar.pull()
 
     def build(self):
-        return self.tar.provision(self.installdir)
+        self.tar.provision(self.installdir)

=== modified file 'snapcraft/sources.py'
--- snapcraft/sources.py	2015-10-19 18:39:31 +0000
+++ snapcraft/sources.py	2015-10-26 14:06:39 +0000
@@ -73,7 +73,7 @@
             cmd = ['bzr', 'branch'] + tag_opts + \
                   [self.source, self.source_dir]
 
-        return snapcraft.common.run(cmd, cwd=os.getcwd())
+        snapcraft.common.run(cmd, cwd=os.getcwd())
 
 
 class Git(Base):
@@ -102,7 +102,7 @@
             cmd = ['git', 'clone'] + branch_opts + \
                   [self.source, self.source_dir]
 
-        return snapcraft.common.run(cmd, cwd=os.getcwd())
+        snapcraft.common.run(cmd, cwd=os.getcwd())
 
 
 class Mercurial(Base):
@@ -129,7 +129,7 @@
                 ref = ['-u', self.source_tag or self.source_branch]
             cmd = ['hg', 'clone'] + ref + [self.source, self.source_dir]
 
-        return snapcraft.common.run(cmd, cwd=os.getcwd())
+        snapcraft.common.run(cmd, cwd=os.getcwd())
 
 
 class Tar(Base):
@@ -146,19 +146,18 @@
 
     def pull(self):
         if not snapcraft.common.isurl(self.source):
-            return True
+            return
 
         req = requests.get(self.source, stream=True, allow_redirects=True)
         if req.status_code is not 200:
-            return False
+            raise EnvironmentError('unexpected http status code when '
+                                   'downloading %r'.format(req.status_code))
 
         file = os.path.join(self.source_dir, os.path.basename(self.source))
         with open(file, 'wb') as f:
             for chunk in req.iter_content(1024):
                 f.write(chunk)
 
-        return True
-
     def provision(self, dst, clean_target=True):
         # TODO add unit tests.
         if snapcraft.common.isurl(self.source):
@@ -172,7 +171,7 @@
             shutil.rmtree(dst)
             os.makedirs(dst)
 
-        return self._extract(tarball, dst)
+        self._extract(tarball, dst)
 
     def _extract(self, tarball, dst):
         with tarfile.open(tarball) as tar:
@@ -208,13 +207,11 @@
 
             tar.extractall(members=filter_members(tar), path=dst)
 
-        return True
-
 
 class Local(Base):
 
     def pull(self):
-        return True
+        pass
 
     def provision(self, dst):
         path = os.path.abspath(self.source)
@@ -226,8 +223,6 @@
             os.remove(dst)
         os.symlink(path, dst)
 
-        return True
-
 
 def get(sourcedir, builddir, options):
     source_type = getattr(options, 'source_type', None)
@@ -237,9 +232,8 @@
     handler_class = _get_source_handler(source_type, options.source)
     handler = handler_class(options.source, sourcedir, source_tag,
                             source_branch)
-    if not handler.pull():
-        return False
-    return handler.provision(builddir)
+    handler.pull()
+    handler.provision(builddir)
 
 
 _source_handler = {
@@ -270,8 +264,8 @@
     elif _tar_type_regex.match(source):
         source_type = 'tar'
     elif snapcraft.common.isurl(source):
-        raise ValueError('No handler to manage source')
+        raise ValueError('no handler to manage source')
     elif not os.path.isdir(source):
-        raise ValueError('Local source is not a directory')
+        raise ValueError('local source is not a directory')
 
     return source_type

=== modified file 'snapcraft/tests/test_base_plugin.py'
--- snapcraft/tests/test_base_plugin.py	2015-10-17 02:25:13 +0000
+++ snapcraft/tests/test_base_plugin.py	2015-10-26 14:06:39 +0000
@@ -14,11 +14,10 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-import fixtures
-import logging
 import unittest.mock
 
 import snapcraft
+from snapcraft import sources
 from snapcraft import tests
 
 
@@ -34,38 +33,27 @@
 
 class TestBasePlugin(tests.TestCase):
 
-    def test_get_source_with_unrecognized_source_must_raise_error(self):
-        fake_logger = fixtures.FakeLogger(level=logging.ERROR)
-        self.useFixture(fake_logger)
-
+    def test_get_source_with_unrecognized_source_must_raise_exception(self):
         options = MockOptions('unrecognized://test_source')
         plugin = snapcraft.BasePlugin('test_plugin', options)
-        with self.assertRaises(SystemExit) as raised:
+        with self.assertRaises(ValueError) as raised:
             plugin.pull()
 
-        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
-        expected = (
-            "Unrecognized source 'unrecognized://test_source' for part "
-            "'test_plugin': No handler to manage source.\n")
-        self.assertEqual(expected, fake_logger.output)
+        self.assertEqual(raised.exception.__str__(),
+                         'no handler to manage source')
 
     @unittest.mock.patch('os.path.isdir')
-    def test_local_non_dir_source_path_must_raise_error(self, mock_isdir):
-        fake_logger = fixtures.FakeLogger(level=logging.ERROR)
-        self.useFixture(fake_logger)
-
+    def test_local_non_dir_source_path_must_raise_exception(self, mock_isdir):
         options = MockOptions('file')
         mock_isdir.return_value = False
         plugin = snapcraft.BasePlugin('test_plugin', options)
-        with self.assertRaises(SystemExit) as raised:
+        with self.assertRaises(ValueError) as raised:
             plugin.pull()
 
         mock_isdir.assert_called_once_with('file')
-        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
-        expected = (
-            "Unrecognized source 'file' for part 'test_plugin': "
-            "Local source is not a directory.\n")
-        self.assertEqual(expected, fake_logger.output)
+
+        self.assertEqual(raised.exception.__str__(),
+                         'local source is not a directory')
 
 
 class GetSourceWithBranches(tests.TestCase):
@@ -84,21 +72,16 @@
     ]
 
     def test_get_source_with_branch_and_tag_must_raise_error(self):
-        fake_logger = fixtures.FakeLogger(level=logging.ERROR)
-        self.useFixture(fake_logger)
-
         options = MockOptions('lp:source', self.source_type,
                               self.source_branch, self.source_tag)
         plugin = snapcraft.BasePlugin('test_plugin', options)
-        with self.assertRaises(SystemExit) as raised:
+        with self.assertRaises(sources.IncompatibleOptionsError) as raised:
             plugin.pull()
 
-        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
-        expected = (
-            'Issues while setting up sources for part \'test_plugin\': '
+        self.assertEqual(
+            raised.exception.__str__(),
             'can\'t specify both source-tag and source-branch for a {} '
-            "source.\n".format(self.source_type))
-        self.assertEqual(expected, fake_logger.output)
+            'source'.format(self.source_type))
 
 
 class GetSourceTestCase(tests.TestCase):
@@ -122,19 +105,14 @@
     ]
 
     def test_get_source_with_branch_must_raise_error(self):
-        fake_logger = fixtures.FakeLogger(level=logging.ERROR)
-        self.useFixture(fake_logger)
-
         options = MockOptions('lp:this', self.source_type, self.source_branch,
                               self.source_tag)
         plugin = snapcraft.BasePlugin('test_plugin', options)
 
-        with self.assertRaises(SystemExit) as raised:
+        with self.assertRaises(sources.IncompatibleOptionsError) as raised:
             plugin.pull()
 
-        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
-        expected = (
-            'Issues while setting up sources for part \'test_plugin\': can\'t '
-            'specify a {} for a {} source.\n'
-            .format(self.error, self.source_type))
-        self.assertEqual(expected, fake_logger.output)
+        self.assertEqual(
+            raised.exception.__str__(),
+            'can\'t specify a {} for a {} source'.format(
+                self.error, self.source_type))

=== modified file 'snapcraft/tests/test_copy_plugin.py'
--- snapcraft/tests/test_copy_plugin.py	2015-10-02 09:13:00 +0000
+++ snapcraft/tests/test_copy_plugin.py	2015-10-26 14:06:39 +0000
@@ -14,7 +14,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-import logging
 import os.path
 from unittest.mock import Mock
 
@@ -34,7 +33,7 @@
         self.dst_prefix = "parts/copy/install/"
         os.makedirs(self.dst_prefix)
 
-    def test_copy_plugin_any_missing_src_warns_and_errors(self):
+    def test_copy_plugin_any_missing_src_raises_exception(self):
         # ensure that a bad file causes a warning and fails the build even
         # if there is a good file last
         self.mock_options.files = {
@@ -44,12 +43,11 @@
         open("zzz", "w").close()
         c = CopyPlugin("copy", self.mock_options)
 
-        fake_logger = fixtures.FakeLogger(level=logging.WARNING)
-        self.useFixture(fake_logger)
+        with self.assertRaises(EnvironmentError) as raised:
+            c.build()
 
-        res = c.build()
-        self.assertFalse(res)
-        self.assertEqual("WARNING: file 'src' missing\n", fake_logger.output)
+        self.assertEqual(raised.exception.__str__(),
+                         'file "src" missing')
 
     def test_copy_plugin_copies(self):
         self.useFixture(fixtures.FakeLogger())
@@ -60,7 +58,7 @@
         open("src", "w").close()
 
         c = CopyPlugin("copy", self.mock_options)
-        self.assertTrue(c.build())
+        c.build()
         self.assertTrue(os.path.exists(os.path.join(self.dst_prefix, "dst")))
 
     def test_copy_plugin_creates_prefixes(self):
@@ -72,6 +70,6 @@
         open("src", "w").close()
 
         c = CopyPlugin("copy", self.mock_options)
-        self.assertTrue(c.build())
+        c.build()
         self.assertTrue(os.path.exists(os.path.join(self.dst_prefix,
                                                     "dir/dst")))

=== modified file 'snapcraft/tests/test_lifecycle.py'
--- snapcraft/tests/test_lifecycle.py	2015-10-19 20:42:36 +0000
+++ snapcraft/tests/test_lifecycle.py	2015-10-26 14:06:39 +0000
@@ -28,36 +28,23 @@
 import snapcraft.tests
 
 
-def get_test_plugin(name='mock', part_name='mock-part', properties=None):
+def get_test_plugin(name='copy', part_name='mock-part', properties=None):
     if properties is None:
-        properties = {}
+        properties = {'files': {'1': '1'}}
     return snapcraft.lifecycle.PluginHandler(name, part_name, properties)
 
 
 class PluginTestCase(snapcraft.tests.TestCase):
 
-    def test_init_unknown_plugin_must_log_error(self):
+    def test_init_unknown_plugin_must_raise_exception(self):
         fake_logger = fixtures.FakeLogger(level=logging.ERROR)
         self.useFixture(fake_logger)
 
-        get_test_plugin('test_unexisting_name')
-
-        self.assertEqual(
-            'Unknown plugin: test_unexisting_name\n', fake_logger.output)
-
-    def test_is_dirty(self):
-        p = get_test_plugin()
-        p.statefile = tempfile.NamedTemporaryFile().name
-        self.addCleanup(os.remove, p.statefile)
-        p.code = Mock()
-        p._setup_stage_packages = Mock()
-        # pull once
-        p.pull()
-        p.code.pull.assert_called()
-        # pull again, not dirty no need to pull
-        p.code.pull.reset_mock()
-        p.pull()
-        self.assertFalse(p.code.pull.called)
+        with self.assertRaises(snapcraft.lifecycle.PluginError) as raised:
+            get_test_plugin('test_unexisting_name')
+
+        self.assertEqual(raised.exception.__str__(),
+                         'unknown plugin: test_unexisting_name')
 
     def test_fileset_include_excludes(self):
         stage_set = [
@@ -224,20 +211,6 @@
         self.assertEqual(
             "path '/abs/exclude' must be relative", str(raised.exception))
 
-    def test_load_plugin_with_invalid_part_must_exit_with_error(self):
-        fake_logger = fixtures.FakeLogger(level=logging.ERROR)
-        self.useFixture(fake_logger)
-
-        with self.assertRaises(SystemExit) as raised:
-            snapcraft.lifecycle.load_plugin(
-                'dummy-part', 'test_unexisting_name')
-
-        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
-        self.assertEqual(
-            'Unknown plugin: test_unexisting_name\n'
-            'Could not load part test_unexisting_name\n',
-            fake_logger.output)
-
 
 class PluginMakedirsTestCase(snapcraft.tests.TestCase):
 

=== modified file 'snapcraft/tests/test_meta.py'
--- snapcraft/tests/test_meta.py	2015-10-02 09:13:00 +0000
+++ snapcraft/tests/test_meta.py	2015-10-26 14:06:39 +0000
@@ -334,7 +334,11 @@
     def test_wrap_exe_must_write_wrapper(self):
         snapdir = common.get_snapdir()
         os.mkdir(snapdir)
+
         relative_exe_path = 'test_relexepath'
+        with open(os.path.join(snapdir, relative_exe_path), 'w'):
+            pass
+
         relative_wrapper_path = meta._wrap_exe(relative_exe_path)
         wrapper_path = os.path.join(snapdir, relative_wrapper_path)
 

=== modified file 'snapcraft/tests/test_sources.py'
--- snapcraft/tests/test_sources.py	2015-10-17 02:25:13 +0000
+++ snapcraft/tests/test_sources.py	2015-10-26 14:06:39 +0000
@@ -59,7 +59,7 @@
             *server.server_address, file_name=tar_file_name)
         tar_source = snapcraft.sources.Tar(source, dest_dir)
 
-        self.assertTrue(tar_source.pull())
+        tar_source.pull()
 
         with open(os.path.join(dest_dir, tar_file_name), 'r') as tar_file:
             self.assertEqual('Test fake tarball file', tar_file.read())
@@ -314,7 +314,7 @@
 
     def test_pull(self):
         local = snapcraft.sources.Local('.', 'source_dir')
-        self.assertTrue(local.pull())
+        local.pull()
 
     def test_provision(self):
         local = snapcraft.sources.Local('.', 'source_dir')