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

[Sergio Schvezov]

Moving source management complexity out of the base plugin

Mostly fixing the 22 in the mccabe output 163:1: 'BasePlugin.get_source' 22 and decoupling source management from the plugin

[Leo Arias (elopio)]

this mccabe check is helping a lot, nice work Sergio.

In cases like this:
363 + def __init__(self, source, source_type=None, source_tag=None, source_branch=None):
364 + super().__init__(source, source_type, source_tag, source_branch)
365 + if source_tag and source_branch:
366 + raise IncompatibleOptionsError('can\'t specify both source-tag and source-branch for a mercurial source')

If you do the argument validation before the call to super.__init__, you will safe a few instructions.
Good to have, but no blocker. +1.

review: Approve

[Michael Vogt (mvo)]

This is really nice work, thanks for doing this! Its a great branch that reduces the complexity and makes everything more modular and easier to read. I put some comments inline but no blockers and not really important, just my 0.02¢ :)

review: Approve

[Snappy Tarmac (snappydevtarmac)]

No proposals found for merge of https://code.launchpad.net/~sergiusens/snapcraft/less-complex-collect into https://code.launchpad.net/~snappy-dev/snapcraft/core.

[Sergio Schvezov (sergiusens)]

Re approving the minor review request (code comment)

review: Approve

[Snappy Tarmac (snappydevtarmac)]

No proposals found for merge of https://code.launchpad.net/~sergiusens/snapcraft/less-complex-collect into https://code.launchpad.net/~snappy-dev/snapcraft/core.

Unmerged revisions

=== modified file 'snapcraft/__init__.py'
--- snapcraft/__init__.py	2015-08-26 08:52:09 +0000
+++ snapcraft/__init__.py	2015-09-01 10:10:15 +0000
@@ -16,11 +16,9 @@
 
 import logging
 import os
-import re
-import tarfile
-import urllib.parse
 
 import snapcraft.common
+import snapcraft.sources
 
 
 logger = logging.getLogger(__name__)
@@ -63,152 +61,23 @@
         return snapcraft.common.run(cmd, cwd=cwd, **kwargs)
 
     def isurl(self, url):
-        return urllib.parse.urlparse(url).scheme != ""
-
-    def pull_bzr(self, source, source_tag=None):
-        tag_opts = []
-        if source_tag:
-            tag_opts = ['-r', 'tag:' + source_tag]
-        if os.path.exists(os.path.join(self.sourcedir, ".bzr")):
-            return self.run(['bzr', 'pull'] + tag_opts + [source, '-d', self.sourcedir], cwd=os.getcwd())
-        else:
-            os.rmdir(self.sourcedir)
-            return self.run(['bzr', 'branch'] + tag_opts + [source, self.sourcedir], cwd=os.getcwd())
-
-    def pull_hg(self, source, source_tag=None, source_branch=None):
-        if source_tag and source_branch:
-            logger.error("You can't specify both source-tag and source-branch for a mercurial source (part '%s').", self.name)
-            snapcraft.common.fatal()
-
-        if os.path.exists(os.path.join(self.sourcedir, ".hg")):
-            ref = []
-            if source_tag:
-                ref = ['-r', source_tag]
-            elif source_branch:
-                ref = ['-b', source_branch]
-            return self.run(['hg', 'pull'] + ref + [source, ], cwd=os.getcwd())
-        else:
-            ref = []
-            if source_tag or source_branch:
-                ref = ['-u', source_tag or source_branch]
-
-            return self.run(['hg', 'clone'] + ref + [source, self.sourcedir], cwd=os.getcwd())
-
-    def pull_git(self, source, source_tag=None, source_branch=None):
-        if source_tag and source_branch:
-            logger.error("You can't specify both source-tag and source-branch for a git source (part '%s').", self.name)
-            snapcraft.common.fatal()
-
-        if os.path.exists(os.path.join(self.sourcedir, ".git")):
-            refspec = 'HEAD'
-            if source_branch:
-                refspec = 'refs/heads/' + source_branch
-            elif source_tag:
-                refspec = 'refs/tags/' + source_tag
-            return self.run(['git', '-C', self.sourcedir, 'pull', source, refspec], cwd=os.getcwd())
-        else:
-            branch_opts = []
-            if source_tag or source_branch:
-                branch_opts = ['--branch', source_tag or source_branch]
-            return self.run(['git', 'clone'] + branch_opts + [source, self.sourcedir], cwd=os.getcwd())
-
-    def pull_tarball(self, source, destdir=None):
-        destdir = destdir or self.sourcedir
-        if self.isurl(source):
-            return self.run(['wget', '-c', source], cwd=destdir)
-        else:
-            return True
-
-    def extract_tarball(self, source, srcdir=None, destdir=None):
-        srcdir = srcdir or self.sourcedir
-        destdir = destdir or self.builddir
-
-        if self.isurl(source):
-            tarball = os.path.join(srcdir, os.path.basename(source))
-        else:
-            tarball = os.path.abspath(source)
-
-        with tarfile.open(tarball) as tar:
-            def filter_members(tar):
-                """Filters members and member names:
-                    - strips common prefix
-                    - bans dangerous names"""
-                members = tar.getmembers()
-                common = os.path.commonprefix([m.name for m in members])
-
-                # commonprefix() works a character at a time and will
-                # consider "d/ab" and "d/abc" to have common prefix "d/ab";
-                # check all members either start with common dir
-                for m in members:
-                    if not (m.name.startswith(common + "/") or
-                            m.isdir() and m.name == common):
-                        # commonprefix() didn't return a dir name; go up one
-                        # level
-                        common = os.path.dirname(common)
-                        break
-
-                for m in members:
-                    if m.name == common:
-                        continue
-                    if m.name.startswith(common + "/"):
-                        m.name = m.name[len(common + "/"):]
-                    # strip leading "/", "./" or "../" as many times as needed
-                    m.name = re.sub(r'^(\.{0,2}/)*', r'', m.name)
-                    yield m
-
-            tar.extractall(members=filter_members(tar), path=destdir)
-
-        return True
+        return snapcraft.common.isurl(url)
 
     def get_source(self, source, source_type=None, source_tag=None, source_branch=None):
-        if source_type is None:
-            if source.startswith("bzr:") or source.startswith("lp:"):
-                source_type = 'bzr'
-            elif source.startswith("git:"):
-                source_type = 'git'
-            elif self.isurl(source):
-                logger.error("Unrecognized source '%s' for part '%s'.", source, self.name)
-                snapcraft.common.fatal()
-
-        if source_type == 'bzr':
-            if source_branch:
-                logger.error("You can't specify source-branch for a bzr source (part '%s').", self.name)
-                snapcraft.common.fatal()
-            if not self.pull_bzr(source, source_tag=source_tag):
-                return False
-            if not self.run(['cp', '-Trfa', self.sourcedir, self.builddir]):
-                return False
-        elif source_type == 'git':
-            if not self.pull_git(source, source_tag=source_tag, source_branch=source_branch):
-                return False
-            if not self.run(['cp', '-Trfa', self.sourcedir, self.builddir]):
-                return False
-        elif source_type == 'hg' or source_type == 'mercurial':
-            if not self.pull_hg(source, source_tag=source_tag, source_branch=source_branch):
-                return False
-            if not self.run(['cp', '-Trfa', self.sourcedir, self.builddir]):
-                return False
-        elif source_type == 'tar':
-            if source_branch:
-                logger.error("You can't specify source-branch for a tar source (part '%s').", self.name)
-                snapcraft.common.fatal()
-            if source_tag:
-                logger.error("You can't specify source-tag for a tar source (part '%s').", self.name)
-                snapcraft.common.fatal()
-            if not self.pull_tarball(source):
-                return False
-            if not self.extract_tarball(source):
-                return False
-        else:
-            # local source dir
-            path = os.path.abspath(source)
-            if os.path.isdir(self.builddir):
-                os.rmdir(self.builddir)
-            else:
-                os.remove(self.builddir)
-            os.symlink(path, self.builddir)
-
-        return True
+        try:
+            handler_class = _get_source_handler(source_type, source)
+        except ValueError:
+            logger.error("Unrecognized source '%s' for part '%s'.", source, self.name)
+            snapcraft.common.fatal()
+
+        try:
+            handler = handler_class(source, self.sourcedir, source_tag, source_branch)
+        except snapcraft.sources.IncompatibleOptionsError as e:
+            logger.error('Issues while setting up sources for part \'%s\': %s.', self.name, e.message)
+            snapcraft.common.fatal()
+        if not handler.pull():
+            return False
+        return handler.provision(self.builddir)
 
     def handle_source_options(self):
         stype = getattr(self.options, 'source_type', None)
@@ -221,3 +90,33 @@
 
     def makedirs(self, d):
         os.makedirs(d, exist_ok=True)
+
+
+def _get_source_handler(source_type, source):
+    if source_type is None:
+        source_type = _get_source_type_from_uri(source)
+
+    if source_type == 'bzr':
+        handler = snapcraft.sources.Bazaar
+    elif source_type == 'git':
+        handler = snapcraft.sources.Git
+    elif source_type == 'mercurial' or source_type == 'hg':
+        handler = snapcraft.sources.Mercurial
+    elif source_type == 'tar':
+        handler = snapcraft.sources.Tar
+    else:
+        handler = snapcraft.sources.Local
+
+    return handler
+
+
+def _get_source_type_from_uri(source):
+    source_type = ''
+    if source.startswith("bzr:") or source.startswith("lp:"):
+        source_type = 'bzr'
+    elif source.startswith("git:"):
+        source_type = 'git'
+    elif snapcraft.common.isurl(source):
+        raise ValueError()
+
+    return source_type

=== modified file 'snapcraft/common.py'
--- snapcraft/common.py	2015-08-25 20:32:40 +0000
+++ snapcraft/common.py	2015-09-01 10:10:15 +0000
@@ -20,6 +20,7 @@
 import subprocess
 import sys
 import tempfile
+import urllib
 
 
 COMMAND_ORDER = ["pull", "build", "stage", "snap"]
@@ -90,3 +91,7 @@
 
 def get_schemadir():
     return _schemadir
+
+
+def isurl(url):
+    return urllib.parse.urlparse(url).scheme != ""

=== modified file 'snapcraft/plugins/tar_content.py'
--- snapcraft/plugins/tar_content.py	2015-07-17 19:39:00 +0000
+++ snapcraft/plugins/tar_content.py	2015-09-01 10:10:15 +0000
@@ -15,12 +15,17 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import snapcraft
+import snapcraft.sources
 
 
 class TarContentPlugin(snapcraft.BasePlugin):
 
+    def __init__(self, name, options):
+        super().__init__(name, options)
+        self.tar = snapcraft.sources.Tar(self.options.source, self.builddir)
+
     def pull(self):
-        return self.pull_tarball(self.options.source, destdir=self.builddir)
+        return self.tar.pull()
 
     def build(self):
-        return self.extract_tarball(self.options.source, srcdir=self.builddir, destdir=self.installdir)
+        return self.tar.provision(self.installdir)

=== added file 'snapcraft/sources.py'
--- snapcraft/sources.py	1970-01-01 00:00:00 +0000
+++ snapcraft/sources.py	2015-09-01 10:10:15 +0000
@@ -0,0 +1,186 @@
+# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*-
+#
+# Copyright (C) 2015 Canonical Ltd
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3 as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# 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
+import os.path
+import tarfile
+import re
+
+import snapcraft.common
+
+
+logger = logging.getLogger(__name__)
+
+
+class IncompatibleOptionsError(Exception):
+
+    def __init__(self, message):
+        self.message = message
+
+
+class Base:
+
+    def __init__(self, source, source_dir, source_tag=None, source_branch=None):
+        self.source = source
+        self.source_dir = source_dir
+        self.source_tag = source_tag
+        self.source_branch = source_branch
+
+    def pull(self):
+        raise NotImplementedError('this is just a base class')
+
+    def provision(self, dst):
+        return snapcraft.common.run(['cp', '-Trfa', self.source_dir, dst], cwd=os.getcwd())
+
+
+class Bazaar(Base):
+
+    def __init__(self, source, source_dir, source_tag=None, source_branch=None):
+        super().__init__(source, source_dir, source_tag, source_branch)
+        if source_branch:
+            raise IncompatibleOptionsError('can\'t specify a source-branch for a bzr source')
+
+    def pull(self):
+        tag_opts = []
+        if self.source_tag:
+            tag_opts = ['-r', 'tag:' + self.source_tag]
+        if os.path.exists(os.path.join(self.source_dir, ".bzr")):
+            cmd = ['bzr', 'pull'] + tag_opts + [self.source, '-d', self.source_dir]
+        else:
+            os.rmdir(self.source_dir)
+            cmd = ['bzr', 'branch'] + tag_opts + [self.source, self.source_dir]
+
+        return snapcraft.common.run(cmd, cwd=os.getcwd())
+
+
+class Git(Base):
+
+    def __init__(self, source, source_dir, source_tag=None, source_branch=None):
+        super().__init__(source, source_dir, source_tag, source_branch)
+        if source_tag and source_branch:
+            raise IncompatibleOptionsError('can\'t specify both source-tag and source-branch for a git source')
+
+    def pull(self):
+        if os.path.exists(os.path.join(self.source_dir, ".git")):
+            refspec = 'HEAD'
+            if self.source_branch:
+                refspec = 'refs/heads/' + self.source_branch
+            elif self.source_tag:
+                refspec = 'refs/tags/' + self.source_tag
+            cmd = ['git', '-C', self.source_dir, 'pull', self.source, refspec]
+        else:
+            branch_opts = []
+            if self.source_tag or self.source_branch:
+                branch_opts = ['--branch', self.source_tag or self.source_branch]
+            cmd = ['git', 'clone'] + branch_opts + [self.source, self.source_dir]
+
+        return snapcraft.common.run(cmd, cwd=os.getcwd())
+
+
+class Mercurial(Base):
+
+    def __init__(self, source, source_dir, source_tag=None, source_branch=None):
+        super().__init__(source, source_dir, source_tag, source_branch)
+        if source_tag and source_branch:
+            raise IncompatibleOptionsError('can\'t specify both source-tag and source-branch for a mercurial source')
+
+    def pull(self):
+        if os.path.exists(os.path.join(self.source_dir, ".hg")):
+            ref = []
+            if self.source_tag:
+                ref = ['-r', self.source_tag]
+            elif self.source_branch:
+                ref = ['-b', self.source_branch]
+            cmd = ['hg', 'pull'] + ref + [self.source, ]
+        else:
+            ref = []
+            if self.source_tag or self.source_branch:
+                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())
+
+
+class Tar(Base):
+
+    def __init__(self, source, source_dir, source_tag=None, source_branch=None):
+        super().__init__(source, source_dir, source_tag, source_branch)
+        if source_tag:
+            raise IncompatibleOptionsError('can\'t specify a source-tag for a tar source')
+        elif source_branch:
+            raise IncompatibleOptionsError('can\'t specify a source-branch for a tar source')
+
+    def pull(self):
+        if snapcraft.common.isurl(self.source):
+            return snapcraft.common.run(['wget', '-q', '-c', self.source], cwd=self.source_dir)
+        else:
+            return True
+
+    def provision(self, dst):
+        # TODO add unit tests.
+        if snapcraft.common.isurl(self.source):
+            tarball = os.path.join(self.source_dir, os.path.basename(self.source))
+        else:
+            tarball = os.path.abspath(self.source)
+
+        with tarfile.open(tarball) as tar:
+            def filter_members(tar):
+                """Filters members and member names:
+                    - strips common prefix
+                    - bans dangerous names"""
+                members = tar.getmembers()
+                common = os.path.commonprefix([m.name for m in members])
+
+                # commonprefix() works a character at a time and will
+                # consider "d/ab" and "d/abc" to have common prefix "d/ab";
+                # check all members either start with common dir
+                for m in members:
+                    if not (m.name.startswith(common + "/") or
+                            m.isdir() and m.name == common):
+                        # commonprefix() didn't return a dir name; go up one
+                        # level
+                        common = os.path.dirname(common)
+                        break
+
+                for m in members:
+                    if m.name == common:
+                        continue
+                    if m.name.startswith(common + "/"):
+                        m.name = m.name[len(common + "/"):]
+                    # strip leading "/", "./" or "../" as many times as needed
+                    m.name = re.sub(r'^(\.{0,2}/)*', r'', m.name)
+                    yield m
+
+            tar.extractall(members=filter_members(tar), path=dst)
+
+        return True
+
+
+class Local(Base):
+
+    def pull(self):
+        return True
+
+    def provision(self, dst):
+        path = os.path.abspath(self.source)
+        if os.path.isdir(dst):
+            os.rmdir(dst)
+        else:
+            os.remove(dst)
+        os.symlink(path, dst)
+
+        return True

=== modified file 'snapcraft/tests/test_base_plugin.py'
--- snapcraft/tests/test_base_plugin.py	2015-08-05 18:13:37 +0000
+++ snapcraft/tests/test_base_plugin.py	2015-09-01 10:10:15 +0000
@@ -14,31 +14,14 @@
 # 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 http.server
 import fixtures
 import logging
 import os
-import threading
 
 import snapcraft
 from snapcraft import tests
 
 
-class FakeTarballHTTPRequestHandler(http.server.BaseHTTPRequestHandler):
-
-    def do_GET(self):
-        data = 'Test fake tarball file'
-        self.send_response(200)
-        self.send_header('Content-Length', len(data))
-        self.send_header('Content-type', 'text/html')
-        self.end_headers()
-        self.wfile.write(data.encode())
-
-    def log_message(self, *args):
-        # Overwritten so the test does not write to stderr.
-        pass
-
-
 class TestBasePlugin(tests.TestCase):
 
     def test_isurl(self):
@@ -49,26 +32,6 @@
         self.assertFalse(plugin.isurl('/foo'))
         self.assertFalse(plugin.isurl('/fo:o'))
 
-    def test_pull_tarball_must_download_to_sourcedir(self):
-        server = http.server.HTTPServer(('', 0), FakeTarballHTTPRequestHandler)
-        server_thread = threading.Thread(target=server.serve_forever)
-        self.addCleanup(server_thread.join)
-        self.addCleanup(server.server_close)
-        self.addCleanup(server.shutdown)
-        server_thread.start()
-
-        plugin_name = 'test_plugin'
-        dest_dir = os.path.join('parts', plugin_name, 'src')
-        os.makedirs(dest_dir)
-        tar_file_name = 'test.tar'
-        source = 'http://{}:{}/{file_name}'.format(
-            *server.server_address, file_name=tar_file_name)
-        plugin = snapcraft.BasePlugin(plugin_name, 'dummy_options')
-        plugin.pull_tarball(source)
-
-        with open(os.path.join(dest_dir, tar_file_name), 'r') as tar_file:
-            self.assertEqual('Test fake tarball file', tar_file.read())
-
     def test_get_source_with_unrecognized_source_must_raise_error(self):
         fake_logger = fixtures.FakeLogger(level=logging.ERROR)
         self.useFixture(fake_logger)
@@ -122,8 +85,9 @@
 
         self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
         expected = (
-            "You can't specify both source-tag and source-branch for a {} "
-            "source (part 'test_plugin').\n".format(self.source_type))
+            'Issues while setting up sources for part \'test_plugin\': '
+            'can\'t specify both source-tag and source-branch for a {} '
+            "source.\n".format(self.source_type))
         self.assertEqual(expected, fake_logger.output)
 
 
@@ -159,6 +123,6 @@
 
         self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
         expected = (
-            "You can't specify {} for a {} source "
-            "(part 'test_plugin').\n".format(self.error, self.source_type))
+            '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)

=== added file 'snapcraft/tests/test_sources.py'
--- snapcraft/tests/test_sources.py	1970-01-01 00:00:00 +0000
+++ snapcraft/tests/test_sources.py	2015-09-01 10:10:15 +0000
@@ -0,0 +1,61 @@
+# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*-
+#
+# Copyright (C) 2015 Canonical Ltd
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3 as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# 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 os
+import http.server
+import threading
+
+from snapcraft import sources
+from snapcraft import tests
+
+
+class FakeTarballHTTPRequestHandler(http.server.BaseHTTPRequestHandler):
+
+    def do_GET(self):
+        data = 'Test fake tarball file'
+        self.send_response(200)
+        self.send_header('Content-Length', len(data))
+        self.send_header('Content-type', 'text/html')
+        self.end_headers()
+        self.wfile.write(data.encode())
+
+    def log_message(self, *args):
+        # Overwritten so the test does not write to stderr.
+        pass
+
+
+class TestTar(tests.TestCase):
+
+    def test_pull_tarball_must_download_to_sourcedir(self):
+        server = http.server.HTTPServer(('', 0), FakeTarballHTTPRequestHandler)
+        server_thread = threading.Thread(target=server.serve_forever)
+        self.addCleanup(server_thread.join)
+        self.addCleanup(server.server_close)
+        self.addCleanup(server.shutdown)
+        server_thread.start()
+
+        plugin_name = 'test_plugin'
+        dest_dir = os.path.join('parts', plugin_name, 'src')
+        os.makedirs(dest_dir)
+        tar_file_name = 'test.tar'
+        source = 'http://{}:{}/{file_name}'.format(
+            *server.server_address, file_name=tar_file_name)
+        tar_source = sources.Tar(source, dest_dir)
+
+        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())